From 73e98664edb2d730eb7d0d5aeb9ff6bd923221cd Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 25 Sep 2026 17:11:01 -0600 Subject: [PATCH 1/4] fix: CREATE MATERIALIZED VIEW WITH DATA orphaned its storage row (#1275) CREATE MATERIALIZED VIEW ... USING pgcolumnar ... WITH DATA builds a transient relation, fills it and swaps, exactly as a rewriting ALTER does. The storage row was written with the transient's OID and the swap left it naming a relation that no longer existed: the row named a dropped 16527 while the matview was 16523 REFRESH MATERIALIZED VIEW already repaired it, because RefreshMatViewStmt is one of the node types the repair gate covers. CreateTableAsStmt was not, so a matview stayed broken between CREATE and its first REFRESH. RESTRICTED TO objtype == OBJECT_MATVIEW, AND A CONTROL SAYS WHY. CREATE TABLE ... AS ... USING pgcolumnar is the same parse node and does NOT have the defect -- measured, its storage row points at itself, because it fills the relation it created instead of swapping a transient in. Without that control the fix would reasonably have been written for the node type as a whole, which is broader than anything measured asked for. test/rewrite_storage_oid.sh asserts it as a premise, so the narrowing is refused the day it stops being true rather than being silently wrong. ITS OWN ARM RATHER THAN THE EXISTING CAST. The comment already standing there warns that AlterTableStmt and RefreshMatViewStmt share a field offset "by coincidence of layout, not by any rule"; CreateTableAsStmt does not share it at all, reaching its relation through ->into->rel. pgcolumnar_rewritten_relids is EMPTY at that point (measured), so the recorded-list route the TRUNCATE path uses is unavailable and the statement's own name is the only handle. Step 1 measured before any code was written, with the node kind, the depth, the recorded list and the resolved relid printed per statement: CREATE MATERIALIZED VIEW ... WITH DATA CreateTableAs/MATVIEW recorded=0 into_rel=16523 CREATE TABLE ... AS (columnar) CreateTableAs/TABLE recorded=0 into_rel=16532 REFRESH MATERIALIZED VIEW (control) RefreshMatView recorded=0 into_rel=0 ALTER TABLE ... TYPE (control) The ALTER control DID NOT RUN -- it is refused by the matview's dependency on the column, so the statement errors and the probe, which sits after the PG_TRY block, never executes. Evidence of nothing, and recorded as such rather than retried a third way. Removal proofs, both harnesses, with the mutation asserted on the host AND in the container copy (0 occurrences of CreateTableAsStmt in each): shell -- mv=0 cta=1 FAIL a matview created WITH DATA leaves its storage row pointing at itself: got [0] want [1] 10 passed + 1 failed pytest AssertionError: ... got 0 want 1 Green: -- mv=1 cta=1, 11 passed + 0 failed, and the pytest twin 11 checks. Neighbours on PG17: projection_rewrite 84/84, preimage_rewrite 86/86, alter_am_cleanup 45/45, truncate_cleanup 25/25, native_rewrite 17/17, drop_cleanup 16/16, native_ctas 12/12, projection_rename_restore 8/8, rewrite_group_scan 5/5. Ledger regenerated from five majors, each rc=0 with 11 RESULT records and one source fingerprint: rows 1804 -> 1810, never 1627 -> 1633, budget updated to match the census, 6 added and 0 removed or altered. was still wrong for every other reader until this. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n --- CHANGELOG.md | 26 +++++++++ src/columnar_tableam.c | 39 ++++++++++++- test/check_ledger.tsv | 6 ++ test/check_ledger_budget.txt | 2 +- test/pytest/TESTS.md | 7 +++ test/pytest/expected_tests.txt | 17 ++++-- test/pytest/test_rewrite_storage_oid.py | 76 +++++++++++++++++++++++++ test/rewrite_storage_oid.sh | 55 ++++++++++++++++++ 8 files changed, 220 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5ae8098..0ddd6725 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,32 @@ true until the next version shipped. ### Fixed +- `CREATE MATERIALIZED VIEW ... USING pgcolumnar ... WITH DATA` no longer + orphans `pgcolumnar.storage.relation_oid` (#1275). The statement builds a + transient relation, fills it and swaps, exactly as a rewriting `ALTER` does, + so the storage row was written with the transient's OID and the swap left it + naming a relation that no longer existed. Measured: the row named a dropped + `16527` while the matview was `16523`, and a lookup by the live relation found + nothing. + + `REFRESH MATERIALIZED VIEW` already repaired it, because that node type was in + the repair gate `ALTER`, `TRUNCATE` and `REFRESH` share. `CreateTableAsStmt` + was not, so a matview stayed broken between `CREATE` and its first `REFRESH`. + + **Restricted to `objtype == OBJECT_MATVIEW`, and a control says why.** + `CREATE TABLE ... AS ... USING pgcolumnar` is the same parse node and does + **not** have the defect — it fills the relation it created instead of swapping + a transient in — so the remedy is narrower than the node type. + `test/rewrite_storage_oid.sh` asserts that as a premise, which refuses the + narrowing the day it stops being true. + + The statement's own name is the only handle: `pgcolumnar_rewritten_relids` is + empty at that point, so the recorded-list route the `TRUNCATE` path uses is + not available. + + `pgcolumnar.analyze()` stopped reading this column in #1276; the column itself + was still wrong for every other reader until this. + - Planning a query over a columnar relation no longer sequentially scans `pgcolumnar.storage` (#1210). `pgcolumnar_written_stripe_row_limit` looked its row up by `relation_oid`, which has no index, so the scan walked the catalog diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index f54e538d..3fea8587 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -2897,7 +2897,7 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, */ if (parsetree != NULL && (IsA(parsetree, AlterTableStmt) || IsA(parsetree, TruncateStmt) || - IsA(parsetree, RefreshMatViewStmt))) + IsA(parsetree, RefreshMatViewStmt) || IsA(parsetree, CreateTableAsStmt))) { List *targets = NIL; ListCell *lc; @@ -2939,7 +2939,42 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, * layout, not by any rule, and one added field in either struct turns * it into a wrong pointer with no diagnostic. */ - if (IsA(parsetree, RefreshMatViewStmt)) + if (IsA(parsetree, CreateTableAsStmt)) + { + /* + * CREATE MATERIALIZED VIEW ... WITH DATA (#1275). It builds a + * transient, fills it and swaps, exactly as a rewriting ALTER + * does, so the storage row is written with the transient's OID + * and the swap leaves it naming a relation that no longer + * exists. Measured: the row named a dropped 16527 while the + * matview was 16523, and a lookup by the live relation found + * nothing until the first REFRESH repaired it through the + * RefreshMatViewStmt arm above. + * + * ITS OWN ARM RATHER THAN THE CAST BELOW, for the reason the + * comment above already gives about the other two: they share a + * field offset by coincidence of layout, and this node does not + * share it at all -- its relation is reached through + * ->into->rel. + * + * MATVIEWS ONLY, AND A CONTROL SAYS SO. `CREATE TABLE ... AS + * ... USING pgcolumnar` is the same parse node and does NOT have + * the defect -- measured, its storage row points at itself, + * because it fills the relation it created instead of swapping + * a transient in. test/rewrite_storage_oid.sh asserts that as a + * premise, so the day it stops being true this narrowing is + * refused rather than silently wrong. + * + * pgcolumnar_rewritten_relids is EMPTY here (measured), so the + * recorded-list route the TRUNCATE path uses is not available + * and the statement's own name is the only handle. + */ + CreateTableAsStmt *cts = (CreateTableAsStmt *) parsetree; + + rv = (cts->objtype == OBJECT_MATVIEW && cts->into) + ? cts->into->rel : NULL; + } + else if (IsA(parsetree, RefreshMatViewStmt)) rv = ((RefreshMatViewStmt *) parsetree)->relation; else rv = ((AlterTableStmt *) parsetree)->relation; diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 478d9721..7975663e 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -1772,8 +1772,14 @@ range_pruning range_pruning premise: the range type collates differently from it range_pruning range_pruning so does a containment probe 15;16;17;18;19 never - range_pruning range_pruning the overlap qual is pushed down, where it used to be dropped 15;16;17;18;19 never - range_pruning range_pruning the same for a containment probe far to its right 15;16;17;18;19 never - +rewrite_storage_oid rewrite_storage_oid a matview created WITH DATA leaves its storage row pointing at itself 15;16;17;18;19 never - rewrite_storage_oid rewrite_storage_oid a type rewrite leaves the storage row pointing at the live table 15;16;17;18;19 2026-09-24 the rewrite hook does not retarget storage.relation_oid +rewrite_storage_oid rewrite_storage_oid and REFRESH still leaves it pointing at itself 15;16;17;18;19 never - +rewrite_storage_oid rewrite_storage_oid premise: CREATE TABLE ... AS leaves its storage row pointing at itself 15;16;17;18;19 never - rewrite_storage_oid rewrite_storage_oid premise: a table that was not rewritten still points at itself 15;16;17;18;19 never - +rewrite_storage_oid rewrite_storage_oid premise: the CREATE TABLE AS table holds them too 15;16;17;18;19 never - +rewrite_storage_oid rewrite_storage_oid premise: the matview holds the rows it was created with 15;16;17;18;19 never - +rewrite_storage_oid rewrite_storage_oid premise: the refreshed matview still holds every row 15;16;17;18;19 never - rewrite_storage_oid rewrite_storage_oid premise: the rewritten table still holds every row 15;16;17;18;19 never - rewrite_storage_oid rewrite_storage_oid premise: the storage row points at the live table before the rewrite 15;16;17;18;19 never - rewrite_storage_oid rewrite_storage_oid premise: the table holds every inserted row 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 89ecdc49..c187d40c 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -1177,4 +1177,4 @@ suites_not_covered 247 # REBASED FROM 1bcfb92b ONTO 81ec3ddc, 32 commits, by the maintainer rather than # the author. The branch predates #1261, and three of its five CI reds were # fixed by the rebase alone rather than by any change here -- see the PR. -checks_never_observed_red 1627 +checks_never_observed_red 1633 diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index eef9fe32..a309e637 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -6285,8 +6285,15 @@ THE ONE THING THIS HALF HAS TO DO THAT THE SHELL HALF DOES NOT is flush the stat Port of `rewrite_storage_oid.sh`. `ALTER COLUMN ... TYPE` writes `pgcolumnar.storage.relation_oid` as the transient relation and then drops it. The live table's OID no longer finds the row. The rows themselves stay readable. +AND THE SAME DEFECT ON A SECOND STATEMENT (#1275). `CREATE MATERIALIZED VIEW ... AS` with data builds a transient, fills it and swaps, exactly as a rewriting `ALTER` does, so the storage row named a relation that no longer existed. Measured before the fix: the row named a dropped 16527 while the matview was 16523. `REFRESH MATERIALIZED VIEW` already repaired it, because that node type was in the repair gate; `CreateTableAsStmt` was not. + +THE `CREATE TABLE ... AS` ARM IS THE CONTROL THAT NARROWS THE FIX, and it is the reason the remedy is restricted to `objtype == OBJECT_MATVIEW` rather than applied to the parse node. Same node, no defect -- it fills the relation it created instead of swapping a transient in. Without that control the fix would reasonably have been written for `CreateTableAsStmt` as a whole, which is broader than anything measured asked for, and the premise now refuses the narrowing the day it stops being true. + +THE `REFRESH` ARM GUARDS THE PATH THAT ALREADY WORKED. It repaired this before the fix, through a different node type, so an arm there is what says the new arm did not displace it. + ### Every test | test | what it holds | | --- | --- | | `test_rewrite_storage_oid` | the storage row matches the live regclass before the rewrite, the row count survives, an unrewritten table still matches itself, and the rewritten table's storage row matches the live regclass afterwards | +| `test_a_matview_created_with_data_points_at_itself` | that the matview and the `CREATE TABLE AS` table both hold their rows, that `CREATE TABLE ... AS` points at itself (the control that narrows the fix), that a matview created `WITH DATA` points at itself, and that `REFRESH` still does | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 12e452b7..f17473f5 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -530,10 +530,17 @@ guard_tests 404 # 1bcfb92b. Carrying that forward would have been a count measured on a tree # that no longer exists; 483 is measured on this one. # REBASED ONTO 5389b35a AFTER #1277 MERGED, AND THE NUMBER WAS COLLECTED RATHER -# THAN ADDED UP. Both sides moved this key -- #1277 took it to 486 and this +# THAN ADDED UP. Both sides moved this key -- #1277 took it to 486 and that # branch had 485 against the older base -- so git CONFLICTED, which is the loud # case this file's own history calls the lucky one. 486 + 1 is the arithmetic -# and 487 is what `pytest --collect-only` over the 53 cluster files reports on -# the merged tree. They agree here; the point is that the second one is evidence -# and the first is a guess that happens to be right. -cluster_tests 487 +# and 487 is what `pytest --collect-only` over the 53 cluster files reported on +# the merged tree. They agreed; the point is that the second one is evidence and +# the first is a guess that happens to be right. +# +# AND IT HAPPENED AGAIN IMMEDIATELY, WHICH IS THE POINT OF THE NOTE ABOVE. +# #1278 and #1275 were both cut from 5389b35a and both moved this key to 487 for +# DIFFERENT tests. #1278 merged first, so 487 is now main's value and #1275's +# own test is not in it. Git conflicted again -- loudly, because the key moved +# on both sides -- and the answer is neither branch's number. Collected on the +# rebased tree, not added up. +cluster_tests 488 diff --git a/test/pytest/test_rewrite_storage_oid.py b/test/pytest/test_rewrite_storage_oid.py index 7ee74efb..93bbb022 100644 --- a/test/pytest/test_rewrite_storage_oid.py +++ b/test/pytest/test_rewrite_storage_oid.py @@ -72,3 +72,79 @@ def test_rewrite_storage_oid(pgc_conn, expect): 1, "a type rewrite leaves the storage row pointing at the live table", ) + + +def _points_at(conn, relname): + return _one( + conn, + f"SELECT (relation_oid = '{relname}'::regclass)::int" + f" FROM pgcolumnar.storage" + f" WHERE storage_id = pgcolumnar.get_storage_id('{relname}')", + ) + + +def test_a_matview_created_with_data_points_at_itself(pgc_conn, expect): + """The same defect on a different statement (#1275). + + `CREATE MATERIALIZED VIEW ... AS` with data builds a transient, fills it and + swaps, exactly as a rewriting `ALTER` does, so the storage row is written + with the transient's OID and the swap leaves it naming a relation that no + longer exists. Measured before the fix: the row named a dropped 16527 while + the matview was 16523. + + `REFRESH MATERIALIZED VIEW` already repaired it, because that node type was + in the repair gate. `CreateTableAsStmt` was not. + + THE `CREATE TABLE ... AS` ARM IS THE CONTROL THAT NARROWS THE CLAIM. Same + parse node, and it does NOT have the defect -- it fills the relation it + created instead of swapping a transient in. Without it the fix would + reasonably have been written for the node type as a whole, which is broader + than anything measured asked for. + """ + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE mv_src (id int, note text) USING pgcolumnar") + cur.execute("INSERT INTO mv_src SELECT g, 'n'||g FROM generate_series(1,900) g") + cur.execute("CREATE MATERIALIZED VIEW mv_data USING pgcolumnar AS" + " SELECT id, note FROM mv_src") + cur.execute("CREATE TABLE mv_cta USING pgcolumnar AS SELECT id, note FROM mv_src") + + # A ROW COUNT FIRST, so the arms below are about a relation that was + # actually populated. A matview created WITH NO DATA writes no storage row + # at all, and every arm here would then be asking about nothing. + expect.num( + _one(pgc_conn, "SELECT count(*) FROM mv_data"), + 900, + "premise: the matview holds the rows it was created with", + ) + expect.num( + _one(pgc_conn, "SELECT count(*) FROM mv_cta"), + 900, + "premise: the CREATE TABLE AS table holds them too", + ) + + mv, cta = _points_at(pgc_conn, "mv_data"), _points_at(pgc_conn, "mv_cta") + print(f"-- mv={mv} cta={cta}") + + # THE CONTROL, AND IT MUST PASS BEFORE THE FIX AS WELL AS AFTER. If this + # ever reads 0 the claim below is no longer about matviews specifically and + # the remedy is a different one. + expect.num( + cta, 1, "premise: CREATE TABLE ... AS leaves its storage row pointing at itself" + ) + expect.num( + mv, 1, "a matview created WITH DATA leaves its storage row pointing at itself" + ) + + # AND REFRESH MUST STILL WORK. It repaired this before the fix, through a + # different node type, so an arm here is what says the fix did not displace + # the path that already worked. + with pgc_conn.cursor() as cur: + cur.execute("REFRESH MATERIALIZED VIEW mv_data") + expect.num( + _one(pgc_conn, "SELECT count(*) FROM mv_data"), + 900, + "premise: the refreshed matview still holds every row", + ) + expect.num( + _points_at(pgc_conn, "mv_data"), 1, "and REFRESH still leaves it pointing at itself" + ) diff --git a/test/rewrite_storage_oid.sh b/test/rewrite_storage_oid.sh index c36ae2de..d518443e 100755 --- a/test/rewrite_storage_oid.sh +++ b/test/rewrite_storage_oid.sh @@ -48,4 +48,59 @@ echo "-- live=$(live_rows)" check_num "a type rewrite leaves the storage row pointing at the live table" \ "$(live_rows)" "1" +# --- and CREATE MATERIALIZED VIEW ... WITH DATA (#1275) ---------------------- +# +# The same defect on a different statement. `CREATE MATERIALIZED VIEW ... AS` +# with data builds a transient, fills it, and swaps -- exactly like the type +# change above -- so the storage row is written with the transient's OID and the +# swap leaves it naming a relation that no longer exists. +# +# `REFRESH MATERIALIZED VIEW` already repairs it, because RefreshMatViewStmt is +# one of the node types the repair runs for. A CreateTableAsStmt was not. +# +# THE `CREATE TABLE ... AS` ARM IS THE CONTROL THAT NARROWS THE CLAIM. It is the +# same parse node and it does NOT have the defect -- measured before the fix, +# rows_by_relation_oid = 1 -- because it fills the relation it created rather +# than swapping a transient in. Without it the fix would reasonably have been +# written for CreateTableAsStmt as a whole, which is broader than anything +# measured asked for. +q "CREATE MATERIALIZED VIEW rew_mv USING pgcolumnar AS + SELECT id, note FROM rew_oid;" >/dev/null +q "CREATE TABLE rew_cta USING pgcolumnar AS + SELECT id, note FROM rew_oid;" >/dev/null + +points_at() { # points_at RELNAME -> 1 when its storage row names it + q "SELECT (relation_oid = '$1'::regclass)::int + FROM pgcolumnar.storage + WHERE storage_id = pgcolumnar.get_storage_id('$1');" +} + +# A ROW COUNT FIRST, so the arms below are about a relation that was actually +# populated. A matview created WITH NO DATA writes no storage row at all, and +# every arm here would then be asking about nothing. +check_num "premise: the matview holds the rows it was created with" \ + "$(q "SELECT count(*) FROM rew_mv;")" "900" +check_num "premise: the CREATE TABLE AS table holds them too" \ + "$(q "SELECT count(*) FROM rew_cta;")" "900" + +echo "-- mv=$(points_at rew_mv) cta=$(points_at rew_cta)" + +# THE CONTROL, AND IT MUST PASS BEFORE THE FIX AS WELL AS AFTER. If this ever +# reads 0 the claim below is no longer about matviews specifically, and the +# remedy is a different one. +check_num "premise: CREATE TABLE ... AS leaves its storage row pointing at itself" \ + "$(points_at rew_cta)" "1" + +check_num "a matview created WITH DATA leaves its storage row pointing at itself" \ + "$(points_at rew_mv)" "1" + +# AND REFRESH MUST STILL WORK. It repaired this before the fix, through a +# different node type, so an arm here is what says the fix did not displace the +# path that already worked. +q "REFRESH MATERIALIZED VIEW rew_mv;" >/dev/null +check_num "premise: the refreshed matview still holds every row" \ + "$(q "SELECT count(*) FROM rew_mv;")" "900" +check_num "and REFRESH still leaves it pointing at itself" \ + "$(points_at rew_mv)" "1" + pgc_summary From 99fcca5c314f2988e364d05a0fde6e46034e57f6 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 25 Sep 2026 17:17:54 -0600 Subject: [PATCH 2/4] test: #1276's matview arm asserted the orphan this change removes (#1275) `pytest (cluster tests, PG 18)` went red on this branch with one failure: premise: the matview's relation_oid still finds no storage row: got 1 want 0 THAT IS THE PREMISE DOING ITS JOB. #1276 added that arm to record a behaviour change -- a WITH DATA matview had an orphaned relation_oid, so analyze() keyed on that column refused a matview holding rows while the metapage route resolved it -- and its premise asserted the orphan was still there. This change removes the orphan, so the premise reddened instead of letting an arm pass quietly under a claim that no longer meant anything. WHAT IS LEFT TO GUARD, AND WHAT IS NOT. With the orphan gone both routes resolve, so that shape no longer distinguishes them and is NOT evidence for #1276's fix -- the projection arms beside it are, and they are untouched. What it still holds is that analyze() reaches a freshly created matview at all, which was an outright error before #1276, and that the two routes agree about WHICH storage. THE IDS, NOT THEIR COUNTS. Two rows counting 1 each can still be two DIFFERENT storages, which is the failure this arm would most want to see, so it compares storage ids: -- a WITH DATA matview: by relation_oid=10000000008, by metapage=10000000008, analyze() succeeded That is #1275's guarantee cross-checked from a different file than the one asserting it. Both harnesses, names kept in parity: premise: the matview's relation_oid finds a storage row the two routes agree which storage a WITH DATA matview owns pgcolumnar.analyze() reaches a matview created WITH DATA analyze_function 68 passed + 0 failed (PG18) test_analyze_function.py + test_compare_to_bash.py 45 passed Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n --- test/analyze_function.sh | 46 +++++++++++++---------- test/pytest/test_analyze_function.py | 56 +++++++++++++++++++--------- 2 files changed, 65 insertions(+), 37 deletions(-) diff --git a/test/analyze_function.sh b/test/analyze_function.sh index bea23253..5f3c3f88 100755 --- a/test/analyze_function.sh +++ b/test/analyze_function.sh @@ -944,33 +944,39 @@ check_text "premise: the capture helper reports a success when there is one" \ check_text "pgcolumnar.analyze() still refuses a relation that has never been written" \ "$empty_state" "refused P0001" -# AND THE BEHAVIOUR CHANGE IS ASSERTED, NOT LEFT TO BE DISCOVERED. A matview -# created WITH DATA has an ORPHANED relation_oid until its first REFRESH -# (#1275): the storage row points at a transient relation that no longer -# exists. Keyed on relation_oid this found nothing and refused a matview holding -# rows; through the metapage it resolves and succeeds. -# -# THIS IS NOT #1275 BEING FIXED. The orphan is still in the catalog and every -# other reader of relation_oid still meets it. Only this caller stops hitting -# it, and an arm that says so is the difference between a recorded change and a -# reader concluding the orphan is gone. +# A MATVIEW CREATED WITH DATA, AND THIS ARM HAS BEEN SUPERSEDED ONCE ALREADY. +# +# #1276 added it to record a behaviour change: a WITH DATA matview had an +# ORPHANED relation_oid until its first REFRESH, so keyed on that column +# analyze() refused a matview holding rows, and through the metapage it +# resolved. Its premise asserted the orphan was still there -- and #1275 then +# removed the orphan, so the premise went red. That is the premise doing its +# job: it recorded a fact about the world and reddened when the world changed, +# rather than passing quietly under an arm that no longer meant anything. +# +# WHAT IS LEFT TO GUARD, AND WHAT IS NOT. With the orphan gone, both routes +# resolve, so this shape no longer distinguishes them and is NOT evidence for +# #1276's fix -- the projection arms above are. What it still holds is that +# analyze() reaches a freshly created matview at all, which was an outright +# error before #1276, and that the two routes agree about WHICH storage, which +# is #1275's guarantee cross-checked from a different file. q "DROP MATERIALIZED VIEW IF EXISTS af_mv; CREATE MATERIALIZED VIEW af_mv USING pgcolumnar AS SELECT a, b, c FROM af_proj;" >/dev/null -mv_by_reloid="$(q "SELECT count(*) FROM pgcolumnar.storage +mv_by_reloid="$(q "SELECT coalesce(min(storage_id)::text, 'none') FROM pgcolumnar.storage WHERE relation_oid = 'af_mv'::regclass::oid;")" -mv_by_meta="$(q "SELECT count(*) FROM pgcolumnar.storage +mv_by_meta="$(q "SELECT coalesce(min(storage_id)::text, 'none') FROM pgcolumnar.storage WHERE storage_id = pgcolumnar.get_storage_id('af_mv'::regclass);")" mv_state="$(q "SELECT af_try('af_mv'::regclass);")" -echo "-- a WITH DATA matview: rows by relation_oid=$mv_by_reloid, by metapage id=$mv_by_meta, analyze() $mv_state" +echo "-- a WITH DATA matview: by relation_oid=$mv_by_reloid, by metapage=$mv_by_meta, analyze() $mv_state" -# THE ORPHAN MUST STILL BE THERE, or this arm is measuring a matview that was -# never in the broken state and says nothing about the change. -check_num "premise: the matview's relation_oid still finds no storage row" \ - "$mv_by_reloid" "0" -check_num "premise: and its metapage still finds exactly one" \ - "$mv_by_meta" "1" +# THE IDS, NOT THEIR COUNTS. Two rows counting 1 each can still be two +# DIFFERENT storages, and that is the failure this arm would most want to see. +check_text "premise: the matview's relation_oid finds a storage row" \ + "$(if [ "$mv_by_reloid" = "none" ]; then echo none; else echo found; fi)" "found" +check_text "the two routes agree which storage a WITH DATA matview owns" \ + "$mv_by_meta" "$mv_by_reloid" -check_text "pgcolumnar.analyze() now reaches a matview whose relation_oid is orphaned" \ +check_text "pgcolumnar.analyze() reaches a matview created WITH DATA" \ "$mv_state" "succeeded" diff --git a/test/pytest/test_analyze_function.py b/test/pytest/test_analyze_function.py index 51940205..31b1c625 100644 --- a/test/pytest/test_analyze_function.py +++ b/test/pytest/test_analyze_function.py @@ -851,29 +851,51 @@ def test_a_covering_projection_does_not_decide_which_columns_get_statistics(pgc_ "pgcolumnar.analyze() still refuses a relation that has never been written", ) - # AND THE BEHAVIOUR CHANGE IS ASSERTED, NOT LEFT TO BE DISCOVERED. A matview - # created WITH DATA has an orphaned relation_oid until its first REFRESH - # (#1275). Keyed on relation_oid this refused a matview holding rows; - # through the metapage it resolves. THAT IS NOT #1275 BEING FIXED -- the - # orphan is still in the catalog for every other reader. + # A MATVIEW CREATED WITH DATA, AND THIS ARM HAS BEEN SUPERSEDED ONCE ALREADY. + # + # #1276 added it to record a behaviour change: a WITH DATA matview had an + # orphaned relation_oid until its first REFRESH, so keyed on that column + # analyze() refused a matview holding rows and through the metapage it + # resolved. Its premise asserted the orphan was still there -- and #1275 + # then removed the orphan, so the premise went red. That is the premise + # doing its job: it recorded a fact about the world and reddened when the + # world changed, rather than passing quietly under an arm that no longer + # meant anything. + # + # WITH THE ORPHAN GONE both routes resolve, so this shape no longer + # distinguishes them and is NOT evidence for #1276's fix -- the projection + # arms above are. What it still holds is that analyze() reaches a freshly + # created matview at all, which was an outright error before #1276, and + # that the two routes agree about WHICH storage. _exec(conn, "CREATE MATERIALIZED VIEW ap_mv USING pgcolumnar AS SELECT a, b, c FROM ap") - mv_by_reloid = int( - _one(conn, "SELECT count(*) FROM pgcolumnar.storage WHERE relation_oid = 'ap_mv'::regclass::oid") + by_reloid = _one( + conn, + "SELECT coalesce(min(storage_id)::text, 'none') FROM pgcolumnar.storage" + " WHERE relation_oid = 'ap_mv'::regclass::oid", ) - mv_by_meta = int( - _one( - conn, - "SELECT count(*) FROM pgcolumnar.storage" - " WHERE storage_id = pgcolumnar.get_storage_id('ap_mv'::regclass)", - ) + by_meta = _one( + conn, + "SELECT coalesce(min(storage_id)::text, 'none') FROM pgcolumnar.storage" + " WHERE storage_id = pgcolumnar.get_storage_id('ap_mv'::regclass)", ) mv_state = _try_analyze(conn, "ap_mv") - print(f"-- a WITH DATA matview: by relation_oid={mv_by_reloid}, by metapage={mv_by_meta}, analyze() {mv_state}") + print(f"-- a WITH DATA matview: by relation_oid={by_reloid}, by metapage={by_meta}, analyze() {mv_state}") - expect.num(mv_by_reloid, 0, "premise: the matview's relation_oid still finds no storage row") - expect.num(mv_by_meta, 1, "premise: and its metapage still finds exactly one") + # THE IDS, NOT THEIR COUNTS. Two rows counting 1 each can still be two + # DIFFERENT storages, and that is the failure this arm would most want to + # see. + expect.text( + "none" if str(by_reloid) == "none" else "found", + "found", + "premise: the matview's relation_oid finds a storage row", + ) + expect.text( + str(by_meta), + str(by_reloid), + "the two routes agree which storage a WITH DATA matview owns", + ) expect.text( mv_state, "succeeded", - "pgcolumnar.analyze() now reaches a matview whose relation_oid is orphaned", + "pgcolumnar.analyze() reaches a matview created WITH DATA", ) From 49d2afc19b99e4ee2cdd1a5d5410db1cf198d056 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 25 Sep 2026 17:20:40 -0600 Subject: [PATCH 3/4] docs: CHANGELOG carried two em dashes and docs_style refuses them (#1275) FAIL CHANGELOG.md carries no em or en dash: got [2] want [0] Both mine, in this branch's CHANGELOG entry. Replaced with the ASCII `--` the rest of the file uses. FOUND ONLY BY THE FULL CORPUS RUN. Nine targeted suite runs and both harnesses were green; `docs_style.sh` is not a neighbour of anything this change touches, so nothing in a reasonable list of affected suites would have reached it. That is the argument for the corpus run rather than a curated subset, and it is worth recording because the curated subset otherwise looks sufficient. TESTS.md holds 238 em dashes already and docs_style does not read it; none of them is mine. Checked rather than assumed: every line this branch adds, across all files, carries zero. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ddd6725..bccb21a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,8 +32,8 @@ true until the next version shipped. **Restricted to `objtype == OBJECT_MATVIEW`, and a control says why.** `CREATE TABLE ... AS ... USING pgcolumnar` is the same parse node and does - **not** have the defect — it fills the relation it created instead of swapping - a transient in — so the remedy is narrower than the node type. + **not** have the defect -- it fills the relation it created instead of + swapping a transient in -- so the remedy is narrower than the node type. `test/rewrite_storage_oid.sh` asserts that as a premise, which refuses the narrowing the day it stops being true. From 004e801b561f64175c14a73189500a73cd326ce8 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 25 Sep 2026 17:29:50 -0600 Subject: [PATCH 4/4] docs: the NoLock reason above this block does not cover a CREATE (#1275) @jdatcmd's review: the lock is held and the call is right, but the justification standing above it was written for TRUNCATE and for a type change -- the statement already holds AccessExclusiveLock on the hierarchy, so NoLock takes nothing new -- and those operate on relations that existed before the statement began. A CREATE MATERIALIZED VIEW has no hierarchy and no pre-existing relation. The lock is held because the creating transaction holds AccessExclusiveLock on a relation it has just made, and this block runs after standard_ProcessUtility so the create has completed. Same conclusion, different derivation, and without the clause the next reader inherits the stated reason rather than the true one. I had flagged that I never re-derived it; that was the right worry and this is the answer. AND THE NARROWING IS EXHAUSTIVE RATHER THAN MERELY UNFALSIFIED. I asked whether a CreateTableAsStmt could swap a transient without being a matview. parsenodes.h annotates CreateTableAsStmt.objtype as OBJECT_TABLE or OBJECT_MATVIEW and nothing else, so `objtype == OBJECT_MATVIEW` partitions the node type. SELECT INTO is OBJECT_TABLE, which the CREATE TABLE ... AS control already measures, not a third case. Read on the 15, 17 and 19 headers rather than on one, because this ships across the matrix and a header annotation is exactly the kind of thing that differs between majors. All three agree. Comment only. Rebuilt and re-run rather than assumed inert, because a comment moves __LINE__: build rc=0, 0 warnings rewrite_storage_oid 11 passed + 0 failed The first draft of this clause embedded parsenodes.h's own `/* ... */` and escaped the terminator. It compiled, and it was the wrong thing to ship: nested comment delimiters are a -Wcomment hazard and unreadable besides. Rewritten as prose naming the annotation instead of quoting it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n --- src/columnar_tableam.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 3fea8587..0aae6319 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -2965,6 +2965,26 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, * premise, so the day it stops being true this narrowing is * refused rather than silently wrong. * + * THE SET IS CLOSED AT TWO VALUES, so this partitions the node + * type rather than guessing at it. parsenodes.h annotates + * CreateTableAsStmt.objtype as OBJECT_TABLE or OBJECT_MATVIEW, + * and nothing else -- read on the 15, 17 and 19 headers rather + * than on one, since this ships across the matrix. SELECT INTO + * is OBJECT_TABLE, so it is the control's case and not a third + * one. Reported by @jdatcmd. + * + * NoLock, AND FOR A CREATE THE REASON IS NOT THE ONE STATED + * ABOVE. That comment says the statement already holds + * AccessExclusiveLock ON THE HIERARCHY, which was written for + * TRUNCATE and for a type change -- statements that operate on + * relations existing before they began. A CREATE has no + * hierarchy and no pre-existing relation: the lock is held + * because the creating transaction holds AccessExclusiveLock on + * a relation it has just made, and this block runs after + * standard_ProcessUtility, so the create has completed. Same + * conclusion, different derivation, and the next reader would + * otherwise inherit the stated reason rather than the true one. + * * pgcolumnar_rewritten_relids is EMPTY here (measured), so the * recorded-list route the TRUNCATE path uses is not available * and the statement's own name is the only handle.