Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit92f33bb

Browse files
committed
Rethink definition of cancel.c's CancelRequested flag.
As it stands, this flag is only set when we've successfully sent acancel request, not if we get SIGINT and then fail to send a cancel.However, for almost all callers, that's the Wrong Thing: we'd preferto abort processing after control-C even if no cancel could be sent.As an example, since commit1d468b9 "pgbench -i" fails to give upsending COPY data even after control-C, if the postmaster has beenstopped, which is clearly not what the code intends and not what anyonewould want. (The fact that it keeps going at all is the fault of aseparate bug in libpq, but not letting CancelRequested become set isclearly not what we want here.)The sole exception, as far as I can find, is that scripts_parallel.c'sParallelSlotsGetIdle tries to consume a query result after issuing acancel, which of course might not terminate quickly if no cancelhappened. But that behavior was poorly thought out too. No user ofParallelSlotsGetIdle tries to continue processing after a cancel,so there is really no point in trying to clear the connection's state.Moreover this has the same defect as for other users of cancel.c,that if the cancel request fails for some reason then we end up withcontrol-C being completely ignored. (On top of that, select_loop failedto distinguish clearly between SIGINT and other reasons for select(2)failing, which means that it's possible that the existing code wouldthink that a cancel has been sent when it hasn't.)Hence, redefine CancelRequested as simply meaning that SIGINT wasreceived. We could add a second flag with the other meaning, butin the absence of any compelling argument why such a flag is needed,I think it would just offer an opportunity for future callers toget it wrong. Also remove the consumeQueryResult call inParallelSlotsGetIdle's failure exit. In passing, simplify theAPI of select_loop.It would now be possible to re-unify psql's cancel_pressed withCancelRequested, partly undoing5d43c3c. But I'm not reallyconvinced that that's worth the trouble, so I left psql alone,other than fixing a misleading comment.This code is new in v13 (cfa4fd3aa), so no need for back-patch.Per investigation of a complaint from Andres Freund.Discussion:https://postgr.es/m/20200603201242.ofvm4jztpqytwfye@alap3.anarazel.de
1 parent1fbb6c9 commit92f33bb

File tree

3 files changed

+19
-35
lines changed

3 files changed

+19
-35
lines changed

‎src/bin/psql/common.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ NoticeProcessor(void *arg, const char *message)
240240
* fgets are coded to handle possible interruption.
241241
*
242242
* On Windows, currently this does not work, so control-C is less useful
243-
* there, and the callback is just a no-op.
243+
* there.
244244
*/
245245
volatileboolsigint_interrupt_enabled= false;
246246

‎src/bin/scripts/scripts_parallel.c

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
#include"scripts_parallel.h"
2929

3030
staticvoidinit_slot(ParallelSlot*slot,PGconn*conn);
31-
staticintselect_loop(intmaxFd,fd_set*workerset,bool*aborting);
31+
staticintselect_loop(intmaxFd,fd_set*workerset);
3232

3333
staticvoid
3434
init_slot(ParallelSlot*slot,PGconn*conn)
@@ -39,25 +39,19 @@ init_slot(ParallelSlot *slot, PGconn *conn)
3939
}
4040

