fix: the cursor Material's plan node id sits above the subplans' ids (#1306 follow-up)

CHANGELOG.md

@@ -87,6 +87,8 @@ true until the next version shipped.
   A cursor over a ctid range gets a Material above the scan, so `FETCH
   BACKWARD` and `FETCH LAST` work: PostgreSQL treats a TID range scan as able to
   scan backward, and without it the fetch reached the access method and failed.
+  The Material's plan node id now sits above the subplans' ids as well, so a
+  cursor whose query has a subquery no longer repeats a node id.
 
 ### Changed
 

src/columnar_tableam.c

@@ -963,6 +963,43 @@ pgcolumnar_plan_has_columnar_tidrange(Plan *plan, List *rtable, int *maxid)
 #define PGC_PLANNER_ARGS	parse, query_string, cursorOptions, boundParams
 #endif
 
+#ifdef USE_ASSERT_CHECKING
+/* Every plan_node_id in a plan tree, and whether any repeats. */
+static void
+pgcolumnar_plan_ids(Plan *plan, Bitmapset **seen, bool *dup)
+{
+	ListCell   *lc;
+
+	if (plan == NULL)
+		return;
+	if (bms_is_member(plan->plan_node_id, *seen))
+		*dup = true;
+	*seen = bms_add_member(*seen, plan->plan_node_id);
+	pgcolumnar_plan_ids(plan->lefttree, seen, dup);
+	pgcolumnar_plan_ids(plan->righttree, seen, dup);
+	switch (nodeTag(plan))
+	{
+		case T_Append:
+			foreach(lc, ((Append *) plan)->appendplans)
+				pgcolumnar_plan_ids(lfirst(lc), seen, dup);
+			break;
+		case T_MergeAppend:
+			foreach(lc, ((MergeAppend *) plan)->mergeplans)
+				pgcolumnar_plan_ids(lfirst(lc), seen, dup);
+			break;
+		case T_SubqueryScan:
+			pgcolumnar_plan_ids(((SubqueryScan *) plan)->subplan, seen, dup);
+			break;
+		case T_CustomScan:
+			foreach(lc, ((CustomScan *) plan)->custom_plans)
+				pgcolumnar_plan_ids(lfirst(lc), seen, dup);
+			break;
+		default:
+			break;
+	}
+}
+#endif
+
 static PlannedStmt *
 pgcolumnar_planner(PGC_PLANNER_PARAMS)
 {
@@ -1004,8 +1041,37 @@ pgcolumnar_planner(PGC_PLANNER_PARAMS)
 												   tle->resname, tle->resjunk));
 		}
 		mat->targetlist = tlist;
+
+		/*
+		 * A node id of its own, above every id in the final plan:
+		 * set_plan_references numbers the main tree and then the subplans from
+		 * one counter, so the main tree's largest id is not the largest (#1306
+		 * review). The walker above stops at the main tree, so the subplans'
+		 * ids are folded in here.
+		 */
+		foreach(lc, stmt->subplans)
+		{
+			int			subid = 0;
+
+			(void) pgcolumnar_plan_has_columnar_tidrange((Plan *) lfirst(lc),
+														 stmt->rtable, &subid);
+			maxid = Max(maxid, subid);
+		}
 		mat->plan_node_id = maxid + 1;
 		stmt->planTree = mat;
+#ifdef USE_ASSERT_CHECKING
+		{
+			/* plannodes.h: plan_node_id is unique across the final plan tree */
+			Bitmapset  *seen = NULL;
+			bool		dup = false;
+
+			pgcolumnar_plan_ids(stmt->planTree, &seen, &dup);
+			foreach(lc, stmt->subplans)
+				pgcolumnar_plan_ids((Plan *) lfirst(lc), &seen, &dup);
+			if (dup)
+				elog(ERROR, "pgcolumnar: duplicate plan_node_id after adding a Material above a cursor's plan");
+		}
+#endif
 	}
 	return stmt;
 }

test/check_ledger.tsv

@@ -1881,6 +1881,7 @@ storage_row_repair	storage_row_repair	premise: the damaged table still reads eve
 storage_row_repair	storage_row_repair	premise: the release notes give the query that finds wrong rows	15;16;17;18;19	never	-
 storage_row_repair	storage_row_repair	the notes' query finds both rows alpha4 left wrong	15;16;17;18;19	2026-10-12	the release notes' query is edited from NOT EXISTS to EXISTS
 tid_range_scan	tid_range_scan	a SCROLL cursor over a ctid range fetches backward as the filter's plan does	15;16;17;18;19	2026-10-12	no planner hook: a cursor over a TID range scan gets no Material
+tid_range_scan	tid_range_scan	a SCROLL cursor whose query has a subplan fetches backward as the filter's plan does	15;16;17;18;19	2026-10-12	the Material's node id from the main tree's largest, not above the subplans'
 tid_range_scan	tid_range_scan	a TID range scan rescanned with new bounds returns what the filter returns	15;16;17;18;19	2026-10-12	getnextslot_tidrange without the boundary-row filter;scan_set_tidrange keeping the reader of the previous range
 tid_range_scan	tid_range_scan	a ctid range over the last row groups reads under a fifth of the table	15;16;17;18;19	2026-10-12	scan_set_tidrange without the group restriction
 tid_range_scan	tid_range_scan	a ctid range returns what the per-row filter returns: ctid < '(515,0)'	15;16;17;18;19	2026-10-12	a lower bound at offset 0 read unclamped;an upper bound's offset past 291 read unclamped;getnextslot_tidrange without the boundary-row filter
@@ -1913,6 +1914,7 @@ tid_range_scan	tid_range_scan	premise: the cursor's query plans a TID range scan
 tid_range_scan	tid_range_scan	premise: the delete removed rows from the range	15;16;17;18;19	never	-
 tid_range_scan	tid_range_scan	premise: the per-range join rescans a TID range scan	15;16;17;18;19	never	-
 tid_range_scan	tid_range_scan	premise: the range runs as a parallel TID range scan with two workers	19	never	-
+tid_range_scan	tid_range_scan	premise: the subquery cursor plans a TID range scan and a subplan	15;16;17;18;19	never	-
 tid_range_scan	tid_range_scan	premise: the table has many row groups, so skipping can show	15;16;17;18;19	never	-
 tid_range_scan	tid_range_scan	premise: the table holds every row	15;16;17;18;19	never	-
 tid_range_scan	tid_range_scan	rows inserted earlier in the same transaction are in the range, as the filter sees them	15;16;17;18;19	never	-

test/check_ledger_budget.txt

@@ -1311,4 +1311,12 @@ suites_not_covered 247
 #     never         1674 + 19 = 1693
 #     dated          233 + 17 =  250
 #     1693 + 250 == 1943
-checks_never_observed_red 1693
+#
+# #1306 follow-up, plan_node_id (OffgridwithJD): 1693 -> 1694. Two checks: the
+# subplan-cursor arm, dated by the fix removed (red XX000 on an assert build,
+# PG16), and its premise, `never`.
+#
+#     ledger rows   1943 + 2 = 1945
+#     never         1693 + 1 = 1694
+#     dated          250 + 1 =  251
+checks_never_observed_red 1694

test/pytest/TESTS.md

@@ -6510,9 +6510,11 @@ Removal proofs, each red on PostgreSQL 19 on the arm named:
 - without the planner hook: both cursor arms;
 - with the hook for SCROLL cursors only: the plain-cursor arm.
 
+THE MATERIAL'S NODE ID. `set_plan_references` numbers the main tree and then the subplans from one counter, so the Material's id must be above the subplans' ids too; the first version took the main tree's largest and repeated a subplan's id (found by @jdatcmd). An assert build checks that every id in the final plan is unique and raises an error otherwise. So `a SCROLL cursor whose query has a subplan fetches backward as the filter's plan does` was red (`XX000`) before the fix on assert builds. On a release build it checks the fetches alone.
+
 ### Every test
 
 | test | what it holds |
 | --- | --- |
-| `test_a_ctid_range_reads_only_its_row_groups` | the table has its rows and many groups; a ctid range plans a TID range scan; eleven ranges, including offsets 0, 291, 500 and 65535, return what the per-row filter returns; a rescanned scan does too; a SCROLL cursor and a plain cursor fetch backward correctly; rows inserted earlier in the transaction are in the range; the tail range reads at least one block and under a fifth of the table; deleted rows stay deleted; after compaction moves rows, ranges over the old and new addresses return what the filter returns |
+| `test_a_ctid_range_reads_only_its_row_groups` | the table has its rows and many groups; a ctid range plans a TID range scan; eleven ranges, including offsets 0, 291, 500 and 65535, return what the per-row filter returns; a rescanned scan does too; a SCROLL cursor and a plain cursor fetch backward correctly, and so does a SCROLL cursor whose query has a subplan; rows inserted earlier in the transaction are in the range; the tail range reads at least one block and under a fifth of the table; deleted rows stay deleted; after compaction moves rows, ranges over the old and new addresses return what the filter returns |
 | `test_a_parallel_tid_range_scan_returns_what_the_filter_returns` | PostgreSQL 19+: the range runs as a parallel TID range scan with two workers launched, and returns what the filter returns. Below 19, `UNSUPPORTED_MAJOR` |

test/pytest/test_tid_range_scan.py

@@ -155,6 +155,32 @@ def cursor_run(tidscan, kind):
     expect.text(cursor_run("on", ""), cursor_run("off", "SCROLL"),
                 "a plain cursor over a ctid range answers a backward fetch correctly")
 
+    # A cursor whose query has a subplan: the Material's node id must not repeat
+    # one of the subplan's (#1306 review); an assert build checks they are unique.
+    sq = (f"SELECT k, (SELECT min(k) FROM tids) AS low FROM tids"
+          f" WHERE ctid > '{a}' AND ctid < '{b}'")
+    sub_plan = _plan(conn, sq)
+    expect.num(int("Tid Range Scan" in sub_plan) + int("InitPlan" in sub_plan), 2,
+               "premise: the subquery cursor plans a TID range scan and a subplan")
+
+    def sub_cursor(tidscan):
+        out = []
+        with conn.cursor() as cur:
+            cur.execute(f"SET enable_tidscan = {tidscan}")
+            cur.execute("BEGIN")
+            try:
+                cur.execute(f"DECLARE c SCROLL CURSOR FOR {sq}")
+                for f in ("FETCH LAST FROM c", "FETCH BACKWARD 3 FROM c"):
+                    cur.execute(f)
+                    out.append(",".join(f"{r[0]}/{r[1]}" for r in cur.fetchall()))
+            except psycopg.Error as e:
+                out.append(f"error {e.sqlstate}")
+            cur.execute("ROLLBACK")
+            cur.execute("RESET enable_tidscan")
+        return " | ".join(out)
+    expect.text(sub_cursor("on"), sub_cursor("off"),
+                "a SCROLL cursor whose query has a subplan fetches backward as the filter's plan does")
+
     # Rows written earlier in the same transaction are in the range.
     def same_xact(tidscan):
         with conn.cursor() as cur:

test/tid_range_scan.sh

@@ -92,6 +92,20 @@ check "a SCROLL cursor over a ctid range fetches backward as the filter's plan d
 check "a plain cursor over a ctid range answers a backward fetch correctly" \
 	"$(cursor_run on plain)" "$(cursor_run off SCROLL)"
 
+# A cursor whose query has a subplan: the Material's node id must not repeat one
+# of the subplan's (#1306 review). set_plan_references numbers the main tree, then
+# the subplans, from one counter; an assert build checks the ids are unique.
+SQ="SELECT id, (SELECT max(id) FROM tr) AS top FROM tr WHERE ctid >= '$MID_A' AND ctid <= '$MID_B'"
+sub_cursor() {	# sub_cursor on|off -> the fetches of a SCROLL cursor over SQ
+	env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -X -At \
+		-v VERBOSITY=sqlstate -c "SET enable_tidscan = $1" -c "BEGIN" -c "DECLARE c SCROLL CURSOR FOR $SQ" \
+		-c "FETCH LAST FROM c" -c "FETCH BACKWARD 2 FROM c" -c "COMMIT" 2>&1 | tr '\n' ' '
+}
+check "premise: the subquery cursor plans a TID range scan and a subplan" \
+	"$(q "EXPLAIN $SQ;" | grep -c 'Tid Range Scan\|InitPlan')" "2"
+check "a SCROLL cursor whose query has a subplan fetches backward as the filter's plan does" \
+	"$(sub_cursor on)" "$(sub_cursor off)"
+
 # Rows written earlier in the same transaction: set_tidrange reads the row-group
 # catalog, and the pending writes must be in it by then (#1306 review).
 same_xact() {	# same_xact on|off -> count|digest of the range after an insert in the same transaction