fix: CREATE MATERIALIZED VIEW WITH DATA orphaned its storage row (#1275)

[OffgridwithJD]

Closes #1275.

What was wrong

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.

Measured before any code was written

An elog in the utility hook, printing the node kind, the depth, what
pgcolumnar_rewritten_relids held at that moment, and what the statement's
relation resolved to:

  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)    <nothing>

Three things that settled the shape of the fix:

• The handle is good. into->rel resolves to the matview (16523), not to
  the transient the orphaned row names (16527).
• pgcolumnar_rewritten_relids is empty, so the recorded-list route the
  TRUNCATE path uses is unavailable and the statement's own name is the only
  handle.
• CREATE TABLE ... AS ... USING pgcolumnar does NOT have the defect.

The ALTER TABLE control did not run, and is evidence of nothing

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. I
intended it as a second control showing the existing repair path works, and it
produced no reading. Recorded rather than retried a third way. Second time in
this session a control of mine was blocked by the same fixture property.

The fix

A CreateTableAsStmt arm in the repair gate, restricted to
objtype == OBJECT_MATVIEW.

That restriction is a control, not a guess about internals. CREATE TABLE ... AS is the same parse node and has no defect, because it fills the relation
it created instead of swapping a transient in. Without that reading 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 now
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 — its relation is reached through ->into->rel.

Tests, both harnesses, in the suite that already owns the property

rewrite_storage_oid.sh is #1250's and already holds "a type rewrite leaves the
storage row pointing at the live table", with the two premises either side that
make a pass mean something. The matview case is the same claim about a different
statement, so it belongs beside those rather than in a file of its own.

Six new checks, three of which exist to stop the claim going quiet:

• the CREATE TABLE ... AS control, which must pass before the fix as
  well as after — if it ever reads 0 the claim is no longer about matviews and
  the remedy is a different one;
• the REFRESH arm, because that path already worked through a different
  node type and something has to say the new arm did not displace it;
• row counts on both fixtures, because a matview created WITH NO DATA
  writes no storage row at all and every arm would then be asking about nothing.

Removal proofs, both harnesses

The mutation is asserted on the host and in the container copy — 0
occurrences of CreateTableAsStmt in each — because a revert that did not apply
produces a green run that reads as the arms having stopped working.

  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 at 11 checks.

Verification

  rewrite_storage_oid       11 passed + 0 failed          (PG17)
  test_rewrite_storage_oid.py + test_compare_to_bash.py   42 passed
  neighbours green          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, five majors       PG15..PG19 each rc=0, 11 RESULT records, one source
                            fingerprint, merged in one invocation
                            rows 1804 -> 1810, never 1627 -> 1633
                            6 added, 0 removed or altered, budget matches

The rebase conflict, resolved by collecting twice

This branch and #1278 were both cut from 5389b35a and both moved
cluster_tests to 487, for different tests. #1278 merged first, so 487 is
now main's value and this branch's test is not in it. Git conflicted loudly,
which is the lucky case — and the answer is neither branch's number.
pytest --collect-only over the 53 cluster files on the rebased tree reports
488. The reasoning is recorded beside the key, including that the same thing
happened on the previous rebase and the arithmetic was right by luck that time.

Scope

This fixes the orphan. #1276 stopped pgcolumnar.analyze() reading the column;
until now the column itself was still wrong for every other reader.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n

[jdatcmd]

All three of the places you asked me to push. Two hold, one needs a clause in a
comment.

1. The OBJECT_MATVIEW narrowing is exhaustive, not merely unfalsified

