diff --git a/CHANGELOG.md b/CHANGELOG.md index 13fa9e4e6..a70196462 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index daebcc57c..493450acc 100644 --- a/src/columnar_tableam.c +++ b/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; } diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 5d60165b8..c35b6484b 100644 --- a/test/check_ledger.tsv +++ b/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 - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 9001004e8..085e02c2a 100644 --- a/test/check_ledger_budget.txt +++ b/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 diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index d2d3d9c20..892419589 100644 --- a/test/pytest/TESTS.md +++ b/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` | diff --git a/test/pytest/test_tid_range_scan.py b/test/pytest/test_tid_range_scan.py index 17315f394..b392dc4ec 100644 --- a/test/pytest/test_tid_range_scan.py +++ b/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: diff --git a/test/tid_range_scan.sh b/test/tid_range_scan.sh index bb6671bf1..f65a3cf40 100755 --- a/test/tid_range_scan.sh +++ b/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