4141
/*
42-
*Loop on select()until a descriptor from the given set becomes readable.
42+
*Waituntil a file descriptor from the given set becomes readable.
4343
*
44-
* If we get a cancel request while we're waiting, we forego all further
45-
* processing and set the *aborting flag to true. The return value must be
46-
* ignored in this case. Otherwise, *aborting is set to false.
44+
* Returns the number of ready descriptors, or -1 on failure (including
45+
* getting a cancel request).
4746
*/
4847
staticint
49-
select_loop(intmaxFd,fd_set*workerset,bool*aborting)
48+
select_loop(intmaxFd,fd_set*workerset)
5049
{
5150
inti;
5251
fd_setsaveSet=*workerset;
5352

5453
if (CancelRequested)
55-
{
56-
*aborting= true;
5754
return-1;
58-
}
59-
else
60-
*aborting= false;
6155

6256
for (;;)
6357
{
@@ -90,7 +84,7 @@ select_loop(int maxFd, fd_set *workerset, bool *aborting)
9084
if (i<0&&errno==EINTR)
9185
continue;/* ignore this */
9286
if (i<0||CancelRequested)
93-
*aborting= true;/* but not this */
87+
return-1;/* but not this */
9488
if (i==0)
9589
continue;/* timeout (Win32 only) */
9690
break;
@@ -135,7 +129,6 @@ ParallelSlotsGetIdle(ParallelSlot *slots, int numslots)
135129
{
136130
fd_setslotset;
137131
intmaxFd=0;
138-
boolaborting;
139132

140133
/* We must reconstruct the fd_set for each call to select_loop */
141134
FD_ZERO(&slotset);
@@ -157,19 +150,12 @@ ParallelSlotsGetIdle(ParallelSlot *slots, int numslots)
157150
}
158151

159152
SetCancelConn(slots->connection);
160-
i=select_loop(maxFd,&slotset,&aborting);
153+
i=select_loop(maxFd,&slotset);
161154
ResetCancelConn();
162155

163-
if (aborting)
164-
{
165-
/*
166-
* We set the cancel-receiving connection to the one in the zeroth
167-
* slot above, so fetch the error from there.
168-
*/
169-
consumeQueryResult(slots->connection);
156+
/* failure? */
157+
if (i<0)
170158
returnNULL;
171-
}
172-
Assert(i!=0);
173159

174160
for (i=0;i<numslots;i++)
175161
{

‎src/fe_utils/cancel.c

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,11 @@
4343
staticPGcancel*volatilecancelConn=NULL;
4444

4545
/*
46-
* CancelRequestedtracks if a cancellation request has completed after
47-
*a signal interruption. Note that if cancelConn is not set, in short
48-
*if SetCancelConn() was never called or if ResetCancelConn() freed
49-
*the cancellation object, then CancelRequested is switched to true after
50-
*all cancellation attempts.
46+
* CancelRequestedis set when we receive SIGINT (or local equivalent).
47+
*There is no provision in this module for resetting it; but applications
48+
*might choose to clear it after successfully recovering from a cancel.
49+
*Note that there is no guarantee that we successfully sent a Cancel request,
50+
*or that the request will have any effect if we did send it.
5151
*/
5252
volatilesig_atomic_tCancelRequested= false;
5353

@@ -148,6 +148,8 @@ handle_sigint(SIGNAL_ARGS)
148148
intsave_errno=errno;
149149
charerrbuf[256];
150150

151+
CancelRequested= true;
152+
151153
if (cancel_callback!=NULL)
152154
cancel_callback();
153155

@@ -156,7 +158,6 @@ handle_sigint(SIGNAL_ARGS)
156158
{
157159
if (PQcancel(cancelConn,errbuf,sizeof(errbuf)))
158160
{
159-
CancelRequested= true;
160161
write_stderr(_("Cancel request sent\n"));
161162
}
162163
else
@@ -165,8 +166,6 @@ handle_sigint(SIGNAL_ARGS)
165166
write_stderr(errbuf);
166167
}
167168
}
168-
else
169-
CancelRequested= true;
170169

171170
errno=save_errno;/* just in case the write changed it */
172171
}
@@ -193,6 +192,8 @@ consoleHandler(DWORD dwCtrlType)
193192
if (dwCtrlType==CTRL_C_EVENT||
194193
dwCtrlType==CTRL_BREAK_EVENT)
195194
{
195+
CancelRequested= true;
196+
196197
if (cancel_callback!=NULL)
197198
cancel_callback();
198199

@@ -203,16 +204,13 @@ consoleHandler(DWORD dwCtrlType)
203204
if (PQcancel(cancelConn,errbuf,sizeof(errbuf)))
204205
{
205206
write_stderr(_("Cancel request sent\n"));
206-
CancelRequested= true;
207207
}
208208
else
209209
{
210210
write_stderr(_("Could not send cancel request: %s"));
211211
write_stderr(errbuf);
212212
}
213213
}
214-
else
215-
CancelRequested= true;
216214

217215
LeaveCriticalSection(&cancelConnLock);
218216

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp