Fix query-cancel handling in spgdoinsert().

 Knowing that a buggy opclass could cause an infinite insertion loop,
 spgdoinsert() intended to allow its loop to be interrupted by query
 cancel.  However, that never actually worked, because in iterations
 after the first, we'd be holding buffer lock(s) which would cause
 InterruptHoldoffCount to be positive, preventing servicing of the
 interrupt.
 
 To fix, check if an interrupt is pending, and if so fall out of
 the insertion loop and service the interrupt after we've released
 the buffers.  If it was indeed a query cancel, that's the end of
 the matter.  If it was a non-canceling interrupt reason, make use
 of the existing provision to retry the whole insertion.  (This isn't
 as wasteful as it might seem, since any upper-level index tuples we
 already created should be usable in the next attempt.)
 
 While there's no known instance of such a bug in existing release
 branches, it still seems like a good idea to back-patch this to
 all supported branches, since the behavior is fairly nasty if a
 loop does happen --- not only is it uncancelable, but it will
 quickly consume memory to the point of an OOM failure.  In any
 case, this code is certainly not working as intended.
 
 Per report from Dilip Kumar.
 
 Discussion: https://postgr.es/m/CAFiTN-uxP_soPhVG840tRMQTBmtA_f_Y8N51G7DKYYqDh7XN-A@mail.gmail.com
 
 

diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index 4d380c99f06f59f046491f769f0816d678282d88..88ae2499dd543fe86b77fc6b7c4a9cec1d305b34 100644 (file)
--- 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;
@@ -2012,6 +2013,14 @@ spgdoinsert(Relation index, SpGistState *state,
    parent.offnum = InvalidOffsetNumber;
    parent.node = -1;
 
+   /*
+    * Before entering the loop, try to clear any pending interrupt condition.
+    * If a query cancel is pending, we might as well accept it now not later;
+    * while if a non-canceling condition is pending, servicing it here avoids
+    * having to restart the insertion and redo all the work so far.
+    */
+   CHECK_FOR_INTERRUPTS();
+
    for (;;)
    {
        bool        isNew = false;
@@ -2019,9 +2028,18 @@ 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.  However, because we'll be holding buffer lock(s)
+        * after the first iteration, ProcessInterrupts() wouldn't be able to
+        * throw a cancel error here.  Hence, if we see that an interrupt is
+        * pending, break out of the loop and deal with the situation below.
+        * Set result = false because we must restart the insertion if the
+        * interrupt isn't a query-cancel-or-die case.
         */
-       CHECK_FOR_INTERRUPTS();
+       if (INTERRUPTS_PENDING_CONDITION())
+       {
+           result = false;
+           break;
+       }
 
        if (current.blkno == InvalidBlockNumber)
        {
@@ -2140,10 +2158,14 @@ 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 +2289,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 here.  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;
 }