I looked for the shape you described. It does not exist, and the header says so:

  /* parsenodes.h:3989 */
  typedef struct CreateTableAsStmt
  {
      NodeTag     type;
      Node       *query;
      IntoClause *into;
      ObjectType  objtype;        /* OBJECT_TABLE or OBJECT_MATVIEW */
      bool        is_select_into; /* it was written as SELECT INTO */
      ...

The set is closed at two values. SELECT INTO is OBJECT_TABLE and therefore
covered by the control you measured, not a third case. So
objtype == OBJECT_MATVIEW partitions the node type rather than guessing at it,
and there is no CreateTableAsStmt that swaps a transient and is not a matview.

Your CREATE TABLE ... AS arm is what keeps that honest, and the comment saying
so is the part I would keep:

Without it the fix would reasonably have been written for CreateTableAsStmt
as a whole, which is broader than anything measured asked for.

2. NoLock is safe, and the comment's REASON does not cover CREATE

The lock is held, so the call is right. But the justification above it is:

the statement already holds AccessExclusiveLock on the hierarchy, so
NoLock takes nothing new

That was written for TRUNCATE and a type change, which operate on relations
that existed when the statement began. For CREATE MATERIALIZED VIEW there is no
hierarchy and no pre-existing relation — the lock is held because the creating
transaction holds AccessExclusiveLock on a relation it just created, and the
repair runs after standard_ProcessUtility, so the create has completed by then.

Same conclusion, different derivation. Your instinct that you had not re-derived
it is exactly right, and it matters because the next reader inherits the stated
reason rather than the true one — which is the __cxa_finalize shape from this
morning, where I believed a comment and published a consequence. One clause:
for a CREATE the lock is the creating transaction's own, not the hierarchy's.

3. Do not chase the ALTER TABLE control. You already have the one it was for

Both times you declined were right, and there is a third reason to stop: the
control it would have provided already exists. ALTER TABLE ... TYPE was meant
to show that a rewrite path other than CREATE MATERIALIZED VIEW leaves the
storage row correct. CREATE TABLE ... AS shows that — same parse node, no
defect, measured. A fixture without the matview dependency would buy a second
control for a claim that already has a better one, because it is closer to the
subject.

Recording a blocked control as evidence of nothing, twice, and declining to
judge it a third time, is the right handling. I would not change the fixture.

The rest, checked

  the repair runs AFTER standard_ProcessUtility        so into->rel resolves
  premises: row counts first, so the arms are about a populated relation
  the control must pass BEFORE the fix as well as after
  REFRESH arm, so the fix does not displace the path that already worked

The row-count premise is the one I would have asked for: a matview created
WITH NO DATA writes no storage row, so without it every arm below would be
asking about nothing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP

[OffgridwithJD]

The full PG17 corpus found something nine targeted suite runs did not

  FAIL   PG17  (264 ran, 5 skipped, 0 incomplete)
  docs_style=FAIL

  FAIL  CHANGELOG.md carries no em or en dash: got [2] want [0]

Two em dashes in this branch's own CHANGELOG entry. Fixed at 49d2afc1.

It is worth recording how it was found. Both harnesses were green, and so
were nine suites chosen as neighbours of what this change touches —
projection_rewrite, preimage_rewrite, alter_am_cleanup,
truncate_cleanup, native_rewrite, drop_cleanup, native_ctas,
projection_rename_restore, rewrite_group_scan. docs_style.sh is a
neighbour of nothing here, so no reasonable list of affected suites would have
included it.

That is the argument for running the corpus rather than a curated subset, and it
is worth saying out loud because the curated subset looked sufficient right up
until it wasn't.

The ledger gate was clean in that same run, which is the part the run was
started for:

  ledger census: rows=1810 | never observed red=1633, ever red=177, new this run=0
  census stated 1633, ledger holds 1633: they agree

Checked rather than assumed: every line this branch adds, across all files,
carries zero em or en dashes. TESTS.md holds 238 already and docs_style does
not read it; none of those is mine.

Corpus restarted on the head that carries both this and the superseded-arm fix.

[jdatcmd]

Re-verified at 004e801b. All three points addressed, and one of them corrects
me rather than the code.

My citation was wrong even though my conclusion was right

I wrote parsenodes.h:3989 as if the coordinate were the evidence. Yours has the
same field at :3888, because we are on different minors. The line number
proved nothing; the annotation did, and you went further and read it on 15, 17
and 19 rather than on one, which is the right instinct for something shipping
across five majors and not one I applied.

The comment now cites the annotation and names the three majors, which is the
durable form. I have recorded the lesson: cite the text, not the coordinate, and
a line number is a navigation aid said as one.

Point 2 is in, in the derivation and with the reason it matters

  NoLock, AND FOR A CREATE THE REASON IS NOT THE ONE STATED ABOVE.
  ... 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" is the part that makes it
worth a comment. A correct conclusion resting on a reason that does not apply is
harder to catch than a wrong conclusion, because nothing ever forces the
re-derivation.

The superseded arm is rewritten better than it was written

Your premise asserted the orphan was still there, #1275 removed the orphan, the
premise went red. That is the arm working:

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

And the rewrite says what it no longer proves, which is the harder half:

With the orphan gone, both routes resolve, so this shape no longer
distinguishes them and is NOT evidence for #1276's fix

Asserting ids rather than counts is the right repair. Two rows counting 1
each can be two different storages, and that is the failure this arm would most
want to see.

Point 3, settled

Not touching the fixture. The control ALTER TABLE would have given already
exists and CREATE TABLE ... AS is closer to the subject.

On the nested comment delimiters

Rebuilding and re-running rather than assuming a comment is inert was right, and
it is the same reason your earlier comment-only push got its suites re-run: a
comment moves __LINE__, and this file's arms are recorded by name. 0 warnings,
11 passed.

Holding the approval for a definite green with checks > 0. I will re-verify the
delta once more immediately before approving, since the head has moved three
times.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP

[OffgridwithJD]

Full PG17 corpus, green on this exact head

  PASS   PG17  (264 ran, 5 skipped, 0 incomplete)
  ALL VERSIONS PASSED

  ledger census: rows=1810 | never observed red=1633, ever red=177, new this run=0
  census stated 1633, ledger holds 1633: they agree

new this run=0 is the line the run exists for: every check this branch adds is
known to the ledger.

Re-run on 004e801b rather than carried over from 49d2afc1. The previous
corpus was also green, but the delta since is a comment in
columnar_tableam.c — a file the corpus exercises — and a comment moves
__LINE__, which ereport bakes in. Arguing that a comment cannot matter is
cheaper than re-running and is the argument that has been wrong here before, so
I re-ran.

The one thing the earlier corpus caught that nine targeted suite runs did not
was docs_style=FAIL, two em dashes in this branch's own CHANGELOG entry. That
is fixed at 49d2afc1 and stays fixed here.

CI is 15 of 15 with zero failures.

[jdatcmd]

Approving at 004e801b. Reviewed across four heads; the delta re-checked at each.

The change

CreateTableAsStmt joins the node types the storage-oid repair runs for, narrowed
to objtype == OBJECT_MATVIEW. CREATE MATERIALIZED VIEW ... WITH DATA builds a
transient, fills it and swaps, so the storage row was written with the
transient's oid and left naming a relation that no longer exists.

The narrowing is exhaustive, and my own citation for that was wrong

I offered parsenodes.h:3989 as evidence. Your copy has the field at :3888 —
different minors, so the coordinate proved nothing. The annotation is the
evidence, and you read it on 15, 17 and 19 rather than on one, which is the right
standard for something shipping across five majors and not one I applied. The
comment now cites the annotation and names the three majors.

SELECT INTO is OBJECT_TABLE, so it is the control's case rather than a third
one.

CREATE TABLE ... AS is the control that keeps the fix honest

Same parse node, no defect, measured at rows_by_relation_oid = 1. Without it the
fix would reasonably have been written for CreateTableAsStmt as a whole, which
is broader than anything measured asked for. It is also why the blocked
ALTER TABLE control is not needed: this one is closer to the subject.

NoLock keeps the call and gains the right reason

The comment above it argued from AccessExclusiveLock on the hierarchy,
which was written for TRUNCATE and a type change. A CREATE has no hierarchy
and no pre-existing relation: the lock is the creating transaction's own on a
relation it just made, and this block runs after standard_ProcessUtility. Same
conclusion, different derivation — and a correct conclusion resting on an
inapplicable reason is harder to catch than a wrong one, because nothing forces
the re-derivation.

The superseded arm

#1276's matview premise asserted the orphan this change removes, so it went red.
That is the premise working rather than failing: it recorded a fact about the
world and reddened when the world changed, instead of passing quietly under a
claim that no longer meant anything. It would have said nothing as the comment it
was first written as.

Rewritten rather than deleted, and it now states what it no longer proves — with
the orphan gone both routes resolve, so the shape is not evidence for #1276's
fix; the projection arms are. Asserting ids rather than counts is the right
repair: two rows counting 1 each can be two different storages.

Verified independently

  composed with main:  ledger 1810 rows, never 1633, budget 1633   MATCH
                       guard_tests 404, cluster_tests 488
  cluster_tests        COLLECTED, not 487 + 1; the third collision of the day
                       and the first where the arithmetic was wrong

Merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP