Re: BUG #15395: Assert failure when using CURRENT OF with inheritance

=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> The following SQL script causes an Assert failure if PostgreSQL is compiled
> with asserts.

Nice catch!  This has evidently been broken for a long time.

> When run on a PostgreSQL instance with Asserts turned off,
> this script seems to work as expected. Thus, I suspect that it may be safe
> to just remove the Assert at execCurrent.c:239 but am not familiar enough
> with this part of the code to be sure.

No, I don't think that's a great fix.  After tracing through it, I can
see that what's going wrong is:

1. The plan tree for the cursor looks like

EXPLAIN DECLARE current_check_cursor SCROLL CURSOR FOR SELECT * FROM current_check;
                                QUERY PLAN
--------------------------------------------------------------------------
 Append  (cost=0.00..58.11 rows=2541 width=36)
   ->  Seq Scan on current_check  (cost=0.00..0.00 rows=1 width=36)
   ->  Seq Scan on current_check_1  (cost=0.00..22.70 rows=1270 width=36)
   ->  Seq Scan on current_check_2  (cost=0.00..22.70 rows=1270 width=36)
(4 rows)

2. The "FETCH ABSOLUTE 2" command runs through the single tuple available
from the seqscans on current_check and current_check_1, and then stops
on the first tuple available from current_check_2.

3. The "FETCH ABSOLUTE 1" command rewinds the executor and fetches the
first available tuple, which is in current_check_1.

4. At this point, the SeqScan node for current_check has been run to
completion, so its ScanTupleSlot is empty.  The node for current_check_1
is correctly positioned on a tuple.  The node for current_check_2 has
not been visited yet, so its state is as ExecReScan left it.  The trouble
is that the only thing that's happened to its ScanTupleSlot is where
heapam.c's initscan() does

    ItemPointerSetInvalid(&scan->rs_ctup.t_self);

It's only rather accidental that the plan node's ScanTupleSlot is
pointing at that at all, but it is.  So execCurrent.c sees an apparently
valid scan tuple containing an invalid t_self, and it spits up, which
I think is entirely a good thing.


I think that the correct fix is to ensure that the ScanTupleSlot gets
cleared during ExecReScan, which fortunately is pretty easy to mechanize
because all scan node types call ExecScanReScan.  So the execScan.c
part of the patch below seems to be enough to resolve the reported case.

However, we're not really out of the woods yet, because I noticed that
places like ExecReScanAppend will postpone calling ExecReScan if there's
a chgParam flag set on their sub-nodes.  This means that we can't
necessarily rely on ExecutorRewind performing an immediate call of
ExecReScan for every scan node in the plan.  It seems to me, therefore,
that we need to teach execCurrent.c to check for pending-rescan flags
and not believe that a node is really on a tuple if it finds such a
flag at or above the node.  The execCurrent.c part of the attached patch
does that.

I've not tried yet to create an actual test case for the chgParam-based
failure.  It's definitely possible that that problem is only hypothetical
at the moment because cursor plans that would satisfy search_plan_tree
would be too simple to contain any such flags.  But I think we'd better
add that logic anyway.

            regards, tom lane

diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index f70e54f..01139e4 100644
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -23,7 +23,8 @@


 static char *fetch_cursor_param_value(ExprContext *econtext, int paramId);
-static ScanState *search_plan_tree(PlanState *node, Oid table_oid);
+static ScanState *search_plan_tree(PlanState *node, Oid table_oid,
+                 bool *pending_rescan);


 /*
@@ -156,8 +157,10 @@ execCurrentOf(CurrentOfExpr *cexpr,
          * aggregation.
          */
         ScanState  *scanstate;
+        bool        pending_rescan = false;

-        scanstate = search_plan_tree(queryDesc->planstate, table_oid);
+        scanstate = search_plan_tree(queryDesc->planstate, table_oid,
+                                     &pending_rescan);
         if (!scanstate)
             ereport(ERROR,
                     (errcode(ERRCODE_INVALID_CURSOR_STATE),
@@ -177,8 +180,12 @@ execCurrentOf(CurrentOfExpr *cexpr,
                      errmsg("cursor \"%s\" is not positioned on a row",
                             cursor_name)));

-        /* Now OK to return false if we found an inactive scan */
-        if (TupIsNull(scanstate->ss_ScanTupleSlot))
+        /*
+         * Now OK to return false if we found an inactive scan.  It is
+         * inactive either if it's not positioned on a row, or there's a
+         * rescan pending for it.
+         */
+        if (TupIsNull(scanstate->ss_ScanTupleSlot) || pending_rescan)
             return false;

         /*
@@ -291,10 +298,20 @@ fetch_cursor_param_value(ExprContext *econtext, int paramId)
  *
  * Search through a PlanState tree for a scan node on the specified table.
  * Return NULL if not found or multiple candidates.
+ *
+ * If a candidate is found, set *pending_rescan to true if that candidate
+ * or any node above it has a pending rescan action, i.e. chgParam != NULL.
+ * That indicates that we shouldn't consider the node to be positioned on a
+ * valid tuple, even if its own state would indicate that it is.  (Caller
+ * must initialize *pending_rescan to false, and should not trust its state
+ * if multiple candidates are found.)
  */
 static ScanState *
-search_plan_tree(PlanState *node, Oid table_oid)
+search_plan_tree(PlanState *node, Oid table_oid,
+                 bool *pending_rescan)
 {
+    ScanState  *result = NULL;
+
     if (node == NULL)
         return NULL;
     switch (nodeTag(node))
@@ -314,7 +331,7 @@ search_plan_tree(PlanState *node, Oid table_oid)
                 ScanState  *sstate = (ScanState *) node;

                 if (RelationGetRelid(sstate->ss_currentRelation) == table_oid)
-                    return sstate;
+                    result = sstate;
                 break;
             }

@@ -325,13 +342,13 @@ search_plan_tree(PlanState *node, Oid table_oid)
         case T_AppendState:
             {
                 AppendState *astate = (AppendState *) node;
-                ScanState  *result = NULL;
                 int            i;

                 for (i = 0; i < astate->as_nplans; i++)
                 {
                     ScanState  *elem = search_plan_tree(astate->appendplans[i],
-                                                        table_oid);
+                                                        table_oid,
+                                                        pending_rescan);

                     if (!elem)
                         continue;
@@ -339,7 +356,7 @@ search_plan_tree(PlanState *node, Oid table_oid)
                         return NULL;    /* multiple matches */
                     result = elem;
                 }
-                return result;
+                break;
             }

             /*
@@ -348,13 +365,13 @@ search_plan_tree(PlanState *node, Oid table_oid)
         case T_MergeAppendState:
             {
                 MergeAppendState *mstate = (MergeAppendState *) node;
-                ScanState  *result = NULL;
                 int            i;

                 for (i = 0; i < mstate->ms_nplans; i++)
                 {
                     ScanState  *elem = search_plan_tree(mstate->mergeplans[i],
-                                                        table_oid);
+                                                        table_oid,
+                                                        pending_rescan);

                     if (!elem)
                         continue;
@@ -362,7 +379,7 @@ search_plan_tree(PlanState *node, Oid table_oid)
                         return NULL;    /* multiple matches */
                     result = elem;
                 }
-                return result;
+                break;
             }

             /*
@@ -371,18 +388,31 @@ search_plan_tree(PlanState *node, Oid table_oid)
              */
         case T_ResultState:
         case T_LimitState:
-            return search_plan_tree(node->lefttree, table_oid);
+            result = search_plan_tree(node->lefttree,
+                                      table_oid,
+                                      pending_rescan);
+            break;

             /*
              * SubqueryScan too, but it keeps the child in a different place
              */
         case T_SubqueryScanState:
-            return search_plan_tree(((SubqueryScanState *) node)->subplan,
-                                    table_oid);
+            result = search_plan_tree(((SubqueryScanState *) node)->subplan,
+                                      table_oid,
+                                      pending_rescan);
+            break;

         default:
             /* Otherwise, assume we can't descend through it */
             break;
     }
-    return NULL;
+
+    /*
+     * If we found a candidate at or below this node, then this node's
+     * chgParam indicates a pending rescan that will affect this node.
+     */
+    if (result && node->chgParam != NULL)
+        *pending_rescan = true;
+
+    return result;
 }
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index caf9173..f84a3fb 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -263,6 +263,12 @@ ExecScanReScan(ScanState *node)
 {
     EState       *estate = node->ps.state;

+    /*
+     * We must clear the scan tuple so that observers (e.g., execCurrent.c)
+     * can tell that this plan node is not positioned on a tuple.
+     */
+    ExecClearTuple(node->ss_ScanTupleSlot);
+
     /* Rescan EvalPlanQual tuple if we're inside an EvalPlanQual recheck */
     if (estate->es_epqScanDone != NULL)
     {