Alvaro Herrera <alvhe...@alvh.no-ip.org> writes:
> (Looking again, the nbtpage.c hunk might have been made obsolete by
> c34787f91058 and other commits).
OK. Here's a revision that adopts your idea, except that I left
out the nbtpage.c change since you aren't sure of that part.
I added a macro that allows spgdoinsert to Assert that it's not
called in a context where the infinite-loop-due-to-InterruptPending
risk would arise. This is a little bit fragile, because it'd be
easy for ill-considered changes to ProcessInterrupts to break it,
but it's better than nothing.
regards, tom lane
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index 4d380c99f0..1c14fc7f76 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -1905,13 +1905,14 @@ spgSplitNodeAction(Relation index, SpGistState *state,
* Insert one item into the index.
*
* Returns true on success, false if we failed to complete the insertion
- * because of conflict with a concurrent insert. In the latter case,
- * caller should re-call spgdoinsert() with the same args.
+ * (typically because of conflict with a concurrent insert). In the latter
+ * case, caller should re-call spgdoinsert() with the same args.
*/
bool
spgdoinsert(Relation index, SpGistState *state,
ItemPointer heapPtr, Datum *datums, bool *isnulls)
{
+ bool result = true;
TupleDesc leafDescriptor = state->leafTupDesc;
bool isnull = isnulls[spgKeyColumn];
int level = 0;
@@ -2019,9 +2020,17 @@ spgdoinsert(Relation index, SpGistState *state,
/*
* Bail out if query cancel is pending. We must have this somewhere
* in the loop since a broken opclass could produce an infinite
- * picksplit loop.
+ * picksplit loop. Moreover, because it's typically the case that
+ * we're holding buffer lock(s), ProcessInterrupts() won't be able to
+ * throw an error now. Hence, if we see that the interrupt condition
+ * remains true, break out of the loop and deal with the case below.
*/
CHECK_FOR_INTERRUPTS();
+ if (INTERRUPTS_PENDING_CONDITION())
+ {
+ result = false;
+ break;
+ }
if (current.blkno == InvalidBlockNumber)
{
@@ -2140,10 +2149,15 @@ spgdoinsert(Relation index, SpGistState *state,
* spgAddNode and spgSplitTuple cases will loop back to here to
* complete the insertion operation. Just in case the choose
* function is broken and produces add or split requests
- * repeatedly, check for query cancel.
+ * repeatedly, check for query cancel (see comments above).
*/
process_inner_tuple:
CHECK_FOR_INTERRUPTS();
+ if (INTERRUPTS_PENDING_CONDITION())
+ {
+ result = false;
+ break;
+ }
innerTuple = (SpGistInnerTuple) PageGetItem(current.page,
PageGetItemId(current.page, current.offnum));
@@ -2267,5 +2281,21 @@ spgdoinsert(Relation index, SpGistState *state,
UnlockReleaseBuffer(parent.buffer);
}
- return true;
+ /*
+ * We do not support being called while some outer function is holding a
+ * buffer lock (or any other reason to postpone query cancels). If that
+ * were the case, telling the caller to retry would create an infinite
+ * loop.
+ */
+ Assert(INTERRUPTS_CAN_BE_PROCESSED());
+
+ /*
+ * Finally, check for interrupts again. If there was a query cancel,
+ * ProcessInterrupts() will be able to throw the error now. If it was
+ * some other kind of interrupt that can just be cleared, return false to
+ * tell our caller to retry.
+ */
+ CHECK_FOR_INTERRUPTS();
+
+ return result;
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 6200699ddd..dd2ade7bb6 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -554,7 +554,7 @@ ProcessClientWriteInterrupt(bool blocked)
{
/*
* Don't mess with whereToSendOutput if ProcessInterrupts wouldn't
- * do anything.
+ * service ProcDiePending.
*/
if (InterruptHoldoffCount == 0 && CritSectionCount == 0)
{
@@ -3118,6 +3118,12 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
* If an interrupt condition is pending, and it's safe to service it,
* then clear the flag and accept the interrupt. Called only when
* InterruptPending is true.
+ *
+ * Note: if INTERRUPTS_CAN_BE_PROCESSED() is true, then ProcessInterrupts
+ * is guaranteed to clear the InterruptPending flag before returning.
+ * (This is not the same as guaranteeing that it's still clear when we
+ * return; another interrupt could have arrived. But we promise that
+ * any pre-existing one will have been serviced.)
*/
void
ProcessInterrupts(void)
@@ -3248,7 +3254,11 @@ ProcessInterrupts(void)
{
/*
* Re-arm InterruptPending so that we process the cancel request as
- * soon as we're done reading the message.
+ * soon as we're done reading the message. (XXX this is seriously
+ * ugly: it complicates INTERRUPTS_CAN_BE_PROCESSED(), and it means we
+ * can't use that macro directly as the initial test in this function,
+ * meaning that this code also creates opportunities for other bugs to
+ * appear.)
*/
InterruptPending = true;
}
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 95202d37af..645766a4ab 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -57,6 +57,11 @@
* allowing die interrupts: HOLD_CANCEL_INTERRUPTS() and
* RESUME_CANCEL_INTERRUPTS().
*
+ * Also, INTERRUPTS_PENDING_CONDITION() can be checked to see whether an
+ * interrupt needs to be serviced, without trying to do so immediately.
+ * Some callers are also interested in INTERRUPTS_CAN_BE_PROCESSED(),
+ * which tells whether ProcessInterrupts is sure to clear the interrupt.
+ *
* Special mechanisms are used to let an interrupt be accepted when we are
* waiting for a lock or when we are waiting for command input (but, of
* course, only if the interrupt holdoff counter is zero). See the
@@ -97,24 +102,27 @@ extern PGDLLIMPORT volatile uint32 CritSectionCount;
/* in tcop/postgres.c */
extern void ProcessInterrupts(void);
+/* Test whether an interrupt is pending */
#ifndef WIN32
+#define INTERRUPTS_PENDING_CONDITION() \
+ (unlikely(InterruptPending))
+#else
+#define INTERRUPTS_PENDING_CONDITION() \
+ (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? pgwin32_dispatch_queued_signals() : 0, \
+ unlikely(InterruptPending))
+#endif
+/* Service interrupt if one is pending */
#define CHECK_FOR_INTERRUPTS() \
do { \
- if (unlikely(InterruptPending)) \
- ProcessInterrupts(); \
-} while(0)
-#else /* WIN32 */
-
-#define CHECK_FOR_INTERRUPTS() \
-do { \
- if (unlikely(UNBLOCKED_SIGNAL_QUEUE())) \
- pgwin32_dispatch_queued_signals(); \
- if (unlikely(InterruptPending)) \
+ if (INTERRUPTS_PENDING_CONDITION()) \
ProcessInterrupts(); \
} while(0)
-#endif /* WIN32 */
+/* Is ProcessInterrupts() guaranteed to clear InterruptPending? */
+#define INTERRUPTS_CAN_BE_PROCESSED() \
+ (InterruptHoldoffCount == 0 && CritSectionCount == 0 && \
+ QueryCancelHoldoffCount == 0)
#define HOLD_INTERRUPTS() (InterruptHoldoffCount++)
