fix: re-record projections after a rewrite (#876, #887) - #892
Conversation
linuxhikerpm
left a comment
linuxhikerpm
left a comment
There was a problem hiding this comment.
Blocker — TRUNCATE … CASCADE misses implicitly truncated tables.
The post-utility repair at src/columnar_tableam.c:2516-2549 only visits relations explicitly listed in TruncateStmt->relations, plus inheritance descendants from find_all_inheritors(). A table reached through a foreign-key CASCADE is neither, so its rewrite is not repaired.
Reproduced on the exact PR head 2ceb0ad030f936369eb21aaa30d6f2f0d7c65c18, PostgreSQL 18.6:
CREATE TABLE cas_parent(id int PRIMARY KEY);
CREATE TABLE cas_child(
id int REFERENCES cas_parent(id),
v int
) USING pgcolumnar;
SELECT pgcolumnar.add_projection(
'cas_child', 'pv', ARRAY['id','v'], ARRAY['v']);
INSERT INTO cas_parent SELECT g FROM generate_series(1,100) g;
INSERT INTO cas_child SELECT g, g%7 FROM generate_series(1,100) g;
TRUNCATE cas_parent CASCADE;
SELECT count(*) FROM pgcolumnar.read_projection('cas_child','pv');Observed:
NOTICE: truncate cascades to table "cas_child"
-- cas_child storage id changed, but current-storage named projection rows = 0
ERROR: 42704: projection "pv" does not exist on "cas_child"
The new 53-check projection_rewrite suite passes on this head, but its multi-table arm names both tables explicitly and therefore does not cover this case. Please collect every relation actually truncated (including FK-cascade additions) and add this as a regression arm.
|
Confirmed, fixed, and thank you — this is a real gap and your reproduction was exact. I reproduced it The defect, reproduced on
|
…very one it names @linuxhikerpm found this in review of commandprompt#892 and the reproduction is exact: TRUNCATE ... CASCADE reaches a table through a foreign key, and that table is neither listed in TruncateStmt->relations nor an inheritance descendant of anything listed. The post-statement repair walked the statement, so it never visited the cascaded table and the projection stayed absent. Reproduced on the previous head 2ceb0ad, PG 18.4: a heap parent, a columnar child referencing it with a declared projection, TRUNCATE parent CASCADE, and then read_projection raises 42704 on the child. The fix stops re-deriving the statement's reach. The table-AM callback already fires on every relation whose storage is actually replaced, so it now records those relids and the post-statement block drains the list. Instrumented on the failing case, the callback reported `rewrite branch taken relid=16573 relname=cas_child` while the post-statement block reported `targets=1` -- it had the right answer all along and the repair was asking the wrong source. Recording rather than re-deriving also avoids duplicating core's foreign-key discovery, which would have been a second copy of logic that changes between majors. Three details worth stating, because each is a way this could have gone wrong: A transient relation is never recorded. make_new_heap's relation has no columnar fork when the callback fires, so it does not take the rewrite branch. That matters because it is dropped before the list is drained, and repairing a dropped relation would raise inside an unrelated statement. The list is cleared when a utility statement starts, drained and cleared when one finishes, and cleared at transaction end through RegisterXactCallback. Without the first and last of those, a statement that ERRORED between recording and draining would leave a relid for the next statement to act on. A rewriting ALTER still needs the statement's own name, because nothing is recorded for it. Both sources are unioned, deduplicated by relid. test/projection_rewrite.sh gains the arm, and it is proved both ways in one tree so the two fingerprints differ by source alone: without the fix 57 passed, 1 failed with the fix 58 passed, 0 failed The arm asserts its premise first, that the CASCADE really did rewrite the child, and reports UNMET_PRECONDITION rather than passing if it did not. The parent is a heap table with no projections, so nothing but the child being repaired can make it pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed against head 41242e3d10a1. Your rebase and depth fix are not on GitHub — the remote
still has two commits, grep -c "depth\|nesting" src/columnar_tableam.c returns 0, and
pgcolumnar_forget_rewritten() is still at :2550 and :2639. So this review is of the tree
GitHub has, and the lifecycle blocker below is the one you have already fixed locally. Everything
else is new.
Five dimensions, each finding attacked by a skeptic told to default to refuting. 20 verdicts, 19
confirmed, 1 refuted. Most confirmations were reproduced on a live server rather than argued.
Blockers
1. The repair takes ShareLock unconditionally, on every ALTER TABLE.
src/columnar_projection.c:405. I read this one myself rather than relying on the review.
rel = table_open(relid, ShareLock);
...
if (present)
continue; /* <- the skip is AFTER the lock */The comment justifies it as "the statement that rewrote this relation already holds
AccessExclusiveLock, so this takes nothing new". That is true for a rewriting statement and false
for the ones that reach this code without rewriting. pgcolumnar_process_utility calls the repair
for every AlterTableStmt on a relation with a declared projection, including subcommands that
take only ShareUpdateExclusiveLock — SET (fillfactor), VALIDATE CONSTRAINT,
ALTER COLUMN SET STATISTICS. For those, ShareLock is new and blocks concurrent writers. And it
is taken before the already-present check, so even the no-op path pays it.
This is the rule the project already states: justify an exclusive-level lock against a weaker
correct one. Take the lock after deciding there is work, or take a weaker one for the decision.
2. The repair is not gated on "did this statement rewrite this relation".
src/columnar_projection.c:384-437. It fires whenever the projection row is absent, so a
metadata-only ALTER TABLE performs an unbounded projection back-fill — reading every live row —
on a statement that rewrote nothing. Pairs with (1): the wrong lock and the wrong work.
3. The re-record can raise inside the drain and abort a statement that succeeded on main.
src/columnar_projection.c:436-449. The declaration_resolves guard covers exactly one failure —
a name the table no longer has. materialize_projection can raise for other reasons, and anything
it raises propagates into a user statement that had already succeeded. That is the same shape you
found and fixed for the rename case; the guard is narrower than the hazard.
4. test/projection_rewrite.sh:316-325 passes with rebuild_projections gutted. The arm that
appears to cover the recovery path cannot fail. Demonstrated by mutation.
5. On current main the suite reports INCOMPLETE, which fails every major.
test/projection_rewrite.sh:269-281. Merge-blocking independently of everything else, and it is a
consequence of main moving — worth re-checking after you rebase onto e42c80d.
6. The lifecycle one you already have. Confirmed independently, with a causation mutation
rather than only a reproduction: a verifier built its own lane, reproduced 42704 on
TRUNCATE ... CASCADE with three different nested-utility trigger bodies, then added a depth
counter itself and watched all three arms go green while the controls stayed green. Your fix
direction is right.
Majors worth folding into the same push
- Two rewrite shapes sit outside the gate.
REFRESH MATERIALIZED VIEWis a rewrite that is
neitherAlterTableStmtnorTruncateStmt, so the projection is lost and the newerrhint
tells the user the opposite. And a replicatedTRUNCATErewrites the subscriber's table
without passing throughProcessUtilityat all, so nothing ever drains. - The recording is ungated while the drain is gated, so the list's stated invariant does not
hold: relids are recorded that nothing will ever drain. - A repaired projection followed by a same-transaction
TRUNCATEleaves the projection wrong.
Raised as a regression; the verifier downgraded it to an incomplete fix rather than a regression,
because the declaration survives andrebuild_projections()recovers it. Still worth an arm. docs/limitations.md:513-520documents a limitationmainhas already fixed, and
docs/sql-reference.md:473-484gives the wrong reason for the one case that still needs
rebuild_projections(). TheCHANGELOGWARNINGparagraph rests on the same stale premise.- The
HINTatsrc/columnar_projection.c:441-446tells the reader to perform an operation the
extension does not offer, and emits an unqualified, wrongly quoted relation name. - Test arms: the
TruncateStmtrelation-list walk can be deleted with the suite still green;
the already-present guard has no arm; the warn-and-skip arm asserts message text where a
SQLSTATE is available; the no-warning negative control passes on no output at all and never
asserts its premise.
Two process notes
Your 12/12 green is green on the buggy tree. All twelve checks pass on 41242e3d10a1 —
including both suites legs — and that is the head whose defect you reproduced. Third distinct way a
green tick has misled us today, after held-not-run on the fork PRs and a stale local tag ref.
Rebase onto e42c80d, not 3a4b985. #891 landed and touches pgcolumnar_process_utility too,
so you are now composing three changes in that one function: #888's rename call inside the
inheritor walk, #891's refusal before the statement, and your record/drain around both. Read the
resolved function end to end rather than trusting a clean git rebase.
Provenance
I verified (1) and the record-site asymmetry against the source myself. The rest come from the
review, most of them reproduced on live servers in their own lanes and prefixes; I have not
personally re-run each one and am not claiming to have. Where a verifier downgraded a finding I
have said so rather than keeping the reviewer's severity.
Requesting changes rather than approving — not because the approach is wrong. The design is right,
the ProcessUtility drain is the correct seam, and the four-shape sweep is better than what #887
asked for. It is the lock, the gating and the arms that need another pass.
|
Correcting two things in my review above. One claim was too strong, and one suggested fix would 1. "Blocks concurrent writers" is wrong. The waste is real; the blocking is not.I wrote that the unconditional The repair closes with an explicit lock mode rather than What survives, narrowed to what is true: every Note also how the measurement was settled: sampling 2. My suggested fix would have disabled half the feature, and my own evidence said soI proposed gating the repair on membership in the rewritten-relid list. That would have stopped I had already proved why, and did not apply it. The Nothing is ever recorded for the user's relation on that path, so nothing would ever be in the list The approach taken instead is better than what I suggested: ask the cheap question — is any 3. And my diagnosis of the missing push was wrongI suggested the rebase push had failed, and offered a mechanism: a That is three corrections against one review. The findings themselves stand — the waste, the |
…dprompt#887) A rewrite mints a new base storage id and pgcolumnar.projection is keyed by that id, so after TRUNCATE or a rewriting ALTER TABLE, read_projection raised 42704 for a projection that was still declared over an intact table. 1.0-alpha3 shipped only a HINT naming pgcolumnar.rebuild_projections(). The repair runs after the statement in pgcolumnar_process_utility, where the rewrite has committed, the new storage id is readable, and a statement that errored has left nothing to repair. Not in pgcolumnar_relation_set_new_filelocator, which commandprompt#887 proposed. Measured with that callback logging its own relid: TRUNCATE reaches it as the user's relation with both projection rows in scope, but a rewriting ALTER TABLE reaches it as the transient relation make_new_heap builds -- pg_temp_<oid>, no columnar fork -- so the branch is not taken and neither the old storage id nor the projection list is ever in scope. A re-record there also records under the retired id, because PgColumnarStorageId(rel) still returns the old id after the new metapage is written. Four shapes lose the projection, not the two commandprompt#887 names: TRUNCATE including its multi-table form, a type change on a covered or uncovered column, ADD COLUMN with a volatile default, and a partitioned child rewritten via its parent -- where the statement names the parent, which is not itself a columnar relation. Hence find_all_inheritors. Core VACUUM FULL and CLUSTER are refused on a columnar table, which bounds the class. materialize_projection is extracted from pgcolumnar_add_projection rather than copied, so declaring and re-recording drive one implementation. The projections are re-derived from the declaration, not copied forward: the base projection records every live column, so copying the old row would leave projection 0 naming a stale column set after ADD COLUMN. A repair attached to another statement must not fail it. ALTER TABLE ... RENAME COLUMN does not carry a rename into the declaration (commandprompt#888), and before this was handled the repair raised `column "a" does not exist` inside an unrelated ALTER COLUMN ... TYPE and rolled that type change back. It now warns and leaves the projection to rebuild_projections(). test/projection_rewrite.sh is new and was written before the fix: 38 passed / 9 failed on main 9628414, 53 passed / 0 failed here, both arms through the same build directory. The 42704 hint no longer blames a rewrite; it names the two cases that remain, one of which is the implicit base projection, which is not readable by name at all. No SQL and no catalog change, so no upgrade script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…very one it names @linuxhikerpm found this in review of commandprompt#892 and the reproduction is exact: TRUNCATE ... CASCADE reaches a table through a foreign key, and that table is neither listed in TruncateStmt->relations nor an inheritance descendant of anything listed. The post-statement repair walked the statement, so it never visited the cascaded table and the projection stayed absent. Reproduced on the previous head 2ceb0ad, PG 18.4: a heap parent, a columnar child referencing it with a declared projection, TRUNCATE parent CASCADE, and then read_projection raises 42704 on the child. The fix stops re-deriving the statement's reach. The table-AM callback already fires on every relation whose storage is actually replaced, so it now records those relids and the post-statement block drains the list. Instrumented on the failing case, the callback reported `rewrite branch taken relid=16573 relname=cas_child` while the post-statement block reported `targets=1` -- it had the right answer all along and the repair was asking the wrong source. Recording rather than re-deriving also avoids duplicating core's foreign-key discovery, which would have been a second copy of logic that changes between majors. Three details worth stating, because each is a way this could have gone wrong: A transient relation is never recorded. make_new_heap's relation has no columnar fork when the callback fires, so it does not take the rewrite branch. That matters because it is dropped before the list is drained, and repairing a dropped relation would raise inside an unrelated statement. The list is cleared when a utility statement starts, drained and cleared when one finishes, and cleared at transaction end through RegisterXactCallback. Without the first and last of those, a statement that ERRORED between recording and draining would leave a relid for the next statement to act on. A rewriting ALTER still needs the statement's own name, because nothing is recorded for it. Both sources are unioned, deduplicated by relid. test/projection_rewrite.sh gains the arm, and it is proved both ways in one tree so the two fingerprints differ by source alone: without the fix 57 passed, 1 failed with the fix 58 passed, 0 failed The arm asserts its premise first, that the CASCADE really did rewrite the child, and reports UNMET_PRECONDITION rather than passing if it did not. The parent is a heap table with no projections, so nothing but the child being repaired can make it pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…ot close Rebasing onto main, which now carries commandprompt#888, made three of this suite's arms unrunnable, and the suite said so rather than passing them. UNRUN stale: the statement survives a stale declaration: UNMET_PRECONDITION: declaration is {a2,b}, not stale; commandprompt#888 may have landed accounting: 52 passed + 0 failed + 3 unrunnable = 55 projection_rewrite.sh: INCOMPLETE exit 67 That is the intended behaviour and the reason the premise was asserted separately. Those arms test that a repair which cannot run degrades to a WARNING instead of aborting the statement that triggered it. They produced the stale declaration with ALTER TABLE ... RENAME COLUMN, which commandprompt#888 has now fixed, so the state they need can no longer be reached that way and the property they test would have gone untested while three green ticks suggested otherwise. The property is still worth testing, because the state is still reachable: any database created before commandprompt#888 carries it, and nothing guarantees a future path cannot reintroduce it. So the arms now write the stale name directly into pgcolumnar.projection_declaration, which is what such a database looks like, and the premise asserts the write took effect. Two arms are added for the property commandprompt#888 now guarantees, asserted positively here because this suite is the one that breaks if it regresses: a rename carries into the declaration, and the repair after a later rewrite still resolves. 60 checks, 0 failed, 0 unrunnable on the rebased tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
@jdatcmd found this reviewing commandprompt#892 and pointed at the right line: the list is process-global and cleared at the START of every pgcolumnar_process_utility, in a function that is re-entrant. ProcessUtility nests. An AFTER TRUNCATE trigger whose function runs any utility statement calls the hook again from inside the outer statement, after the callback has recorded the truncated relation. Measured with the callback logging its own order, before this change: entry: clearing, had 0 the outer TRUNCATE starts record: relid=16573 the cascaded child is recorded entry: clearing, had 1 the trigger's nested utility clears it drain: recorded=0 the outer drain has nothing left A DIRECTLY NAMED table survives that, because the statement's own relation list is a second source. That is why the defect needed composing to see: a table reached by FK CASCADE has the recorded list as its only route, so cascade plus nested utility is what leaves the projection absent and read_projection raising 42704. Either alone passes, which is exactly why the existing cascade arm did not catch it. Two changes, because there were two instances of one mistake: The list is cleared only when the OUTERMOST utility statement begins, tracked by a depth counter. A nested statement must not discard what its caller is holding. And a drain now removes the relids it repaired rather than emptying the list. The blanket clear after draining was the same defect facing the other way: an inner statement would have discarded a relid an outer one recorded and had not yet drained. The depth is decremented through PG_CATCH so an ERROR cannot leave it raised, and the transaction-end callback resets both the list and the depth. test/projection_rewrite.sh gains the composed arm, proved both ways in one tree: without the depth fix 65 passed, 1 failed with it 66 passed, 0 failed The arm asserts three premises before its verdict: the trigger is installed on the cascaded child, the projection read before the truncate, and the nested utility really ran. That last one uses a PERMANENT marker table, because a TEMP one vanishes with the trigger's session and an earlier version of this probe read 0 and reported a premise failure I first mistook for the mechanism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Three defects from @jdatcmd's commandprompt#892 review, all in the repair itself. The ShareLock was unconditional. The repair is reached for every AlterTableStmt on a relation with a declared projection, not only one that rewrote it, and the lock was taken before the already-present check. Measured: SET (fillfactor), ALTER COLUMN SET STATISTICS and SET (autovacuum_enabled) each opened the relation with ShareLock with nothing to repair. ShareLock conflicts with RowExclusiveLock, so a metadata-only statement blocked concurrent writers. The question is now asked under AccessShareLock, which conflicts with nothing a writer takes, and ShareLock is taken only once a projection is known missing. materialize_projection could abort a statement that succeeds on main. The declaration_resolves guard covers exactly one failure, a name the table no longer has, and the hazard is wider. The call now runs in an internal subtransaction: a failure is rolled back, reported as a WARNING carrying the original SQLSTATE and message, and the user statement continues. REFRESH MATERIALIZED VIEW is a rewrite that is neither AlterTableStmt nor TruncateStmt, so the projection was lost while the new errhint told the user the opposite. It now reaches the drain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
@jdatcmd demonstrated by mutation that this arm passed with rebuild_projections gutted. It truncated and then called the function, but the repair in this PR already restores a TRUNCATE, so the comparison held whether or not the function did anything. A test named after a function it does not exercise is worse than no test, because the name is what a reader trusts. The arm now deletes the projection rows directly, which is the state a logical restore leaves: pg_dump carries projection_declaration and cannot carry the storage. That is the case the function exists for. Three premise checks assert the projection read before removal, that it is genuinely absent after, and that the declaration survived. The RETURN VALUE is asserted, not only the end state, and a second call must report rebuilding nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Four more from @jdatcmd's commandprompt#892 review, plus one I found reading the composed ProcessUtility hook end to end as the review asked. The RefreshMatViewStmt I added last commit was cast to AlterTableStmt to read its relation. Both structs happen to place that field at offset 8 -- measured on PG 15 through 19, because NodeTag is 4 bytes and RefreshMatViewStmt's two bools fit in its tail padding -- so it read the right field. By coincidence of layout, not by any rule: one added field in either struct turns it into a wrong pointer with no diagnostic. It now dispatches on the node type. The TruncateStmt relation-list walk is deleted. The review said the suite stays green without it, and that is right, so I instrumented it instead of arguing: across the 80 checks it fired twice, both times for a NON-columnar parent in a cascade arm, where the repair is a no-op. TRUNCATE reaches the table-AM callback for every relation it rewrites, including the ones it never names, so the recorded list already holds them. The stale-declaration HINT named an operation this extension does not offer. Measured: rebuild_projections() re-runs the same stale declaration and raises the same missing-column error, and drop_projection() refuses with 42704 because the projection row is exactly what is absent. add_projection() with the same name replaces the declaration and materialises it -- it restored all 200 rows. The HINT now says that. Both messages carry a schema-qualified relation name. Unqualified, the HINT for a table in schema "s" said rebuild_projections('t'), and running exactly that gave ERROR: relation "t" does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
84 checks, from 76. REFRESH MATERIALIZED VIEW has an arm. Proved load-bearing by removal: with RefreshMatViewStmt out of the gate it reports ERROR: projection "pp" does not exist on "mv1", and the new HINT then tells the reader that re-recording is automatic, which is the opposite of what happened. REFRESH ... CONCURRENTLY gets no arm because it is not a rewrite -- the storage id is unchanged across it, measured -- so an arm would be permanently unrunnable rather than green. The already-present guard has an arm. It runs far more often than the repair does, because every AlterTableStmt on a relation with a declared projection reaches it, and it is why the decision is now made under AccessShareLock. Two arms could not fail, both found by @jdatcmd. The warn-and-skip arm counted lines matching the message text or the string "rebuild_projections"; rewording the HINT in this same PR would have silently halved that count. It now asserts SQLSTATE 42703, which is the contract, and the prose separately. The no-warning negative control was satisfied by no output at all, so it now asserts that the statement completed and rewrote before "no warning" is allowed to mean anything. And retired_rows() counted a live matview's rows as retired, because it filtered relkind = 'r' and a matview is 'm'. The helper is global, so that one mistake failed four later arms that had nothing to do with matviews. 84 passed, 0 failed, on PG 18 and PG 19. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
docs/limitations.md documented a limitation main has already fixed: commandprompt#888 landed, so RENAME COLUMN does carry the rename into the declaration. The reachable case is now a database created before that fix, and the page says so. docs/sql-reference.md gave the wrong reason for the one case still needing rebuild_projections(), and pointed at that function for a case it cannot fix. Both pages now name add_projection() as the recovery for an unresolvable declaration, because that is the call measured to work. Both record the rewrite this extension cannot see. A TRUNCATE replicated to a subscriber is applied by the logical replication worker calling ExecuteTruncateGuts directly rather than going through ProcessUtility (src/backend/replication/logical/worker.c, apply_handle_truncate, read against PostgreSQL 18.4). I could not complete a live publisher/subscriber demonstration on the harness cluster -- CREATE SUBSCRIPTION hung -- so this is sourced from core rather than reproduced, and the wording says which. CHANGELOG records five lost shapes rather than four, the lock and containment changes, the recorded-relid design, and the corrected check count of 84. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
The gate caught these, not me. test/docs_style.sh failed on PG 18 with two over-long sentences in docs/limitations.md and one in docs/sql-reference.md: 35, 28 and 32 words against the 25-word limit the checker enforces from ISO 24495-1. Each is split at the join that was carrying the extra clause, so no content is lost. plain_language_check.py now reports ok for every file it checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
@jdatcmd noted the recording is ungated while the drain is gated, so the list holds relids nothing will drain. That is true. The comment claimed the list held what the statement rewrote, which is narrower than what it holds. Narrowing the recording to match the drain was considered and not done. Nothing observable follows from the asymmetry, so no arm could redden if the narrowing were wrong, and a change no test can catch is worse than a stated invariant. The comment now says why it is inert: a relid recorded outside a utility statement is cleared before the next outermost one records anything, and one recorded by a nested call is drained by the enclosing statement, where the repair finds the projection present and returns under AccessShareLock without doing work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
41242e3 to
02b1ed4
Compare
|
Pushed. Rebased onto Taking your blockers in order. Blockers1. The unconditional 2. Not gated on "did this statement rewrite this relation" — fixed by (1). The unbounded back-fill is now behind the same early return: a statement that rewrote nothing does the 3. The re-record could abort a statement that succeeds on 4. The 5. 6. The lifecycle fix — thank you for the causation mutation. Building the depth counter yourself and watching three arms go green while the controls stayed green is stronger evidence than my reproduction was, and I would not have got that from re-running my own arm. Majors
Replicated The
So the The The already-present guard — has an arm now. It runs far more often than the repair does, which is the same fact that makes (1) matter. Both vacuous arms — fixed. The warn-and-skip arm counted lines matching the message text or the literal
A defect of mine that reading the composed hook caughtI added Worth recording that I predicted a segfault there and was wrong. The probe said the branch was reached with a valid pointer, and the offsets said why. The measurement corrected me; the reasoning would not have. Two of your findings I have not closedThe recording is ungated while the drain is gated. Still true. The list is cleared for the outermost statement, so it does not leak across statements, but the stated invariant is stronger than what holds and I would rather fix that than reword it. A repaired projection followed by a same-transaction On your process note
Taken, and it happened again in miniature today: this PR's own suite was 76/76 green while Verification, and exactly what it coversHead is Full matrix on That matrix ran before the rebase onto measured as One shortcut I tried and threw away, because it bears on how much the above is worth. Before re-gating I tried to prove the two commits after the first matrix were inert — one docs-only, one a comment-only change with 0 non-comment lines altered. Then I compiled both and compared: Different. Almost certainly |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approving at 02b1ed4. Every blocker from my review is closed, and I checked each against the
branch rather than against the report. 12/12, and I looked at the breakdown rather than the colour:
{"SUCCESS": 12}, nothing neutral or skipped hiding inside a zero-failure count.
What I verified myself
The lock. src/columnar_projection.c:433 opens under AccessShareLock, decides, closes at
:476, and only then takes ShareLock at :477 when there is something to materialise. The
comment now records the measurement rather than the old justification that was true of a rewriting
statement and false of the metadata-only ones. This is a better resolution than the one I proposed
— gating on the rewritten-relid list would have disabled the ALTER COLUMN TYPE repair entirely,
which my own PROBE887 run had already shown and I failed to apply.
The fail-soft is textbook and I read it line by line, because a naive PG_TRY around
BeginInternalSubTransaction corrupts transaction state and this is the riskiest new code in the
PR. It is correct: CurrentMemoryContext and CurrentResourceOwner saved before the begin;
ReleaseCurrentSubTransaction then both restored on the success path; on the error path a switch
to oldcxt before CopyErrorData, then FlushErrorState,
RollbackAndReleaseCurrentSubTransaction, both restored again, and the WARNING carrying
edata->sqlerrcode rather than a flattened message. That is plpgsql's own shape.
REFRESH MATERIALIZED VIEW is in the node set at :2780-2781 and dispatched on node type at
:2823, not through the struct-layout coincidence. Finding that the AlterTableStmt cast worked
only because NodeTag is 4 bytes and RefreshMatViewStmt's two bools fit in its tail padding —
after predicting a segfault — is the kind of thing that only measurement produces.
The TruncateStmt walk is gone, grep -c returns 0. Deleting it and instrumenting rather than
arguing was the right way to settle it, and "fired exactly twice across 80 checks, both for a
non-columnar parent where the repair is a no-op" is the answer.
Both vacuous arms are fixed, and one proved my point inside the same PR. The stale-declaration
arm now writes the stale name directly into pgcolumnar.projection_declaration instead of
depending on a rename that #888 removed as a cause, so it neither passes vacuously nor reports
UNMET_PRECONDITION. And the warn arm asserts ^WARNING: 42703: under VERBOSITY=verbose
rather than prose — the HINT was reworded in this very PR, which would have silently halved a
text-based count.
One imprecision in the scoping, which does not change the conclusion
You wrote that docs_style.sh reads CHANGELOG.md "only for its version-marker check at line
138". It reads it for two checks: the em/en dash count at :113-114 as well as the version
marker at :138. I verified the reader set the way you framed it — every .sh under test/ at
edd729e, not a working-tree grep — and docs_style.sh is indeed the only one:
suites on edd729e that read CHANGELOG.md: test/docs_style.sh
So the reduced re-gate is sound, because you re-ran docs_style entirely (9/9 on both majors)
rather than the single check you described. The scoping holds; only the sentence was narrower than
the file.
I do not think a third matrix is warranted. The rebase changed one file by +20/-3, exactly one
suite reads it, and that suite was re-run in full on both majors. Saying so explicitly rather than
implying a full re-gate is the right call and I would rather have this than an unqualified green.
The two you left open
Both correctly left open. The recording-ungated-while-the-drain-is-gated asymmetry is a stated
invariant that does not hold rather than a defect that bites, and the same-transaction TRUNCATE
case genuinely cannot get an honest arm until #896 lands — today it would pass on the rollback
rather than on the behaviour, which is the definition of an arm that stops meaning anything later.
The replicated TRUNCATE gap documented rather than closed is also right. The callback still
records, so the information exists and only the drain is missing; that is a tractable follow-up
rather than an implied hole.
Good work. This is a much better change than the one I reviewed.
|
Your correction is right, and I verified it rather than taking it — The part worth recording is that the evidence was already in my own output. The post-rebase log I read before writing that sentence begins: The first line names the check I then said did not exist. I did not read my own output carefully enough, and the sentence was narrower than the file for no better reason than that. It did not change the conclusion only because I ran the whole suite rather than the one check I had described — which is luck, not method. Had I "optimised" by running just the version-marker check, the scoping would have been wrong in a way that mattered. The right lesson is that the unit of a reduced re-gate is the suite, not the check inside it, because my model of which checks a suite contains is exactly the thing that was wrong. Also taking your point on the lock resolution. I did not know that gating on the rewritten-relid list would have disabled the Thanks for the review. It found six real defects in a change I had already gated green, four of which were instruments rather than code — and the two you left me to leave open were the right two. |
3 participants
Fixes #876. Closes #887.
A rewrite mints a new base storage id;
pgcolumnar.projectionis keyed by that id; so afterTRUNCATEor a rewritingALTER TABLE,pgcolumnar.read_projectionraised42704for aprojection that was still declared over an intact table. 1.0-alpha3 shipped only a
HINTnamingpgcolumnar.rebuild_projections(). This re-records them automatically.Three things I found that change the shape #887 proposed
#887 says "the gap is one function". It is two, and the function it names cannot be one of them.
Instrumented
pgcolumnar_relation_set_new_filelocatorto log its own relid:TRUNCATEreaches the callback as the user's own relation with the old fork attached and bothprojection rows in scope. A rewriting
ALTER TABLEreaches it as the transient relationmake_new_heapbuilds — a different oid, no columnar fork — so the rewrite branch is never takenand neither the old storage id nor the projection list is ever visible there. @jdatcmd raised this
as a hypothesis and reproduced it independently on a different container and prefix.
A re-record in that callback is also wrong in a second way:
PgColumnarStorageId(rel)stillreturns the old id after
PgColumnarWriteNewMetapage, so it records under the storage therewrite just retired. Measured, guarded to real rewrites, on top of this branch:
Unguarded it does not even survive
CREATE TABLE(cache lookup failed for relation N). I hadargued on the issue that it would raise a
projection_pkeyviolation instead; that was a claim Iread off a
CREATE UNIQUE INDEXand never ran, and running it refuted it. Corrected there.The class is four shapes, not two. Swept rather than assumed, on 18.4:
TRUNCATE, including its multi-table formADD COLUMNwith a constant defaultVACUUM FULLALTER COLUMN TYPE, covered or uncovered columnDROP COLUMNCLUSTERADD COLUMNwith a volatile defaultVACUUM,SET TABLESPACESET ACCESS METHODto the same method, no-op type changeThe last one is not in #887 and is why the hook walks
find_all_inheritors: the statement namesthe parent, which is not itself a columnar relation, so a fix reading only the named relation never
fires. The refused pair bounds the class.
A repair attached to someone else's statement must not fail that statement. The re-record
resolves the declaration's column NAMES, and
ALTER TABLE ... RENAME COLUMNdoes not carry arename into the declaration (@linuxhikerpm's #888). Before this was handled, the repair raised
column "a" does not existinside an unrelatedALTER TABLE ... ALTER COLUMN id TYPE bigintandrolled that type change back — measured, with a control. It now warns and skips:
This is independent of whether #888 lands, and #888 removes the cause rather than the symptom — I
am happy to rebase behind it.
What the change is
The repair runs after the statement in
pgcolumnar_process_utility, which is where the#778rename block already repairs catalog state for the same reason: by then the rewrite has committed,
the new storage id is readable, and a statement that errored has left nothing to repair.
materialize_projectionis extracted frompgcolumnar_add_projectionrather than copied, sodeclaring a projection and re-recording one drive a single implementation.
PgColumnarListProjectionDeclarationsis new, and sits incolumnar_metadata.hwith the otherthree declaration functions, per that header's own rule about single-consumer declarations.
Re-derived from the declaration, not copied forward. That is what makes
ADD COLUMNcorrect: thebase projection records every live column, so copying the old row would leave projection 0 naming
a stale column set. The arm wants
{1,2,3,4}and gets{}without this.No SQL, no catalog change, so no upgrade script.
pgcolumnar--1.0-alpha3.sqlis untouched.Evidence
test/projection_rewrite.shis new and was written before the fix. Same suite, same fixtures, botharms through the same build directory so a fingerprint difference is a source difference:
.somain962841413c939bfe90bfbc691659ac4Every arm compares a
pgc_set_hashofread_projectionagainst the base table rather thanchecking that the call did not raise, so a projection re-recorded EMPTY fails — which matters
because the correct end state after a bare
TRUNCATEis an empty projection that answers. Everyarm also asserts what its operation DID and reports
UNMET_PRECONDITIONrather than a pass when itdid not: an operation that failed or no-opped leaves the storage id unchanged and
read_projectionanswering, which is indistinguishable from a path that handles projections correctly. Three arms
carry
vacuum,vacuum_sortedandcluster, which re-record for themselves, so a future fixmoved into the table-AM callback reddens here instead of double-recording.
One defect was in my own suite. P2 asked "does this storage id have a
pgcolumnar.storagerow",and a storage row is written on the first WRITE, not when the storage is created — so right
after a
TRUNCATEa correctly re-recorded projection read as retired. It had been passing onmainonly because with no projection rows at all, an oracle that cannot tell "no rows" from "correct
rows" reads 0 either way. It now asks whether a row's storage id is the current id of some live
columnar relation, and says in a comment that it is a guard rather than the detector for #876.
Gate
Preflight across every packaged major, warnings are failures here as they are in CI:
Full suite matrix,
test/run_all_versions.shon 18 and 19, against this exact commit:The two skips are
native_repackandpg19_vacuum_options, both PG19-only, and both PASS on the 19arm.
projection_rewriteanddocs_stylePASS on both arms, and no suite reports FAIL anywhere inthe run. The gate log records the HEAD it ran against and it is this commit, because a green belongs
to a SHA rather than to a branch name.
The derived suite list was taken from the tree rather than guessed: 37 registered suites name the
identifiers this change touches (
add_projection,read_projection,projection_declaration,rebuild_projections,drop_projection,reconstruct_via_projection,RENAME COLUMN,ALTER COLUMN,TRUNCATE,SET ACCESS METHOD). All 37 are in the matrix above. Nothing in the treedepends on the old hint wording.
The first gate run was red and it was mine.
docs_stylefailed on both arms: two sentences Iadded ran 37 and 41 words against that gate's plain-language limit, and one used a prose
double-hyphen. Both passages are rewritten, and the run above is the re-run after that fix rather
than a re-interpretation of the first one.
Docs
CHANGELOG.mdunder[Unreleased],docs/sql-reference.md(rebuild_projectionsis no longerthe routine step after a rewrite, and what still needs it), and
docs/limitations.md(the renamegap is now the surviving limitation). The
42704hint no longer names a rewrite as the likelycause; it names the two cases that remain, one of which is the implicit base projection — which is
not readable by name at all, and which the old hint blamed on a rewrite that never happened.
RELEASE_NOTES_1.0-alpha3.mdstill says #876 "can affect you today". That describes a releasedversion, so I have left it alone deliberately — @jdatcmd, that wording is yours to decide.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a