From dce2dd0aae6ed3acc6b923f653844ea627abe566 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Tue, 8 Sep 2026 22:31:36 +0000 Subject: [PATCH 01/11] fix: re-record projections after a rewrite (#876, #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 #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_, 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 #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 (#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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 75 +++++++++ docs/limitations.md | 8 + docs/sql-reference.md | 12 ++ src/columnar.h | 14 ++ src/columnar_metadata.c | 56 +++++++ src/columnar_metadata.h | 2 + src/columnar_projection.c | 325 +++++++++++++++++++++++++++++-------- src/columnar_tableam.c | 65 ++++++++ test/projection_rewrite.sh | 293 +++++++++++++++++++++++++++++++++ test/run_all_versions.sh | 1 + 10 files changed, 780 insertions(+), 71 deletions(-) create mode 100755 test/projection_rewrite.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index b467fe31..6b4920f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,81 @@ true until the next version shipped. appear in the stored columns. `DROP COLUMN IF EXISTS` of a column that is not there is unaffected. +- A rewrite no longer loses a declared projection (#876, #887). + + `TRUNCATE`, `ALTER TABLE ... ALTER COLUMN ... TYPE` and `ALTER TABLE ... ADD + COLUMN` with a volatile default each mint a new base storage id. + `pgcolumnar.projection` is keyed by that id, so afterwards + `pgcolumnar.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 projections are now re-recorded + automatically and the manual call is no longer part of the routine path. + + **Four shapes lost the projection, not the two the issue named.** Swept on + 18.4 rather than reasoned about: `TRUNCATE` including its multi-table form, a + type change on a covered or an uncovered column, `ADD COLUMN` with a volatile + default, and **a partitioned child rewritten by a type change on its parent** -- + where the statement names the parent, which is not itself a columnar relation. + `ADD COLUMN` with a constant default, `DROP COLUMN`, `VACUUM`, `SET ACCESS + METHOD` to the same method, `SET TABLESPACE` and a no-op type change do not + rewrite and were never affected. Core `VACUUM FULL` and `CLUSTER` are refused + on a columnar table, which bounds the class. + + **The repair runs after the statement, in `ProcessUtility`, not in the table-AM + callback.** `pgcolumnar_relation_set_new_filelocator` cannot do this job, which + is measurable rather than arguable: with the callback logging its own relid, + `TRUNCATE` reaches it as the user's relation with the old fork attached and both + projection rows in scope, but a rewriting `ALTER TABLE` reaches it as the + transient relation `make_new_heap` builds -- `pg_temp_`, no columnar fork -- + so the rewrite branch is not taken and neither the old storage id nor the + projection list is ever in scope. A re-record placed there also records under the + id the rewrite just retired, because `PgColumnarStorageId(rel)` still returns the + old id after the new metapage is written. + + **The projections are re-derived from the declaration, not copied forward.** + `pgcolumnar.projection_declaration` records column NAMES and survives a rewrite, + and resolving those names against the relation as it is now is what makes `ADD + COLUMN` correct: the base projection records every live column, so a copy of the + old row would leave it naming a stale column set. `materialize_projection` is + extracted from `pgcolumnar.add_projection` so both paths drive one + implementation. + + **A repair that cannot run degrades to a WARNING and never fails the statement + that triggered it.** `ALTER TABLE ... RENAME COLUMN` does not carry the rename + into the declaration, so a declaration can name a column the table no longer + has. Before this was handled, the repair raised `column "a" does not exist` + inside an unrelated `ALTER TABLE ... ALTER COLUMN id TYPE bigint` and rolled that + type change back -- turning a silently lost projection into a blocked schema + change. It now reports + + WARNING: could not restore projection "p" on "t" after rewrite + DETAIL: Its declaration names a column the table no longer has. + HINT: Correct the declaration, then call + pgcolumnar.rebuild_projections('t'). + + and leaves the projection to that function. + +- The `42704` hint no longer names a rewrite as the likely cause, since a rewrite + now re-records. It names the two cases that remain: a declaration that no longer + resolves, and the implicit base projection, which is not readable by name at all. + +### Added + +- `test/projection_rewrite.sh`, 53 checks. Nothing in the tree asserted that a + projection answers after a rewrite, which is why this was silent. + + Every arm compares a `pgc_set_hash` of `read_projection` against the base table + rather than checking that the call did not raise, so a projection re-recorded + EMPTY fails -- which matters because the correct end state after a bare + `TRUNCATE` is an empty projection that answers. Every arm also asserts what its + operation DID (`REWROTE`, `NOOP` or `FAILED`) and reports its properties as + `UNMET_PRECONDITION` rather than as passes when it did not: an operation that + failed or no-opped leaves the storage id unchanged and `read_projection` + answering, which is indistinguishable from a path that handles projections + correctly. Three arms carry `pgcolumnar.vacuum`, `vacuum_sorted` and `cluster`, + which already re-record for themselves, so a future fix moved into the table-AM + callback reddens here instead of double-recording. + ## [1.0-alpha3] - 2026-09-02 ### Added diff --git a/docs/limitations.md b/docs/limitations.md index 92d0f9a1..edafd722 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -510,6 +510,14 @@ or an explicit `VACUUM` marks the group. Turn the feature off with ## Projections +`ALTER TABLE ... RENAME COLUMN` does not rename the column inside a projection's +declaration. The projection itself keeps working, because its storage records +attnums rather than names. What breaks is anything that reads the declaration +back. There are two such readers. `pgcolumnar.rebuild_projections()` needs the +declaration after a logical restore. The automatic re-record needs it after a +rewrite, and reports an unusable declaration as a WARNING. To recover, rename the +column back, or drop and re-declare the projection. + A projection is an additional sorted copy. Each projection therefore adds write cost and storage cost. `pgcolumnar.vacuum` builds the projections again. diff --git a/docs/sql-reference.md b/docs/sql-reference.md index d96ad26f..4df2c785 100644 --- a/docs/sql-reference.md +++ b/docs/sql-reference.md @@ -470,6 +470,18 @@ each table in the database. Run this after a logical restore. A second run builds nothing, so it is safe to run at any time. +You no longer need it after a rewrite. `TRUNCATE`, a rewriting `ALTER TABLE`, and +the maintenance rewrites re-record their projections themselves. Two cases still +need this function. The first is a logical restore. The second is a declaration +that names a column the table no longer has, because +`ALTER TABLE ... RENAME COLUMN` does not yet carry the rename into the +declaration. A rewrite that meets such a declaration reports it as + + WARNING: could not restore projection "p" on "t" after rewrite + DETAIL: Its declaration names a column the table no longer has. + +Correct the declaration, then run this. + ```sql SELECT pgcolumnar.rebuild_projections(); ``` diff --git a/src/columnar.h b/src/columnar.h index eec6b51a..a17d5f9b 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -528,6 +528,20 @@ extern int pgcolumnar_written_stripe_row_limit(Oid relid); /* projection catalog (gap 26, format 2.2). List entries are PgColumnarProjection* * palloc'd in the current context, ordered by projection_id. */ extern List *PgColumnarListProjections(uint64 storageId); +/* + * A declared projection, as pgcolumnar.projection_declaration holds it: by + * relation and by column NAME. Distinct from PgColumnarProjection, which is the + * materialised row keyed by storage id and holding attnums (#876, #887). + */ +typedef struct PgColumnarProjectionDeclaration +{ + Oid relid; + char *name; + ArrayType *columns; + ArrayType *sortKey; +} PgColumnarProjectionDeclaration; + +extern void PgColumnarRerecordProjectionsAfterRewrite(Oid relid); extern void PgColumnarInsertProjectionRow(const PgColumnarProjection *proj); /* The dumpable declaration behind a projection, keyed by regclass and stored as * column names so a dump and restore can carry it (#266). */ diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index f4ba835b..05558e56 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -3496,6 +3496,62 @@ PgColumnarInsertProjectionRow(const PgColumnarProjection *proj) CommandCounterIncrement(); /* make the row visible to later reads */ } +/* + * PgColumnarListProjectionDeclarations + * Every declared projection for one relation, by column NAME. + * + * The declaration is keyed by relation, not by storage id, so it is the only + * record of a projection that survives a rewrite -- which is what makes it the + * source a post-rewrite re-record must read from (#876, #887). The storage-id + * keyed pgcolumnar.projection rows are gone by then. + * + * Names rather than attnums, deliberately: that is what the declaration holds, + * because a dump and restore cannot carry attnums (#266). The caller resolves + * them against the relation as it is NOW, which is the behaviour a type change + * or an added column needs. + */ +List * +PgColumnarListProjectionDeclarations(Oid relid) +{ + Relation rel = open_columnar_table("projection_declaration", + AccessShareLock); + TupleDesc tupdesc = RelationGetDescr(rel); + ScanKeyData key[1]; + SysScanDesc scan; + HeapTuple tuple; + List *result = NIL; + + ScanKeyInit(&key[0], Anum_projection_declaration_rel, BTEqualStrategyNumber, + F_OIDEQ, ObjectIdGetDatum(relid)); + + scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key); + while (HeapTupleIsValid(tuple = systable_getnext(scan))) + { + PgColumnarProjectionDeclaration *d = + palloc0(sizeof(PgColumnarProjectionDeclaration)); + bool isnull; + Datum v; + + d->relid = relid; + v = heap_getattr(tuple, Anum_projection_declaration_name, tupdesc, + &isnull); + d->name = pstrdup(NameStr(*DatumGetName(v))); + v = heap_getattr(tuple, Anum_projection_declaration_columns, tupdesc, + &isnull); + /* Copied out of the scan: the tuple is not ours past systable_getnext. */ + d->columns = isnull ? NULL : DatumGetArrayTypePCopy(v); + v = heap_getattr(tuple, Anum_projection_declaration_sort_key, tupdesc, + &isnull); + d->sortKey = isnull ? NULL : DatumGetArrayTypePCopy(v); + + result = lappend(result, d); + } + systable_endscan(scan); + table_close(rel, AccessShareLock); + + return result; +} + /* * PgColumnarRecordProjectionDeclaration * Record the intent behind a projection: which relation, which name, and diff --git a/src/columnar_metadata.h b/src/columnar_metadata.h index ccf59332..905be11c 100644 --- a/src/columnar_metadata.h +++ b/src/columnar_metadata.h @@ -90,6 +90,8 @@ extern void PgColumnarRecordProjectionDeclaration(Oid relid, const char *name, ArrayType *columns, ArrayType *sortKey); +extern List *PgColumnarListProjectionDeclarations(Oid relid); + extern void PgColumnarRenameProjectionDeclarationColumn(Oid relid, const char *oldName, const char *newName); diff --git a/src/columnar_projection.c b/src/columnar_projection.c index 67e55f3a..793ca657 100644 --- a/src/columnar_projection.c +++ b/src/columnar_projection.c @@ -143,60 +143,76 @@ record_base_projection(Relation rel, uint64 storageId, List *existing) } /* - * pgcolumnar.add_projection(rel, name, columns text[], sort_key text[]) - * Declare a projection: a named column subset sorted on sort_key. + * declaration_resolves + * Does every column name in this declaration still name a live column? + * + * resolve_columns raises on a name it cannot resolve, which is right when a user + * is declaring a projection and wrong when a rewrite is repairing one: there the + * error would propagate out of whatever statement triggered the repair. Asked + * first, and separately, so the caller can decline to materialise instead. + * + * A declaration goes stale because ALTER TABLE ... RENAME COLUMN does not carry + * the rename through projection_declaration's columns and sort_key (#888). + * Measured before this existed: the repair raised `column "a" does not exist` + * inside an unrelated ALTER TABLE ... ALTER COLUMN id TYPE bigint and the type + * change rolled back. */ -Datum -pgcolumnar_add_projection(PG_FUNCTION_ARGS) +static bool +declaration_resolves(Oid relid, ArrayType *names) { - Oid relid; - char *projname; - ArrayType *colsArr; - ArrayType *sortArr; - Relation rel; - uint64 storageId; - List *existing; - ListCell *lc; - PgColumnarProjection proj; - int nextId = 1; - int i, - j; + Datum *elems; + bool *nulls; + int count; + int i; - if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2)) - ereport(ERROR, - (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED), - errmsg("rel, name, and columns must not be NULL"))); + if (names == NULL) + return true; - relid = PG_GETARG_OID(0); - projname = text_to_cstring(PG_GETARG_TEXT_PP(1)); - colsArr = PG_GETARG_ARRAYTYPE_P(2); - sortArr = PG_ARGISNULL(3) ? NULL : PG_GETARG_ARRAYTYPE_P(3); + deconstruct_array(names, TEXTOID, -1, false, TYPALIGN_INT, + &elems, &nulls, &count); - if (!PgColumnarIsColumnarRelation(relid)) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a columnar table", - get_rel_name(relid)))); + for (i = 0; i < count; i++) + { + AttrNumber attno; - if (strlen(projname) == 0) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("projection name must not be empty"))); - if (strlen(projname) >= NAMEDATALEN) - ereport(ERROR, - (errcode(ERRCODE_NAME_TOO_LONG), - errmsg("projection name \"%s\" is too long", projname))); + if (nulls[i]) + return false; - PgColumnarRequireTableOwnerByOid(relid); + attno = get_attnum(relid, text_to_cstring(DatumGetTextPP(elems[i]))); + if (attno == InvalidAttrNumber || attno < 0) + return false; + } - /* - * ShareLock: block concurrent INSERT/UPDATE/DELETE (RowExclusiveLock) while - * we back-fill the projection from existing rows, so no concurrently written - * row is missed -- the same lock non-concurrent CREATE INDEX takes. Reads are - * unaffected. (A CONCURRENTLY variant is future work.) - */ - rel = table_open(relid, ShareLock); - storageId = PgColumnarStorageId(rel); + return true; +} + +/* + * materialize_projection + * Record one projection under the relation's current storage id and + * back-fill it from the rows the table holds now. + * + * Extracted from pgcolumnar_add_projection so that the post-rewrite re-record + * (PgColumnarRerecordProjectionsAfterRewrite) drives the SAME code rather than a + * second copy of it. Everything here is per-materialisation; what stayed behind + * in add_projection is what belongs to the DECLARING act -- the owner check and + * the declaration row itself, neither of which a re-record repeats. + * + * Takes the column lists as name arrays, the form the declaration holds, and + * resolves them against the relation as it is now. add_projection passes the + * user's arrays straight through, so its behaviour is unchanged. + */ +static void +materialize_projection(Relation rel, char *projname, ArrayType *colsArr, + ArrayType *sortArr) +{ + Oid relid = RelationGetRelid(rel); + uint64 storageId = PgColumnarStorageId(rel); + List *existing; + PgColumnarProjection proj; + ListCell *lc; + int nextId = 1; + int i, + j; existing = PgColumnarListProjections(storageId); record_base_projection(rel, storageId, existing); @@ -254,16 +270,6 @@ pgcolumnar_add_projection(PG_FUNCTION_ARGS) /* populate the projection from the table's existing rows (gap 26 back-fill) */ PgColumnarBackfillProjection(rel, &proj); - /* - * Record the declaration behind it, by relation and column name, so a dump - * and restore can carry the intent even though it cannot carry the storage - * (#266). Written here rather than in the SQL binding so that a projection - * cannot come into existence without one. - */ - PgColumnarRecordProjectionDeclaration(relid, projname, colsArr, - sortArr ? sortArr : - construct_empty_array(TEXTOID)); - /* * An open write state on this relation cached its projection-writer list on * its first row and latched it, including when the list was empty because no @@ -276,11 +282,179 @@ pgcolumnar_add_projection(PG_FUNCTION_ARGS) * sees rather than what follows it. */ PgColumnarResetProjectionWritersForRelation(relid); +} + +/* + * pgcolumnar.add_projection(rel, name, columns text[], sort_key text[]) + * Declare a projection: a named column subset sorted on sort_key. + */ +Datum +pgcolumnar_add_projection(PG_FUNCTION_ARGS) +{ + Oid relid; + char *projname; + ArrayType *colsArr; + ArrayType *sortArr; + Relation rel; + + if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2)) + ereport(ERROR, + (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED), + errmsg("rel, name, and columns must not be NULL"))); + + relid = PG_GETARG_OID(0); + projname = text_to_cstring(PG_GETARG_TEXT_PP(1)); + colsArr = PG_GETARG_ARRAYTYPE_P(2); + sortArr = PG_ARGISNULL(3) ? NULL : PG_GETARG_ARRAYTYPE_P(3); + + if (!PgColumnarIsColumnarRelation(relid)) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is not a columnar table", + get_rel_name(relid)))); + + if (strlen(projname) == 0) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("projection name must not be empty"))); + if (strlen(projname) >= NAMEDATALEN) + ereport(ERROR, + (errcode(ERRCODE_NAME_TOO_LONG), + errmsg("projection name \"%s\" is too long", projname))); + + PgColumnarRequireTableOwnerByOid(relid); + + /* + * ShareLock: block concurrent INSERT/UPDATE/DELETE (RowExclusiveLock) while + * we back-fill the projection from existing rows, so no concurrently written + * row is missed -- the same lock non-concurrent CREATE INDEX takes. Reads are + * unaffected. (A CONCURRENTLY variant is future work.) + */ + rel = table_open(relid, ShareLock); + + materialize_projection(rel, projname, colsArr, sortArr); + + /* + * Record the declaration behind it, by relation and column name, so a dump + * and restore can carry the intent even though it cannot carry the storage + * (#266). Written here rather than in the SQL binding so that a projection + * cannot come into existence without one. + */ + PgColumnarRecordProjectionDeclaration(relid, projname, colsArr, + sortArr ? sortArr : + construct_empty_array(TEXTOID)); table_close(rel, ShareLock); PG_RETURN_VOID(); } +/* + * PgColumnarRerecordProjectionsAfterRewrite + * Re-materialise this relation's declared projections under whatever + * storage id it has NOW (#876, #887). + * + * A rewrite mints a new base storage id, and pgcolumnar.projection is keyed by + * that id, so every projection row a rewrite leaves behind describes storage the + * relation no longer has. pgcolumnar_delete_storage_tree removes those rows + * (#867), which leaves read_projection raising 42704 for a projection that is + * still declared over an intact table. This restores them. + * + * Skips a declaration whose projection is already recorded under the current + * storage id, so it writes nothing for a statement that rewrote nothing, and + * nothing for the paths that re-record for themselves (pgcolumnar_compact_relation + * and its zorder sibling). test/projection_rewrite.sh carries those three as + * regression arms rather than leaving the property to this comment. + * + * Re-derived from the DECLARATION rather than copied from the old rows, for two + * reasons. The old rows are already gone on the TRUNCATE path. And a rewrite can + * change the relation's shape: ALTER TABLE ... ADD COLUMN with a volatile + * default rewrites AND adds a column, and the base projection records all live + * columns, so copying the old row forward would leave projection 0 naming a + * stale column set. Resolving names against the relation as it is now gets both + * cases right for the same reason. + * + * Not called from pgcolumnar_relation_set_new_filelocator, which is where #887 + * proposed it. That callback cannot do this job: a rewriting ALTER TABLE reaches + * it on the TRANSIENT relation make_new_heap builds, with no columnar fork and a + * different oid, so neither the old storage id nor the projection list is ever + * in scope. Measured on 18.4 with the callback logging its own relid: TRUNCATE + * arrives as the user's relation, ALTER COLUMN TYPE arrives as pg_temp_. + */ +void +PgColumnarRerecordProjectionsAfterRewrite(Oid relid) +{ + List *decls; + ListCell *lc; + Relation rel; + uint64 storageId; + List *existing; + + if (!PgColumnarIsColumnarRelation(relid)) + return; + + decls = PgColumnarListProjectionDeclarations(relid); + if (decls == NIL) + return; + + /* + * ShareLock, matching add_projection: the back-fill below reads every live + * row, so concurrent writers must be held off exactly as they are when a + * projection is first created. The statement that rewrote this relation + * already holds AccessExclusiveLock, so this takes nothing new. + */ + rel = table_open(relid, ShareLock); + storageId = PgColumnarStorageId(rel); + existing = PgColumnarListProjections(storageId); + + foreach(lc, decls) + { + PgColumnarProjectionDeclaration *d = + (PgColumnarProjectionDeclaration *) lfirst(lc); + ListCell *lc2; + bool present = false; + + foreach(lc2, existing) + { + PgColumnarProjection *p = (PgColumnarProjection *) lfirst(lc2); + + if (p->projectionId > 0 && strcmp(p->name, d->name) == 0) + { + present = true; + break; + } + } + if (present) + continue; + + /* + * A declaration naming a column this relation no longer has cannot be + * materialised, and must not take the statement that triggered this + * repair down with it. WARNING and move on: the declaration survives, + * so pgcolumnar.rebuild_projections() remains the recovery once the + * names are correct again. + */ + if (!declaration_resolves(relid, d->columns) || + !declaration_resolves(relid, d->sortKey)) + { + ereport(WARNING, + (errcode(ERRCODE_UNDEFINED_COLUMN), + errmsg("could not restore projection \"%s\" on \"%s\" after rewrite", + d->name, get_rel_name(relid)), + errdetail("Its declaration names a column the table no longer has."), + errhint("Correct the declaration, then call pgcolumnar.rebuild_projections(%s).", + quote_literal_cstr(get_rel_name(relid))))); + continue; + } + + materialize_projection(rel, d->name, d->columns, d->sortKey); + /* the new row must be visible to the next iteration's id/name check */ + CommandCounterIncrement(); + existing = PgColumnarListProjections(PgColumnarStorageId(rel)); + } + + table_close(rel, ShareLock); +} + /* * pgcolumnar.drop_projection(rel, name) * Drop a declared projection. The base projection cannot be dropped. @@ -342,11 +516,14 @@ pgcolumnar_drop_projection(PG_FUNCTION_ARGS) (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("projection \"%s\" does not exist on \"%s\"", projname, get_rel_name(relid)), - errhint("A rewrite -- TRUNCATE, vacuum, recluster -- mints a new " - "storage id while the projection rows keep the old one, " - "so a projection that is still declared can read as " - "absent (#876). pgcolumnar.rebuild_projections() " - "re-records them."))); + errhint("A declared projection is re-recorded automatically after a " + "rewrite (#887), so this is no longer the usual cause. It can " + "still read as absent when its declaration names a column the " + "table no longer has, which the rewrite reports as a WARNING, " + "or when the name given is the implicit base projection, which " + "is not readable by name. pgcolumnar.rebuild_projections() " + "re-records a declared projection once its declaration " + "resolves."))); if (targetId == 0) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), @@ -483,11 +660,14 @@ pgcolumnar_read_projection(PG_FUNCTION_ARGS) (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("projection \"%s\" does not exist on \"%s\"", projname, get_rel_name(relid)), - errhint("A rewrite -- TRUNCATE, vacuum, recluster -- mints a new " - "storage id while the projection rows keep the old one, " - "so a projection that is still declared can read as " - "absent (#876). pgcolumnar.rebuild_projections() " - "re-records them."))); + errhint("A declared projection is re-recorded automatically after a " + "rewrite (#887), so this is no longer the usual cause. It can " + "still read as absent when its declaration names a column the " + "table no longer has, which the rewrite reports as a WARNING, " + "or when the name given is the implicit base projection, which " + "is not readable by name. pgcolumnar.rebuild_projections() " + "re-records a declared projection once its declaration " + "resolves."))); ncols = proj->columnsLen; @@ -674,11 +854,14 @@ pgcolumnar_reconstruct_via_projection(PG_FUNCTION_ARGS) (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("projection \"%s\" does not exist on \"%s\"", projname, get_rel_name(relid)), - errhint("A rewrite -- TRUNCATE, vacuum, recluster -- mints a new " - "storage id while the projection rows keep the old one, " - "so a projection that is still declared can read as " - "absent (#876). pgcolumnar.rebuild_projections() " - "re-records them."))); + errhint("A declared projection is re-recorded automatically after a " + "rewrite (#887), so this is no longer the usual cause. It can " + "still read as absent when its declaration names a column the " + "table no longer has, which the rewrite reports as a WARNING, " + "or when the name given is the implicit base projection, which " + "is not readable by name. pgcolumnar.rebuild_projections() " + "re-records a declared projection once its declaration " + "resolves."))); ncols = proj->columnsLen; diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index cfc8c159..b02b7eaa 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -2624,6 +2624,71 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); + /* + * A rewrite loses this relation's declared projections, so re-record them + * (#876, #887). + * + * AFTER the statement, for the same reason the rename block below runs there: + * the rewrite has committed to the catalog by this point, the new storage id + * is readable, and a statement that ERRORED has left nothing to repair. + * + * Here rather than in pgcolumnar_relation_set_new_filelocator, which is where + * #887 proposed it. Measured on 18.4 with that callback logging its own + * relid: TRUNCATE reaches it as the user's relation with the fork attached + * and both projection rows in scope, but a rewriting ALTER TABLE reaches it + * as the TRANSIENT relation make_new_heap builds -- pg_temp_, no + * columnar fork -- so the rewrite branch is not taken and the old storage id + * and projection list are never in scope. The callback can serve one of the + * two shapes and not the other. + * + * Every relation the statement could have rewritten, and their descendants. + * A TRUNCATE names any number of relations and rewrites all of them + * (measured), and a type change on a PARTITIONED parent rewrites each + * columnar partition while the parent named in the statement is not itself a + * columnar relation (measured: the child loses its projection). Both shapes + * are arms in test/projection_rewrite.sh. find_all_inheritors for the same + * reason the rename block walks it; the statement already holds + * AccessExclusiveLock on the hierarchy, so NoLock takes nothing new. + */ + if (parsetree != NULL && + (IsA(parsetree, AlterTableStmt) || IsA(parsetree, TruncateStmt))) + { + List *targets = NIL; + ListCell *lc; + + if (IsA(parsetree, TruncateStmt)) + { + foreach(lc, ((TruncateStmt *) parsetree)->relations) + { + RangeVar *rv = (RangeVar *) lfirst(lc); + Oid relid = RangeVarGetRelid(rv, NoLock, true); + + if (OidIsValid(relid)) + targets = lappend_oid(targets, relid); + } + } + else + { + AlterTableStmt *ats = (AlterTableStmt *) parsetree; + Oid relid = ats->relation ? + RangeVarGetRelid(ats->relation, NoLock, true) : InvalidOid; + + if (OidIsValid(relid)) + targets = lappend_oid(targets, relid); + } + + foreach(lc, targets) + { + List *kin = find_all_inheritors(lfirst_oid(lc), NoLock, NULL); + ListCell *lc2; + + foreach(lc2, kin) + PgColumnarRerecordProjectionsAfterRewrite(lfirst_oid(lc2)); + list_free(kin); + } + list_free(targets); + } + /* * A column rename must be carried through the ordering mark (#778). The * mark records its sort key as column NAMES and both ordering self-gates diff --git a/test/projection_rewrite.sh b/test/projection_rewrite.sh new file mode 100755 index 00000000..a30a5ca4 --- /dev/null +++ b/test/projection_rewrite.sh @@ -0,0 +1,293 @@ +#!/usr/bin/env bash +# +# pgColumnar: a declared projection must survive a rewrite (#876, #887). +# +# #876 reports the symptom: pgcolumnar.read_projection raises 42704 after a +# rewrite, for a projection that is still declared over an intact base table. +# Three properties must hold after ANY rewrite, and before this suite nothing in +# the tree asserted the first of them: +# +# P1 read_projection answers, and holds exactly the rows the base table holds. +# P2 no pgcolumnar.projection row names a storage id the table no longer has. +# P3 the declaration survives, so a rebuild is always possible. +# +# Two traps this suite is built around. +# +# An arm that asserts only "read_projection did not raise" passes on a tree where +# the projection was re-recorded EMPTY, so every arm compares pgc_set_hash +# against the base table rather than counting rows or checking for an error. +# +# And an operation that FAILED or that no-opped leaves the storage id unchanged +# and read_projection answering -- indistinguishable from an operation that +# handled projections correctly. Three rows of #887's table were vacuous that +# way. So every arm asserts what the operation DID (REWROTE / NOOP / FAILED) +# before its outcome is allowed to mean anything. +# +# Usage: test/projection_rewrite.sh [PG_CONFIG] +# +# Written fresh for pgColumnar; it does not reuse any upstream test file. + +set -uo pipefail + +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +N=5000 + +# --------------------------------------------------------------------------- +# Premises. Every verdict below is vacuous if the fixture is not what it says, +# so these run first and are checks in their own right. +# --------------------------------------------------------------------------- +psql_run "CREATE TABLE prem (id int, a int, b text) USING pgcolumnar;" +psql_run "INSERT INTO prem SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" +psql_run "SELECT pgcolumnar.add_projection('prem','pp',ARRAY['a','b'],ARRAY['a']);" +check "PREMISE base holds its rows" "$(q 'SELECT count(*) FROM prem;')" "$N" +check "PREMISE projection answers before any rewrite" \ + "$(q "SELECT count(*) FROM pgcolumnar.read_projection('prem','pp');")" "$N" +check "PREMISE projection agrees with base before any rewrite" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('prem','pp')")" \ + "$(pgc_set_hash "SELECT a::text||'|'||b FROM prem")" +check "PREMISE two catalog rows under the current storage" \ + "$(q "SELECT count(*) FROM pgcolumnar.projection WHERE storage_id = pgcolumnar.get_storage_id('prem');")" "2" + +# P2's oracle: a projection row whose storage id is not the CURRENT storage id of +# any live columnar relation. It must read 0 at every point in this suite. +# +# NOT "has no pgcolumnar.storage row", which is what this asserted first and is +# wrong in a way that made it pass for the wrong reason. A storage row is written +# on the first WRITE, not when the storage is created, so straight after a +# TRUNCATE the table's own current storage has no row -- and a correctly +# re-recorded projection under it read as retired. The version below asks the +# question the property is about, and it is deliberately GLOBAL: a row orphaned +# under any table is a leak, whichever arm caused it. +# +# This arm is a guard, not the detector for #876. It reads 0 on unmodified main +# too, because pgcolumnar_delete_storage_tree deletes the rows rather than +# stranding them (#867). It is here to redden if a fix strands them instead -- +# which re-recording in the wrong place does. +retired_rows() { + q "SELECT count(*) FROM pgcolumnar.projection p + WHERE NOT EXISTS ( + SELECT 1 FROM pg_class c JOIN pg_am am ON am.oid = c.relam + WHERE am.amname = 'pgcolumnar' AND c.relkind = 'r' + AND pgcolumnar.get_storage_id(c.oid) = p.storage_id);" +} +check "PREMISE no retired projection rows to begin with" "$(retired_rows)" "0" + +# --------------------------------------------------------------------------- +# One arm: build a fixture, run one operation, assert all three properties. +# +# $1 table $2 operation SQL $3 what the operation must DO +# $4 the base-table projection of the covered columns, AFTER the operation +# --------------------------------------------------------------------------- +arm() { + local tag="$1" op="$2" wantdid="$3" oracle="$4" + local sid0 did err + + psql_run "CREATE TABLE $tag (id int, a int, b text) USING pgcolumnar;" >/dev/null + psql_run "INSERT INTO $tag SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" >/dev/null + psql_run "SELECT pgcolumnar.add_projection('$tag','pp',ARRAY['a','b'],ARRAY['a']);" >/dev/null + sid0="$(q "SELECT pgcolumnar.get_storage_id('$tag');")" + + if err="$(psql_run "$op" 2>&1)"; then + [ "$sid0" = "$(q "SELECT pgcolumnar.get_storage_id('$tag');")" ] && did=NOOP || did=REWROTE + else + did="FAILED" + fi + + # The gate. An operation that did not do what the arm is about proves + # nothing either way, so the properties are not reported for it. + if [ "$did" != "$wantdid" ]; then + pgc_fail "$tag: operation must $wantdid" \ + "got $did${err:+ — $(sed -nE 's/.*(ERROR:.*)$/\1/p' <<<"$err" | head -1)}" + check_unrunnable "$tag P1 projection agrees with base" \ + UNMET_PRECONDITION "operation did not $wantdid" + check_unrunnable "$tag P2 no retired projection rows" \ + UNMET_PRECONDITION "operation did not $wantdid" + check_unrunnable "$tag P3 declaration survives" \ + UNMET_PRECONDITION "operation did not $wantdid" + return + fi + pgc_pass "$tag: operation $wantdid" + + # P1 -- the projection holds exactly what the base holds. Compared as a set + # hash, so a projection re-recorded EMPTY fails here rather than passing on + # "read_projection did not raise". read_projection raising is also a failure: + # pgc_set_hash of a failed query returns empty, which cannot equal the + # oracle unless the base is empty too -- and where the base IS empty the + # oracle is the EMPTY sentinel, which a raised error still does not produce. + check "$tag P1 projection agrees with base" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('$tag','pp')")" \ + "$(pgc_set_hash "$oracle")" + check "$tag P2 no retired projection rows" "$(retired_rows)" "0" + check "$tag P3 declaration survives" \ + "$(q "SELECT count(*) FROM pgcolumnar.projection_declaration + WHERE rel = '$tag'::regclass AND name = 'pp';")" "1" +} + +echo "-- the rewrites that lose the projection today (#876)" + +# TRUNCATE then re-INSERT: the projection must answer with the NEW rows. +arm t_trunc_reinsert \ + "TRUNCATE t_trunc_reinsert; + INSERT INTO t_trunc_reinsert SELECT g, g%7, 'z'||g FROM generate_series(1,100) g;" \ + REWROTE \ + "SELECT a::text||'|'||b FROM t_trunc_reinsert" + +# TRUNCATE alone. The correct end state is a projection that answers and is +# EMPTY, which is why the oracle is the base table rather than a row count: an +# arm demanding rows would be wrong here and right for every other arm. +arm t_trunc_empty "TRUNCATE t_trunc_empty;" REWROTE \ + "SELECT a::text||'|'||b FROM t_trunc_empty" + +# A type change on a COVERED column. The stored value must come back under the +# new type, so the oracle is read after the ALTER, not before. +arm t_altertype "ALTER TABLE t_altertype ALTER COLUMN a TYPE bigint;" REWROTE \ + "SELECT a::text||'|'||b FROM t_altertype" + +# A type change on a column the projection does NOT cover still rewrites the +# whole table, so it loses the projection just the same. +arm t_altertype_unc "ALTER TABLE t_altertype_unc ALTER COLUMN id TYPE bigint;" REWROTE \ + "SELECT a::text||'|'||b FROM t_altertype_unc" + +# ADD COLUMN with a volatile default rewrites AND changes the live column set. +# The base projection (projection_id 0) records all live columns, so a fix that +# copies the old row verbatim leaves it naming a stale column set -- which no +# arm above would catch, because pp does not cover the new column. +arm t_addcol_vol \ + "ALTER TABLE t_addcol_vol ADD COLUMN zz double precision DEFAULT random();" \ + REWROTE "SELECT a::text||'|'||b FROM t_addcol_vol" + +# TRUNCATE naming SEVERAL tables rewrites every one of them, so a fix that +# repairs only the first relation in the statement passes every arm above. +psql_run "CREATE TABLE t_trunc_two (id int, a int, b text) USING pgcolumnar;" >/dev/null +psql_run "INSERT INTO t_trunc_two SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" >/dev/null +psql_run "SELECT pgcolumnar.add_projection('t_trunc_two','pp',ARRAY['a','b'],ARRAY['a']);" >/dev/null +arm t_trunc_multi "TRUNCATE t_trunc_multi, t_trunc_two;" REWROTE \ + "SELECT a::text||'|'||b FROM t_trunc_multi" +check "t_trunc_multi: the SECOND table in the statement also survives" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('t_trunc_two','pp')")" \ + "$(pgc_set_hash "SELECT a::text||'|'||b FROM t_trunc_two")" + +# A partitioned CHILD, rewritten by a type change on the PARENT. The statement +# names pt, the rewrite lands on pt1, and pt is not itself a columnar relation -- +# so a fix that looks only at the relation named in the statement never fires +# here. This is the same reason the #778 rename block walks find_all_inheritors. +echo "-- a partitioned child rewritten through its parent" +psql_run "CREATE TABLE ptr (id int, a int, b text) PARTITION BY RANGE (id);" +psql_run "CREATE TABLE ptr1 PARTITION OF ptr FOR VALUES FROM (1) TO (100000) USING pgcolumnar;" +psql_run "INSERT INTO ptr SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" +psql_run "SELECT pgcolumnar.add_projection('ptr1','pp',ARRAY['a','b'],ARRAY['a']);" +PTR_SID0="$(q "SELECT pgcolumnar.get_storage_id('ptr1');")" +psql_run "ALTER TABLE ptr ALTER COLUMN a TYPE bigint;" +if [ "$PTR_SID0" = "$(q "SELECT pgcolumnar.get_storage_id('ptr1');")" ]; then + check_unrunnable "ptr1 P1 child projection agrees with base" \ + UNMET_PRECONDITION "the parent-level ALTER did not rewrite the child" +else + pgc_pass "ptr1: parent-level ALTER rewrote the child" + check "ptr1 P1 child projection agrees with base" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('ptr1','pp')")" \ + "$(pgc_set_hash "SELECT a::text||'|'||b FROM ptr1")" +fi + +echo "-- and the base projection must still name every live column" +check "t_addcol_vol base projection covers the added column" \ + "$(q "SELECT columns FROM pgcolumnar.projection + WHERE storage_id = pgcolumnar.get_storage_id('t_addcol_vol') + AND projection_id = 0;")" \ + "{1,2,3,4}" + +echo "-- the rewrites that already handle projections: regression arms" +# These are GREEN on main. pgcolumnar_compact_relation and its siblings re-record +# after RelationSetNewRelfilenumber, which dispatches through the same table-AM +# callback the arms above go through. So a fix placed IN that callback double- +# records for these three and violates projection_pkey (storage_id, +# projection_id). They are here to redden if that happens. +arm r_vacuum "SELECT pgcolumnar.vacuum('r_vacuum');" REWROTE \ + "SELECT a::text||'|'||b FROM r_vacuum" +arm r_vacsorted "SELECT pgcolumnar.vacuum_sorted('r_vacsorted','a');" REWROTE \ + "SELECT a::text||'|'||b FROM r_vacsorted" +arm r_cluster "SELECT pgcolumnar.cluster('r_cluster','a');" REWROTE \ + "SELECT a::text||'|'||b FROM r_cluster" + +# --------------------------------------------------------------------------- +# The re-record must never abort the statement that triggered it. +# +# A declaration can name a column the relation no longer has: ALTER TABLE ... +# RENAME COLUMN does not carry the rename through projection_declaration's +# columns/sort_key arrays (#888). The re-record resolves those NAMES against the +# relation as it is now, so on such a table it cannot materialise the projection. +# +# What it must not do is take the user's statement down with it. Measured before +# this arm existed: the re-record raised `column "a" does not exist` inside an +# unrelated ALTER TABLE ... ALTER COLUMN id TYPE bigint, exit 1, and the type +# change was rolled back -- turning a silently lost projection into a blocked +# schema change. A repair that cannot run must degrade to a WARNING and leave +# the projection for rebuild_projections, which is the documented recovery. +# +# This arm is independent of whether #888 lands: a declaration can also go stale +# through a path nobody has closed yet, and the statement must survive either way. +echo "-- a stale declaration must not abort the statement (#888 interaction)" +psql_run "CREATE TABLE stale (id int, a int, b text) USING pgcolumnar;" +psql_run "INSERT INTO stale SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" +psql_run "SELECT pgcolumnar.add_projection('stale','pp',ARRAY['a','b'],ARRAY['a']);" +psql_run "ALTER TABLE stale RENAME COLUMN a TO a2;" +STALE_DECL="$(q "SELECT columns::text FROM pgcolumnar.projection_declaration WHERE rel='stale'::regclass;")" +if [ "$STALE_DECL" = "{a,b}" ]; then + pgc_pass "PREMISE the rename left the declaration naming a dead column" +else + # #888 landing makes this premise false, and then the arm below is vacuous + # rather than passing: it can only test a statement that must survive a + # stale declaration if the declaration is actually stale. + check_unrunnable "stale: the statement survives a stale declaration" \ + UNMET_PRECONDITION "declaration is $STALE_DECL, not stale; #888 may have landed" + check_unrunnable "stale: the base table keeps its rows" \ + UNMET_PRECONDITION "declaration is $STALE_DECL, not stale; #888 may have landed" + check_unrunnable "stale: the unrelated type change took effect" \ + UNMET_PRECONDITION "declaration is $STALE_DECL, not stale; #888 may have landed" +fi +if [ "$STALE_DECL" = "{a,b}" ]; then + if psql_run "ALTER TABLE stale ALTER COLUMN id TYPE bigint;" >/dev/null 2>&1; then + pgc_pass "stale: the statement survives a stale declaration" + else + pgc_fail "stale: the statement survives a stale declaration" \ + "the re-record aborted an unrelated ALTER TABLE" + fi + check "stale: the base table keeps its rows" "$(q 'SELECT count(*) FROM stale;')" "$N" + check "stale: the unrelated type change took effect" \ + "$(q "SELECT format_type(atttypid,atttypmod) FROM pg_attribute + WHERE attrelid='stale'::regclass AND attname='id';")" "bigint" + + # A skip the user is never told about is a silent projection loss, which is + # the whole complaint in #876. Assert the WARNING from its own output, and + # assert the negative control in the same breath: a table whose declaration + # is intact must not produce one, or the arm passes on a warning that fires + # unconditionally. + psql_run "CREATE TABLE stale2 (id int, a int, b text) USING pgcolumnar;" >/dev/null + psql_run "INSERT INTO stale2 SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" >/dev/null + psql_run "SELECT pgcolumnar.add_projection('stale2','pp',ARRAY['a','b'],ARRAY['a']);" >/dev/null + psql_run "ALTER TABLE stale2 RENAME COLUMN a TO a2;" >/dev/null + check "stale: the skip warns, naming the projection and the recovery" \ + "$(psql_run "ALTER TABLE stale2 ALTER COLUMN id TYPE bigint;" 2>&1 | + grep -cE 'WARNING:.*could not restore projection "pp"|rebuild_projections')" "2" + psql_run "CREATE TABLE fresh2 (id int, a int, b text) USING pgcolumnar;" >/dev/null + psql_run "INSERT INTO fresh2 SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" >/dev/null + psql_run "SELECT pgcolumnar.add_projection('fresh2','pp',ARRAY['a','b'],ARRAY['a']);" >/dev/null + check "stale: an intact declaration produces NO warning" \ + "$(psql_run "ALTER TABLE fresh2 ALTER COLUMN id TYPE bigint;" 2>&1 | + grep -ci warning)" "0" +fi + +echo "-- rebuild_projections stays the documented manual recovery (#876)" +psql_run "CREATE TABLE rec (id int, a int, b text) USING pgcolumnar;" +psql_run "INSERT INTO rec SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" +psql_run "SELECT pgcolumnar.add_projection('rec','pp',ARRAY['a','b'],ARRAY['a']);" +psql_run "TRUNCATE rec; INSERT INTO rec SELECT g, g%9, 'y'||g FROM generate_series(1,200) g;" +psql_run "SELECT pgcolumnar.rebuild_projections('rec');" >/dev/null 2>&1 +check "rebuild_projections still repairs a lost projection" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('rec','pp')")" \ + "$(pgc_set_hash "SELECT a::text||'|'||b FROM rec")" +check "and leaves no retired projection rows" "$(retired_rows)" "0" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index b66aa79b..603e1f9c 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -243,6 +243,7 @@ SUITES=( projection_drop_column projection_privilege projection_rename_restore + projection_rewrite projection_update projections pushdown_report From 4cd9d6fbdf104d9d8213ae4c9ffdc6769a0f9bba Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Tue, 8 Sep 2026 23:49:28 +0000 Subject: [PATCH 02/11] fix: repair projections for every relation a statement rewrote, not every one it names @linuxhikerpm found this in review of #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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- src/columnar_tableam.c | 91 +++++++++++++++++++++++++++++++++++++- test/projection_rewrite.sh | 34 ++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index b02b7eaa..79f04681 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -102,6 +102,67 @@ static const struct config_enum_entry pgcolumnar_compression_options[] = { /* forward declaration of the AM routine so hooks can compare against it */ static const TableAmRoutine pgcolumnar_am_methods; +/* + * Relations this statement actually rewrote, recorded by + * pgcolumnar_relation_set_new_filelocator and drained by + * pgcolumnar_process_utility so their projections can be re-recorded (#876, #887). + * + * Recorded rather than re-derived from the statement, because the statement does + * not name everything it rewrites. TRUNCATE ... CASCADE reaches a table through a + * foreign key: it is neither listed in TruncateStmt->relations nor an inheritance + * descendant of anything listed, so a repair that walks the statement misses it + * (@linuxhikerpm, #892 review). The callback, by contrast, fires on every relation + * whose storage is actually replaced, which is the set we want by definition. + * + * Only relations that took the rewrite branch land here, so a transient relation + * built by make_new_heap never does: at its creation there is no columnar fork and + * the branch is not taken. That matters, because such a relation is dropped before + * this list is drained. + * + * TopMemoryContext, because the callback runs inside the statement's context and + * the list has to outlive it. Cleared when a utility statement starts, drained and + * cleared when one finishes, and cleared at transaction end so an ERROR between + * those two points cannot carry a relid into the next statement. + */ +static List *pgcolumnar_rewritten_relids = NIL; +static bool pgcolumnar_xact_cb_registered = false; + +static void +pgcolumnar_forget_rewritten(void) +{ + if (pgcolumnar_rewritten_relids != NIL) + { + list_free(pgcolumnar_rewritten_relids); + pgcolumnar_rewritten_relids = NIL; + } +} + +static void +pgcolumnar_rewritten_xact_callback(XactEvent event, void *arg) +{ + /* Any transaction end, committed or not: the list belongs to one statement. */ + pgcolumnar_forget_rewritten(); +} + +static void +pgcolumnar_record_rewritten(Oid relid) +{ + MemoryContext old; + + if (!pgcolumnar_xact_cb_registered) + { + RegisterXactCallback(pgcolumnar_rewritten_xact_callback, NULL); + pgcolumnar_xact_cb_registered = true; + } + + if (list_member_oid(pgcolumnar_rewritten_relids, relid)) + return; + + old = MemoryContextSwitchTo(TopMemoryContext); + pgcolumnar_rewritten_relids = lappend_oid(pgcolumnar_rewritten_relids, relid); + MemoryContextSwitchTo(old); +} + static object_access_hook_type prev_object_access_hook = NULL; static ProcessUtility_hook_type prev_process_utility_hook = NULL; static ExecutorEnd_hook_type prev_executor_end_hook = NULL; @@ -818,6 +879,12 @@ pgcolumnar_relation_set_new_filelocator(Relation rel, if (smgrexists(oldsrel, MAIN_FORKNUM) && smgrnblocks(oldsrel, MAIN_FORKNUM) >= COLUMNAR_INITIALIZED_NBLOCKS) { + /* + * Remember that this relation was rewritten, before the storage tree that + * proves it goes away. pgcolumnar_process_utility drains the list. + */ + pgcolumnar_record_rewritten(RelationGetRelid(rel)); + pgcolumnar_delete_storage_tree(PgColumnarStorageId(rel)); /* @@ -2608,6 +2675,12 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, { Node *parsetree = pstmt->utilityStmt; + /* + * Start from empty: a statement that ERRORED between recording and draining + * must not leave a relid for the next one to act on. + */ + pgcolumnar_forget_rewritten(); + /* read-only inspection, so readOnlyTree needs no copy of the tree */ if (parsetree != NULL && IsA(parsetree, AlterTableStmt)) { @@ -2656,6 +2729,13 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, List *targets = NIL; ListCell *lc; + /* + * Everything the callback saw rewritten. This is the set that catches a + * TRUNCATE ... CASCADE, whose extra tables the statement never names. + */ + foreach(lc, pgcolumnar_rewritten_relids) + targets = lappend_oid(targets, lfirst_oid(lc)); + if (IsA(parsetree, TruncateStmt)) { foreach(lc, ((TruncateStmt *) parsetree)->relations) @@ -2663,7 +2743,7 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, RangeVar *rv = (RangeVar *) lfirst(lc); Oid relid = RangeVarGetRelid(rv, NoLock, true); - if (OidIsValid(relid)) + if (OidIsValid(relid) && !list_member_oid(targets, relid)) targets = lappend_oid(targets, relid); } } @@ -2673,7 +2753,13 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, Oid relid = ats->relation ? RangeVarGetRelid(ats->relation, NoLock, true) : InvalidOid; - if (OidIsValid(relid)) + /* + * The named relation as well as the recorded ones. A rewriting ALTER + * reaches the callback on a transient relation, so nothing is recorded + * for it and the statement's own name is the only route to the + * relation that needs repairing. + */ + if (OidIsValid(relid) && !list_member_oid(targets, relid)) targets = lappend_oid(targets, relid); } @@ -2687,6 +2773,7 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, list_free(kin); } list_free(targets); + pgcolumnar_forget_rewritten(); } /* diff --git a/test/projection_rewrite.sh b/test/projection_rewrite.sh index a30a5ca4..adc93456 100755 --- a/test/projection_rewrite.sh +++ b/test/projection_rewrite.sh @@ -170,6 +170,40 @@ check "t_trunc_multi: the SECOND table in the statement also survives" \ "$(pgc_set_hash "SELECT pgcolumnar.read_projection('t_trunc_two','pp')")" \ "$(pgc_set_hash "SELECT a::text||'|'||b FROM t_trunc_two")" +# TRUNCATE ... CASCADE reaches a table through a FOREIGN KEY. That table is not +# named in the statement and is not an inheritance descendant of anything named, so +# a repair that walks the statement's relation list plus find_all_inheritors never +# visits it (@linuxhikerpm, #892 review). The repair therefore records what the +# table-AM callback actually rewrote instead of re-deriving the statement's reach. +# +# The parent here is a HEAP table with no projections of its own, so the only thing +# that can make this arm pass is the child being repaired. +echo "-- a columnar table truncated through a foreign-key CASCADE" +psql_run "CREATE TABLE cas_parent (id int PRIMARY KEY);" +psql_run "CREATE TABLE cas_child (id int REFERENCES cas_parent(id), v int) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.add_projection('cas_child','pv',ARRAY['id','v'],ARRAY['v']);" +psql_run "INSERT INTO cas_parent SELECT g FROM generate_series(1,$N) g;" +psql_run "INSERT INTO cas_child SELECT g, g%7 FROM generate_series(1,$N) g;" +check "PREMISE the cascade fixture reads before the truncate" \ + "$(q "SELECT count(*) FROM pgcolumnar.read_projection('cas_child','pv');")" "$N" +CAS_SID0="$(q "SELECT pgcolumnar.get_storage_id('cas_child');")" +psql_run "TRUNCATE cas_parent CASCADE;" +if [ "$CAS_SID0" = "$(q "SELECT pgcolumnar.get_storage_id('cas_child');")" ]; then + # If the cascade did not rewrite the child there is nothing to repair, and the + # arm below would pass for the wrong reason. + check_unrunnable "cas_child P1 the cascaded child keeps its projection" \ + UNMET_PRECONDITION "the CASCADE did not rewrite the child" +else + pgc_pass "cas_child: the CASCADE rewrote the child" + check "cas_child P1 the cascaded child keeps its projection" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('cas_child','pv')")" \ + "$(pgc_set_hash "SELECT id::text||'|'||v::text FROM cas_child")" + check "cas_child P2 no retired projection rows" "$(retired_rows)" "0" + check "cas_child P3 declaration survives" \ + "$(q "SELECT count(*) FROM pgcolumnar.projection_declaration + WHERE rel = 'cas_child'::regclass AND name = 'pv';")" "1" +fi + # A partitioned CHILD, rewritten by a type change on the PARENT. The statement # names pt, the rewrite lands on pt1, and pt is not itself a columnar relation -- # so a fix that looks only at the relation named in the statement never fires From 077639519047ae5380b648b028b7b2567f3dc8d6 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 00:09:40 +0000 Subject: [PATCH 03/11] test: reach the stale declaration by a route #888 does not close Rebasing onto main, which now carries #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; #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 #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 #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 #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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/projection_rewrite.sh | 78 +++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 30 deletions(-) diff --git a/test/projection_rewrite.sh b/test/projection_rewrite.sh index adc93456..6ed89545 100755 --- a/test/projection_rewrite.sh +++ b/test/projection_rewrite.sh @@ -248,40 +248,42 @@ arm r_cluster "SELECT pgcolumnar.cluster('r_cluster','a');" REWROT # --------------------------------------------------------------------------- # The re-record must never abort the statement that triggered it. # -# A declaration can name a column the relation no longer has: ALTER TABLE ... -# RENAME COLUMN does not carry the rename through projection_declaration's -# columns/sort_key arrays (#888). The re-record resolves those NAMES against the -# relation as it is now, so on such a table it cannot materialise the projection. +# A declaration can name a column the relation no longer has. Measured before this +# arm existed: the re-record raised `column "a" does not exist` inside an unrelated +# ALTER TABLE ... ALTER COLUMN id TYPE bigint, exit 1, and the type change was +# rolled back -- turning a silently lost projection into a blocked schema change. A +# repair that cannot run must degrade to a WARNING and leave the projection for +# rebuild_projections, which is the documented recovery. # -# What it must not do is take the user's statement down with it. Measured before -# this arm existed: the re-record raised `column "a" does not exist` inside an -# unrelated ALTER TABLE ... ALTER COLUMN id TYPE bigint, exit 1, and the type -# change was rolled back -- turning a silently lost projection into a blocked -# schema change. A repair that cannot run must degrade to a WARNING and leave -# the projection for rebuild_projections, which is the documented recovery. -# -# This arm is independent of whether #888 lands: a declaration can also go stale -# through a path nobody has closed yet, and the statement must survive either way. -echo "-- a stale declaration must not abort the statement (#888 interaction)" +# HOW THE STALE DECLARATION IS PRODUCED, and why it is not a rename. +# ALTER TABLE ... RENAME COLUMN used to leave the declaration behind, and #888 fixed +# that. So a rename can no longer produce this state, and an earlier version of this +# arm correctly reported UNMET_PRECONDITION once #888 landed rather than passing +# vacuously. The state is still reachable: any database created before #888 carries +# it, and nothing guarantees some future path cannot reintroduce it. So the arm +# writes the stale name directly into pgcolumnar.projection_declaration, which is +# exactly what such a database looks like, and keeps testing the property that +# matters -- that the repair cannot take a user's statement down with it. +echo "-- a stale declaration must not abort the statement" psql_run "CREATE TABLE stale (id int, a int, b text) USING pgcolumnar;" psql_run "INSERT INTO stale SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" psql_run "SELECT pgcolumnar.add_projection('stale','pp',ARRAY['a','b'],ARRAY['a']);" -psql_run "ALTER TABLE stale RENAME COLUMN a TO a2;" +# Name a column the table does not have, as a pre-#888 database would after a rename. +psql_run "UPDATE pgcolumnar.projection_declaration + SET columns = ARRAY['gone','b'], sort_key = ARRAY['gone'] + WHERE rel = 'stale'::regclass AND name = 'pp';" STALE_DECL="$(q "SELECT columns::text FROM pgcolumnar.projection_declaration WHERE rel='stale'::regclass;")" -if [ "$STALE_DECL" = "{a,b}" ]; then - pgc_pass "PREMISE the rename left the declaration naming a dead column" +if [ "$STALE_DECL" = "{gone,b}" ]; then + pgc_pass "PREMISE the declaration names a column the table lacks" else - # #888 landing makes this premise false, and then the arm below is vacuous - # rather than passing: it can only test a statement that must survive a - # stale declaration if the declaration is actually stale. check_unrunnable "stale: the statement survives a stale declaration" \ - UNMET_PRECONDITION "declaration is $STALE_DECL, not stale; #888 may have landed" + UNMET_PRECONDITION "declaration is $STALE_DECL, so it is not stale" check_unrunnable "stale: the base table keeps its rows" \ - UNMET_PRECONDITION "declaration is $STALE_DECL, not stale; #888 may have landed" + UNMET_PRECONDITION "declaration is $STALE_DECL, so it is not stale" check_unrunnable "stale: the unrelated type change took effect" \ - UNMET_PRECONDITION "declaration is $STALE_DECL, not stale; #888 may have landed" + UNMET_PRECONDITION "declaration is $STALE_DECL, so it is not stale" fi -if [ "$STALE_DECL" = "{a,b}" ]; then +if [ "$STALE_DECL" = "{gone,b}" ]; then if psql_run "ALTER TABLE stale ALTER COLUMN id TYPE bigint;" >/dev/null 2>&1; then pgc_pass "stale: the statement survives a stale declaration" else @@ -293,15 +295,15 @@ if [ "$STALE_DECL" = "{a,b}" ]; then "$(q "SELECT format_type(atttypid,atttypmod) FROM pg_attribute WHERE attrelid='stale'::regclass AND attname='id';")" "bigint" - # A skip the user is never told about is a silent projection loss, which is - # the whole complaint in #876. Assert the WARNING from its own output, and - # assert the negative control in the same breath: a table whose declaration - # is intact must not produce one, or the arm passes on a warning that fires - # unconditionally. + # A skip the user is never told about is a silent projection loss, which is the + # whole complaint in #876. Assert the WARNING from its own output, and assert the + # negative control in the same breath: a table whose declaration is intact must + # not produce one, or the arm passes on a warning that fires unconditionally. psql_run "CREATE TABLE stale2 (id int, a int, b text) USING pgcolumnar;" >/dev/null psql_run "INSERT INTO stale2 SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" >/dev/null psql_run "SELECT pgcolumnar.add_projection('stale2','pp',ARRAY['a','b'],ARRAY['a']);" >/dev/null - psql_run "ALTER TABLE stale2 RENAME COLUMN a TO a2;" >/dev/null + psql_run "UPDATE pgcolumnar.projection_declaration SET columns = ARRAY['gone','b'], + sort_key = ARRAY['gone'] WHERE rel = 'stale2'::regclass;" >/dev/null check "stale: the skip warns, naming the projection and the recovery" \ "$(psql_run "ALTER TABLE stale2 ALTER COLUMN id TYPE bigint;" 2>&1 | grep -cE 'WARNING:.*could not restore projection "pp"|rebuild_projections')" "2" @@ -313,6 +315,22 @@ if [ "$STALE_DECL" = "{a,b}" ]; then grep -ci warning)" "0" fi +# And the property #888 now guarantees, asserted here too because this suite is the +# one that breaks if it regresses: a rename carries into the declaration, so the +# repair after a later rewrite still resolves. +echo "-- a renamed column no longer strands the declaration (#888)" +psql_run "CREATE TABLE renamed (id int, a int, b text) USING pgcolumnar;" +psql_run "INSERT INTO renamed SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" +psql_run "SELECT pgcolumnar.add_projection('renamed','pp',ARRAY['a','b'],ARRAY['a']);" +psql_run "ALTER TABLE renamed RENAME COLUMN a TO a2;" +check "renamed: the declaration followed the rename" \ + "$(q "SELECT columns::text FROM pgcolumnar.projection_declaration WHERE rel='renamed'::regclass;")" \ + "{a2,b}" +psql_run "ALTER TABLE renamed ALTER COLUMN id TYPE bigint;" +check "renamed: and the projection survives a later rewrite" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('renamed','pp')")" \ + "$(pgc_set_hash "SELECT a2::text||'|'||b FROM renamed")" + echo "-- rebuild_projections stays the documented manual recovery (#876)" psql_run "CREATE TABLE rec (id int, a int, b text) USING pgcolumnar;" psql_run "INSERT INTO rec SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" From 0a4b37c0808f835001b9b69cfe81d90c87e3ebb6 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 9 Sep 2026 00:31:11 +0000 Subject: [PATCH 04/11] fix: the rewritten-relid list must survive a nested utility statement @jdatcmd found this reviewing #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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- src/columnar_tableam.c | 67 ++++++++++++++++++++++++++++++++------ test/projection_rewrite.sh | 50 ++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 10 deletions(-) diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 79f04681..94726c83 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -127,6 +127,30 @@ static const TableAmRoutine pgcolumnar_am_methods; static List *pgcolumnar_rewritten_relids = NIL; static bool pgcolumnar_xact_cb_registered = false; +/* + * How deep we are in nested pgcolumnar_process_utility calls. + * + * ProcessUtility is RE-ENTRANT. An AFTER TRUNCATE trigger whose function runs any + * utility statement calls it again from inside the outer statement, after the + * callback has already recorded the truncated relation. Measured with the callback + * logging its own order: + * + * 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 + * supplies it as a second source. A table reached by FK CASCADE does not: the + * recorded list is its only route, so the projection stays absent and + * read_projection raises 42704. Reproduced with a trigger on the cascaded child. + * + * So the list is cleared only when the OUTERMOST utility statement begins, and a + * drain removes the relids it repaired rather than emptying the list, because an + * inner statement must not discard what an outer one is still holding. + */ +static int pgcolumnar_utility_depth = 0; + static void pgcolumnar_forget_rewritten(void) { @@ -142,6 +166,7 @@ pgcolumnar_rewritten_xact_callback(XactEvent event, void *arg) { /* Any transaction end, committed or not: the list belongs to one statement. */ pgcolumnar_forget_rewritten(); + pgcolumnar_utility_depth = 0; } static void @@ -2676,10 +2701,13 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, Node *parsetree = pstmt->utilityStmt; /* - * Start from empty: a statement that ERRORED between recording and draining - * must not leave a relid for the next one to act on. + * Start from empty, but only for the OUTERMOST statement: a statement that + * ERRORED between recording and draining must not leave a relid for the next + * one, while a NESTED statement must not discard what its caller is holding. */ - pgcolumnar_forget_rewritten(); + if (pgcolumnar_utility_depth == 0) + pgcolumnar_forget_rewritten(); + pgcolumnar_utility_depth++; /* read-only inspection, so readOnlyTree needs no copy of the tree */ if (parsetree != NULL && IsA(parsetree, AlterTableStmt)) @@ -2690,12 +2718,21 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, pgcolumnar_reject_set_am_to_columnar(stmt); } - if (prev_process_utility_hook) - prev_process_utility_hook(pstmt, queryString, readOnlyTree, context, - params, queryEnv, dest, qc); - else - standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, - params, queryEnv, dest, qc); + PG_TRY(); + { + if (prev_process_utility_hook) + prev_process_utility_hook(pstmt, queryString, readOnlyTree, context, + params, queryEnv, dest, qc); + else + standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, + params, queryEnv, dest, qc); + } + PG_CATCH(); + { + pgcolumnar_utility_depth--; + PG_RE_THROW(); + } + PG_END_TRY(); /* * A rewrite loses this relation's declared projections, so re-record them @@ -2772,10 +2809,20 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, PgColumnarRerecordProjectionsAfterRewrite(lfirst_oid(lc2)); list_free(kin); } + + /* + * Forget only what was repaired here. Emptying the list would discard a + * relid an OUTER statement recorded and has not drained yet, which is the + * same defect as clearing on entry from a nested call. + */ + foreach(lc, targets) + pgcolumnar_rewritten_relids = + list_delete_oid(pgcolumnar_rewritten_relids, lfirst_oid(lc)); list_free(targets); - pgcolumnar_forget_rewritten(); } + pgcolumnar_utility_depth--; + /* * A column rename must be carried through the ordering mark (#778). The * mark records its sort key as column NAMES and both ordering self-gates diff --git a/test/projection_rewrite.sh b/test/projection_rewrite.sh index 6ed89545..05968763 100755 --- a/test/projection_rewrite.sh +++ b/test/projection_rewrite.sh @@ -204,6 +204,56 @@ else WHERE rel = 'cas_child'::regclass AND name = 'pv';")" "1" fi +# ProcessUtility is RE-ENTRANT, and the repair records what it must fix in a +# process-global list. An AFTER TRUNCATE trigger whose function runs any utility +# statement calls the hook again from inside the outer statement, after the callback +# has already recorded the truncated relation (@jdatcmd, #892 review). +# +# Measured with the callback logging its own order, before this was fixed: +# 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, which is why this arm composes the nested utility with a CASCADE. +# For a cascaded table the recorded list is the only route, so the two together are +# what leave the projection absent. Either alone passes. +echo "-- a cascade whose trigger runs a nested utility statement" +psql_run "CREATE TABLE reent_parent (id int PRIMARY KEY);" +psql_run "CREATE TABLE reent_child (id int REFERENCES reent_parent(id), v int) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.add_projection('reent_child','pv',ARRAY['id','v'],ARRAY['v']);" +psql_run "INSERT INTO reent_parent SELECT g FROM generate_series(1,$N) g;" +psql_run "INSERT INTO reent_child SELECT g, g%7 FROM generate_series(1,$N) g;" +psql_run "CREATE FUNCTION reent_nested_utility() RETURNS trigger LANGUAGE plpgsql AS \$\$ +BEGIN + -- A utility statement, so it re-enters the hook. Permanent, so the premise + -- below can see from another session that it really ran; a TEMP table would + -- vanish with the trigger's session and the premise would silently read 0. + CREATE TABLE IF NOT EXISTS reent_marker (x int); + RETURN NULL; +END\$\$;" +psql_run "CREATE TRIGGER reent_child_trunc AFTER TRUNCATE ON reent_child + FOR EACH STATEMENT EXECUTE FUNCTION reent_nested_utility();" +check "PREMISE the trigger is installed on the cascaded child" \ + "$(q "SELECT count(*) FROM pg_trigger WHERE tgrelid='reent_child'::regclass AND NOT tgisinternal;")" "1" +check "PREMISE the projection reads before the truncate" \ + "$(q "SELECT count(*) FROM pgcolumnar.read_projection('reent_child','pv');")" "$N" +REENT_SID0="$(q "SELECT pgcolumnar.get_storage_id('reent_child');")" +psql_run "TRUNCATE reent_parent CASCADE;" +check "PREMISE the nested utility statement really ran" \ + "$(q "SELECT count(*) FROM pg_class WHERE relname='reent_marker';")" "1" +if [ "$REENT_SID0" = "$(q "SELECT pgcolumnar.get_storage_id('reent_child');")" ]; then + check_unrunnable "reent_child P1 the projection survives a nested utility" \ + UNMET_PRECONDITION "the CASCADE did not rewrite the child" +else + pgc_pass "reent_child: the CASCADE rewrote the child" + check "reent_child P1 the projection survives a nested utility" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('reent_child','pv')")" \ + "$(pgc_set_hash "SELECT id::text||'|'||v::text FROM reent_child")" + check "reent_child P2 no retired projection rows" "$(retired_rows)" "0" +fi + # A partitioned CHILD, rewritten by a type change on the PARENT. The statement # names pt, the rewrite lands on pt1, and pt is not itself a columnar relation -- # so a fix that looks only at the relation named in the statement never fires From e2ba96461b0c548f3051ec8ad92650396ef24716 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 02:23:23 +0000 Subject: [PATCH 05/11] fix: decide the repair under a weak lock, and contain what it can raise Three defects from @jdatcmd's #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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- src/columnar_projection.c | 108 ++++++++++++++++++++++++++++++++++++-- src/columnar_tableam.c | 3 +- 2 files changed, 105 insertions(+), 6 deletions(-) diff --git a/src/columnar_projection.c b/src/columnar_projection.c index 793ca657..bfb50bf3 100644 --- a/src/columnar_projection.c +++ b/src/columnar_projection.c @@ -397,11 +397,65 @@ PgColumnarRerecordProjectionsAfterRewrite(Oid relid) return; /* - * ShareLock, matching add_projection: the back-fill below reads every live - * row, so concurrent writers must be held off exactly as they are when a - * projection is first created. The statement that rewrote this relation - * already holds AccessExclusiveLock, so this takes nothing new. + * DECIDE FIRST, UNDER AccessShareLock, AND ONLY THEN TAKE ShareLock. + * + * The repair is reached for every AlterTableStmt on a relation with a declared + * projection, not only for one that rewrote it. Measured: three metadata-only + * statements -- SET (fillfactor), ALTER COLUMN SET STATISTICS, + * SET (autovacuum_enabled) -- opened the relation with ShareLock three times + * with nothing to repair (@jdatcmd, #892 review). + * + * ShareLock conflicts with RowExclusiveLock. The old comment justified it as + * "the statement already holds AccessExclusiveLock, so this takes nothing new", + * which is true of a rewriting statement and false of the metadata-only ones + * that also arrive here. So the cheap question is asked under AccessShareLock, + * which conflicts with nothing a writer takes, and the heavier lock is taken + * only when there is a projection to materialise. */ + rel = table_open(relid, AccessShareLock); + storageId = PgColumnarStorageId(rel); + existing = PgColumnarListProjections(storageId); + + { + bool anyMissing = false; + + foreach(lc, decls) + { + PgColumnarProjectionDeclaration *d = + (PgColumnarProjectionDeclaration *) lfirst(lc); + ListCell *lc2; + bool present = false; + + foreach(lc2, existing) + { + PgColumnarProjection *p = (PgColumnarProjection *) lfirst(lc2); + + if (p->projectionId > 0 && strcmp(p->name, d->name) == 0) + { + present = true; + break; + } + } + if (!present) + { + anyMissing = true; + break; + } + } + + if (!anyMissing) + { + table_close(rel, AccessShareLock); + return; + } + } + + /* + * There is work to do, so now take the lock the back-fill needs: it reads + * every live row, and concurrent writers must be held off exactly as they are + * when a projection is first created. + */ + table_close(rel, AccessShareLock); rel = table_open(relid, ShareLock); storageId = PgColumnarStorageId(rel); existing = PgColumnarListProjections(storageId); @@ -446,7 +500,51 @@ PgColumnarRerecordProjectionsAfterRewrite(Oid relid) continue; } - materialize_projection(rel, d->name, d->columns, d->sortKey); + /* + * A repair that cannot finish must not abort the statement that triggered + * it. declaration_resolves above catches the one failure mode we know + * about; materialize_projection can raise for others, and anything it + * raises would otherwise propagate into a statement that succeeds on main + * (@jdatcmd, #892 review). + * + * An internal subtransaction is the only way to catch and continue: after + * an ERROR the transaction is unusable until it is rolled back, so this is + * the same shape plpgsql's EXCEPTION uses. + */ + { + MemoryContext oldcxt = CurrentMemoryContext; + ResourceOwner oldowner = CurrentResourceOwner; + + BeginInternalSubTransaction(NULL); + PG_TRY(); + { + materialize_projection(rel, d->name, d->columns, d->sortKey); + ReleaseCurrentSubTransaction(); + MemoryContextSwitchTo(oldcxt); + CurrentResourceOwner = oldowner; + } + PG_CATCH(); + { + ErrorData *edata; + + MemoryContextSwitchTo(oldcxt); + edata = CopyErrorData(); + FlushErrorState(); + RollbackAndReleaseCurrentSubTransaction(); + MemoryContextSwitchTo(oldcxt); + CurrentResourceOwner = oldowner; + + ereport(WARNING, + (errcode(edata->sqlerrcode), + errmsg("could not restore projection \"%s\" on \"%s\" after rewrite", + d->name, get_rel_name(relid)), + errdetail("%s", edata->message), + errhint("Call pgcolumnar.rebuild_projections(%s) once the cause is fixed.", + quote_literal_cstr(get_rel_name(relid))))); + FreeErrorData(edata); + } + PG_END_TRY(); + } /* the new row must be visible to the next iteration's id/name check */ CommandCounterIncrement(); existing = PgColumnarListProjections(PgColumnarStorageId(rel)); diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 94726c83..85883774 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -2761,7 +2761,8 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, * AccessExclusiveLock on the hierarchy, so NoLock takes nothing new. */ if (parsetree != NULL && - (IsA(parsetree, AlterTableStmt) || IsA(parsetree, TruncateStmt))) + (IsA(parsetree, AlterTableStmt) || IsA(parsetree, TruncateStmt) || + IsA(parsetree, RefreshMatViewStmt))) { List *targets = NIL; ListCell *lc; From df45c35befcb95e6e375cd3cae228ca3a0ba415f Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 02:23:23 +0000 Subject: [PATCH 06/11] test: make the rebuild_projections arm able to fail @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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/projection_rewrite.sh | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/test/projection_rewrite.sh b/test/projection_rewrite.sh index 05968763..0ae4e9c5 100755 --- a/test/projection_rewrite.sh +++ b/test/projection_rewrite.sh @@ -381,15 +381,42 @@ check "renamed: and the projection survives a later rewrite" \ "$(pgc_set_hash "SELECT pgcolumnar.read_projection('renamed','pp')")" \ "$(pgc_set_hash "SELECT a2::text||'|'||b FROM renamed")" -echo "-- rebuild_projections stays the documented manual recovery (#876)" +# rebuild_projections stays the documented manual recovery (#876), and this arm has +# to make the projection GENUINELY ABSENT to test it. +# +# The earlier version truncated and then called rebuild_projections. Once the repair +# in this PR landed, the TRUNCATE was already repaired automatically, so the +# comparison passed whether or not rebuild_projections did anything: it passed with +# the function gutted (@jdatcmd, #892 review). A test named after a function it does +# not exercise is worse than no test, because the name is what a reader trusts. +# +# So the projection rows are deleted directly, which is what a logical restore +# leaves: pg_dump carries pgcolumnar.projection_declaration and cannot carry the +# storage, and that is the case the function was written for. Its RETURN VALUE is +# asserted, not merely the end state, because the count is the direct evidence it +# did the work. +echo "-- rebuild_projections rebuilds a projection whose storage is absent (#876)" psql_run "CREATE TABLE rec (id int, a int, b text) USING pgcolumnar;" psql_run "INSERT INTO rec SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" psql_run "SELECT pgcolumnar.add_projection('rec','pp',ARRAY['a','b'],ARRAY['a']);" -psql_run "TRUNCATE rec; INSERT INTO rec SELECT g, g%9, 'y'||g FROM generate_series(1,200) g;" -psql_run "SELECT pgcolumnar.rebuild_projections('rec');" >/dev/null 2>&1 -check "rebuild_projections still repairs a lost projection" \ +check "PREMISE the projection reads before it is removed" \ + "$(q "SELECT count(*) FROM pgcolumnar.read_projection('rec','pp');")" "$N" +# Simulate the restored state: the declaration survives, the storage does not. +psql_run "DELETE FROM pgcolumnar.projection + WHERE storage_id = pgcolumnar.get_storage_id('rec') AND projection_id > 0;" +check "PREMISE the projection is now genuinely absent" \ + "$(q "SELECT count(*) FROM pgcolumnar.projection + WHERE storage_id = pgcolumnar.get_storage_id('rec') AND projection_id > 0;")" "0" +check "PREMISE and the declaration survived, so a rebuild is possible" \ + "$(q "SELECT count(*) FROM pgcolumnar.projection_declaration + WHERE rel = 'rec'::regclass AND name = 'pp';")" "1" +check "rebuild_projections reports rebuilding exactly one projection" \ + "$(q "SELECT pgcolumnar.rebuild_projections('rec');")" "1" +check "and the rebuilt projection matches the base table" \ "$(pgc_set_hash "SELECT pgcolumnar.read_projection('rec','pp')")" \ "$(pgc_set_hash "SELECT a::text||'|'||b FROM rec")" -check "and leaves no retired projection rows" "$(retired_rows)" "0" +check "and it leaves no retired projection rows" "$(retired_rows)" "0" +check "a second call rebuilds nothing, so it is safe to run at any time" \ + "$(q "SELECT pgcolumnar.rebuild_projections('rec');")" "0" pgc_summary From b811e299e6141088c2908aa4b15304ff9d490bf6 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 02:50:23 +0000 Subject: [PATCH 07/11] fix: dispatch on the node type, drop the dead walk, and fix the hints Four more from @jdatcmd's #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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- src/columnar_projection.c | 38 ++++++++++++++++++++++++---- src/columnar_tableam.c | 52 +++++++++++++++++++++++++-------------- 2 files changed, 66 insertions(+), 24 deletions(-) diff --git a/src/columnar_projection.c b/src/columnar_projection.c index bfb50bf3..614da899 100644 --- a/src/columnar_projection.c +++ b/src/columnar_projection.c @@ -32,6 +32,7 @@ #include "utils/array.h" #include "utils/builtins.h" #include "utils/lsyscache.h" +#include "utils/ruleutils.h" #include "utils/memutils.h" #include "utils/rel.h" #include "utils/snapmgr.h" @@ -348,6 +349,23 @@ pgcolumnar_add_projection(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } +/* + * projection_hint_relname + * The relation name a HINT can be pasted from: schema-qualified and quoted. + * + * get_rel_name() alone is unqualified, and a HINT that names a relation outside + * the reader's search_path tells them to run a statement that fails. Measured on + * 18.4: a stale declaration on a table in schema "s" produced + * HINT: ... pgcolumnar.rebuild_projections('t'), and running exactly that gave + * ERROR: relation "t" does not exist (@jdatcmd, #892 review). + */ +static char * +projection_hint_relname(Oid relid) +{ + return quote_qualified_identifier(get_namespace_name(get_rel_namespace(relid)), + get_rel_name(relid)); +} + /* * PgColumnarRerecordProjectionsAfterRewrite * Re-materialise this relation's declared projections under whatever @@ -493,10 +511,20 @@ PgColumnarRerecordProjectionsAfterRewrite(Oid relid) ereport(WARNING, (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("could not restore projection \"%s\" on \"%s\" after rewrite", - d->name, get_rel_name(relid)), + d->name, projection_hint_relname(relid)), errdetail("Its declaration names a column the table no longer has."), - errhint("Correct the declaration, then call pgcolumnar.rebuild_projections(%s).", - quote_literal_cstr(get_rel_name(relid))))); + /* + * add_projection, not "correct the declaration": there is no operation that + * edits a declaration in place, and the two obvious alternatives were measured + * to fail here. 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: measured, + * it restored all 200 rows on a table whose declaration named a dropped column. + */ + errhint("Call pgcolumnar.add_projection(%s, %s, ...) again, naming columns the table has. That replaces the declaration.", + quote_literal_cstr(projection_hint_relname(relid)), + quote_literal_cstr(d->name)))); continue; } @@ -537,10 +565,10 @@ PgColumnarRerecordProjectionsAfterRewrite(Oid relid) ereport(WARNING, (errcode(edata->sqlerrcode), errmsg("could not restore projection \"%s\" on \"%s\" after rewrite", - d->name, get_rel_name(relid)), + d->name, projection_hint_relname(relid)), errdetail("%s", edata->message), errhint("Call pgcolumnar.rebuild_projections(%s) once the cause is fixed.", - quote_literal_cstr(get_rel_name(relid))))); + quote_literal_cstr(projection_hint_relname(relid))))); FreeErrorData(edata); } PG_END_TRY(); diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 85883774..3168d73d 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -2774,29 +2774,43 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, foreach(lc, pgcolumnar_rewritten_relids) targets = lappend_oid(targets, lfirst_oid(lc)); - if (IsA(parsetree, TruncateStmt)) - { - foreach(lc, ((TruncateStmt *) parsetree)->relations) - { - RangeVar *rv = (RangeVar *) lfirst(lc); - Oid relid = RangeVarGetRelid(rv, NoLock, true); - - if (OidIsValid(relid) && !list_member_oid(targets, relid)) - targets = lappend_oid(targets, relid); - } - } - else + /* + * TRUNCATE needs nothing from the statement itself. + * + * It reaches the table-AM callback for every relation it rewrites, + * including the ones it never names -- a CASCADE, and every partition of a + * named parent -- so the recorded list already holds them. Walking the + * statement's own relation list on top of that was measured to add nothing: + * instrumented across the 80 checks in test/projection_rewrite.sh it fired + * twice, both times for a NON-columnar parent in a cascade arm, where the + * repair is a no-op. Deleting it left all 80 green (@jdatcmd, #892 review). + * + * A rewriting ALTER is the opposite case, and the reason this branch exists + * at all: it reaches the callback as the TRANSIENT relation make_new_heap + * builds, so nothing is recorded for the user's relation and the statement's + * own name is the only route to it. + */ + if (!IsA(parsetree, TruncateStmt)) { - AlterTableStmt *ats = (AlterTableStmt *) parsetree; - Oid relid = ats->relation ? - RangeVarGetRelid(ats->relation, NoLock, true) : InvalidOid; + RangeVar *rv; + Oid relid; /* - * The named relation as well as the recorded ones. A rewriting ALTER - * reaches the callback on a transient relation, so nothing is recorded - * for it and the statement's own name is the only route to the - * relation that needs repairing. + * Dispatch on the node type rather than casting to AlterTableStmt for + * both. The two structs happen to place `relation` at the same offset + * -- measured as 8 on PG 15 through 19, because NodeTag is 4 bytes and + * RefreshMatViewStmt's two bools fit in its tail padding -- so a single + * cast reads the right field today. It does so by coincidence of + * 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)) + rv = ((RefreshMatViewStmt *) parsetree)->relation; + else + rv = ((AlterTableStmt *) parsetree)->relation; + + relid = rv ? RangeVarGetRelid(rv, NoLock, true) : InvalidOid; + if (OidIsValid(relid) && !list_member_oid(targets, relid)) targets = lappend_oid(targets, relid); } From 6c7e1ec2f80218cfb2e6e4c834c2e9afb036e27f Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 02:50:23 +0000 Subject: [PATCH 08/11] test: cover REFRESH, the already-present guard, and fix two vacuous arms 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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/projection_rewrite.sh | 102 ++++++++++++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 7 deletions(-) diff --git a/test/projection_rewrite.sh b/test/projection_rewrite.sh index 0ae4e9c5..9ef23072 100755 --- a/test/projection_rewrite.sh +++ b/test/projection_rewrite.sh @@ -67,10 +67,14 @@ check "PREMISE two catalog rows under the current storage" \ # stranding them (#867). It is here to redden if a fix strands them instead -- # which re-recording in the wrong place does. retired_rows() { + # relkind IN ('r','m'): a MATERIALIZED VIEW can be columnar and carry a + # projection, and 'r' alone counted a live matview's rows as retired. That + # poisoned four later arms, because this helper is global rather than + # per-relation: every P2 check after the matview arm read the same 2. q "SELECT count(*) FROM pgcolumnar.projection p WHERE NOT EXISTS ( SELECT 1 FROM pg_class c JOIN pg_am am ON am.oid = c.relam - WHERE am.amname = 'pgcolumnar' AND c.relkind = 'r' + WHERE am.amname = 'pgcolumnar' AND c.relkind IN ('r','m') AND pgcolumnar.get_storage_id(c.oid) = p.storage_id);" } check "PREMISE no retired projection rows to begin with" "$(retired_rows)" "0" @@ -275,6 +279,64 @@ else "$(pgc_set_hash "SELECT a::text||'|'||b FROM ptr1")" fi +echo "-- the repair must be a no-op when the projection is already present" +# The already-present guard had no arm (@jdatcmd, #892 review), and it runs far more +# often than the repair does: EVERY AlterTableStmt on a relation with a declared +# projection reaches the repair, including the ones that rewrite nothing. That is +# also why the decision is now made under AccessShareLock -- these three statements +# take ShareUpdateExclusiveLock, and the repair must not escalate past them. +psql_run "CREATE TABLE noop (id int, a int, b text) USING pgcolumnar;" +psql_run "INSERT INTO noop SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" +psql_run "SELECT pgcolumnar.add_projection('noop','pp',ARRAY['a','b'],ARRAY['a']);" +NOOP_SID="$(q "SELECT pgcolumnar.get_storage_id('noop');")" +check "PREMISE the projection is present before the metadata-only statements" \ + "$(q "SELECT count(*) FROM pgcolumnar.projection + WHERE storage_id = pgcolumnar.get_storage_id('noop');")" "2" +psql_run "ALTER TABLE noop ALTER COLUMN a SET STATISTICS 50;" +psql_run "ALTER TABLE noop SET (autovacuum_enabled = false);" +check "noop: the metadata-only statements rewrote nothing" \ + "$(q "SELECT pgcolumnar.get_storage_id('noop');")" "$NOOP_SID" +check "noop: and the repair added no projection row" \ + "$(q "SELECT count(*) FROM pgcolumnar.projection + WHERE storage_id = pgcolumnar.get_storage_id('noop');")" "2" +check "noop: and the projection still agrees with the base table" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('noop','pp')")" \ + "$(pgc_set_hash "SELECT a::text||'|'||b FROM noop")" + +echo "-- REFRESH MATERIALIZED VIEW: a rewrite that is neither ALTER nor TRUNCATE" +# The third rewriting utility shape, and the one a gate naming only AlterTableStmt +# and TruncateStmt lets through (@jdatcmd, #892 review). Proved load-bearing by +# removal: with RefreshMatViewStmt taken out of the gate, this arm 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 has no arm because it is not a rewrite: measured, the +# storage id is unchanged across it, so there is nothing to repair and an arm would +# be permanently unrunnable rather than merely green. +psql_run "CREATE TABLE mv_base (id int, a int, b text);" +psql_run "INSERT INTO mv_base SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" +psql_run "CREATE MATERIALIZED VIEW mv1 USING pgcolumnar AS SELECT id, a, b FROM mv_base;" +psql_run "SELECT pgcolumnar.add_projection('mv1','pp',ARRAY['a','b'],ARRAY['a']);" +check "PREMISE the matview projection reads before the refresh" \ + "$(q "SELECT count(*) FROM pgcolumnar.read_projection('mv1','pp');")" "$N" +MV_SID0="$(q "SELECT pgcolumnar.get_storage_id('mv1');")" +psql_run "UPDATE mv_base SET b = 'z'||id WHERE id <= 10;" +psql_run "REFRESH MATERIALIZED VIEW mv1;" +if [ "$MV_SID0" = "$(q "SELECT pgcolumnar.get_storage_id('mv1');")" ]; then + check_unrunnable "mv1 P1 the refreshed matview keeps its projection" \ + UNMET_PRECONDITION "REFRESH did not rewrite the matview" +else + pgc_pass "mv1: REFRESH rewrote the matview" + check "mv1 P1 the refreshed matview keeps its projection" \ + "$(pgc_set_hash "SELECT pgcolumnar.read_projection('mv1','pp')")" \ + "$(pgc_set_hash "SELECT a::text||'|'||b FROM mv1")" +fi +check "mv1 P2 no projection row names a storage the matview no longer has" \ + "$(retired_rows)" "0" +check "mv1 P3 the declaration survives the refresh" \ + "$(q "SELECT count(*) FROM pgcolumnar.projection_declaration + WHERE rel = 'mv1'::regclass AND name = 'pp';")" "1" + echo "-- and the base projection must still name every live column" check "t_addcol_vol base projection covers the added column" \ "$(q "SELECT columns FROM pgcolumnar.projection @@ -354,15 +416,41 @@ if [ "$STALE_DECL" = "{gone,b}" ]; then psql_run "SELECT pgcolumnar.add_projection('stale2','pp',ARRAY['a','b'],ARRAY['a']);" >/dev/null psql_run "UPDATE pgcolumnar.projection_declaration SET columns = ARRAY['gone','b'], sort_key = ARRAY['gone'] WHERE rel = 'stale2'::regclass;" >/dev/null - check "stale: the skip warns, naming the projection and the recovery" \ - "$(psql_run "ALTER TABLE stale2 ALTER COLUMN id TYPE bigint;" 2>&1 | - grep -cE 'WARNING:.*could not restore projection "pp"|rebuild_projections')" "2" + # Assert the SQLSTATE, not the prose. The message text is prose and will be + # reworded -- it already was, in this very PR -- while 42703 is the contract. + # The earlier version counted lines matching the text OR the string + # "rebuild_projections", and rewording the HINT to name the recovery that + # actually works would have silently halved that count (@jdatcmd, #892 review). + STALE_OUT="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" \ + -U postgres -d "$PGC_DB" -v VERBOSITY=verbose \ + -c "ALTER TABLE stale2 ALTER COLUMN id TYPE bigint;" 2>&1)" + check "stale: the skip warns with SQLSTATE 42703" \ + "$(printf '%s\n' "$STALE_OUT" | grep -c '^WARNING: 42703: ')" "1" + check "stale: and the warning names the projection it skipped" \ + "$(printf '%s\n' "$STALE_OUT" | grep -c 'could not restore projection "pp"')" "1" + check "stale: and it names a recovery the extension actually offers" \ + "$(printf '%s\n' "$STALE_OUT" | grep -c 'add_projection')" "1" + psql_run "CREATE TABLE fresh2 (id int, a int, b text) USING pgcolumnar;" >/dev/null psql_run "INSERT INTO fresh2 SELECT g, g%50, 'b'||g FROM generate_series(1,$N) g;" >/dev/null psql_run "SELECT pgcolumnar.add_projection('fresh2','pp',ARRAY['a','b'],ARRAY['a']);" >/dev/null - check "stale: an intact declaration produces NO warning" \ - "$(psql_run "ALTER TABLE fresh2 ALTER COLUMN id TYPE bigint;" 2>&1 | - grep -ci warning)" "0" + # The negative control needs its own premise. "no line matched WARNING" is + # satisfied by NO OUTPUT AT ALL, so a statement that never ran would pass it + # (@jdatcmd, #892 review). Assert that the statement ran and rewrote first. + FRESH_SID0="$(q "SELECT pgcolumnar.get_storage_id('fresh2');")" + FRESH_OUT="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" \ + -U postgres -d "$PGC_DB" \ + -c "ALTER TABLE fresh2 ALTER COLUMN id TYPE bigint;" 2>&1)" + check "PREMISE the control statement completed, so its output is not empty" \ + "$(printf '%s\n' "$FRESH_OUT" | grep -c '^ALTER TABLE$')" "1" + if [ "$FRESH_SID0" = "$(q "SELECT pgcolumnar.get_storage_id('fresh2');")" ]; then + check_unrunnable "stale: an intact declaration produces NO warning" \ + UNMET_PRECONDITION "the control ALTER did not rewrite fresh2" + else + pgc_pass "PREMISE the control statement rewrote fresh2, so a warning was possible" + check "stale: an intact declaration produces NO warning" \ + "$(printf '%s\n' "$FRESH_OUT" | grep -ci warning)" "0" + fi fi # And the property #888 now guarantees, asserted here too because this suite is the From 64f569c6a0bbfc8135fac14a34c46930c709d1d2 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 02:50:23 +0000 Subject: [PATCH 09/11] docs: correct the projection limitation and the recovery it names docs/limitations.md documented a limitation main has already fixed: #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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 67 +++++++++++++++++++++++++++++++------------ docs/limitations.md | 34 +++++++++++++++++----- docs/sql-reference.md | 27 +++++++++++------ 3 files changed, 94 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b4920f0..4aba729d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,11 +60,15 @@ true until the next version shipped. `pgcolumnar.rebuild_projections()`; the projections are now re-recorded automatically and the manual call is no longer part of the routine path. - **Four shapes lost the projection, not the two the issue named.** Swept on - 18.4 rather than reasoned about: `TRUNCATE` including its multi-table form, a - type change on a covered or an uncovered column, `ADD COLUMN` with a volatile - default, and **a partitioned child rewritten by a type change on its parent** -- - where the statement names the parent, which is not itself a columnar relation. + **Five shapes lost the projection, not the two the issue named.** Swept on + 18.4 rather than reasoned about: `TRUNCATE` including its multi-table form and + its `CASCADE` form, a type change on a covered or an uncovered column, `ADD + COLUMN` with a volatile default, **a partitioned child rewritten by a type + change on its parent** -- where the statement names the parent, which is not + itself a columnar relation -- and `REFRESH MATERIALIZED VIEW`, which is neither + an `AlterTableStmt` nor a `TruncateStmt` and so escaped a gate naming only those + two. `REFRESH ... CONCURRENTLY` is not a rewrite: the storage id is unchanged + across it, measured. `ADD COLUMN` with a constant default, `DROP COLUMN`, `VACUUM`, `SET ACCESS METHOD` to the same method, `SET TABLESPACE` and a no-op type change do not rewrite and were never affected. Core `VACUUM FULL` and `CLUSTER` are refused @@ -90,19 +94,46 @@ true until the next version shipped. implementation. **A repair that cannot run degrades to a WARNING and never fails the statement - that triggered it.** `ALTER TABLE ... RENAME COLUMN` does not carry the rename - into the declaration, so a declaration can name a column the table no longer - has. Before this was handled, the repair raised `column "a" does not exist` - inside an unrelated `ALTER TABLE ... ALTER COLUMN id TYPE bigint` and rolled that - type change back -- turning a silently lost projection into a blocked schema - change. It now reports - - WARNING: could not restore projection "p" on "t" after rewrite + that triggered it.** A declaration can name a column the table no longer has; + `ALTER TABLE ... RENAME COLUMN` used to leave one behind and no longer does, so + the reachable case is a database created before that fix. Before this was + handled, the repair raised `column "a" does not exist` inside an unrelated + `ALTER TABLE ... ALTER COLUMN id TYPE bigint` and rolled that type change back -- + turning a silently lost projection into a blocked schema change. It now reports + + WARNING: 42703: could not restore projection "p" on "public.t" after rewrite DETAIL: Its declaration names a column the table no longer has. - HINT: Correct the declaration, then call - pgcolumnar.rebuild_projections('t'). - - and leaves the projection to that function. + HINT: Call pgcolumnar.add_projection('public.t', 'p', ...) again, + naming columns the table has. That replaces the declaration. + + **The HINT names `add_projection`, not `rebuild_projections`, because the other + two candidates were measured to fail.** `rebuild_projections()` re-runs the same + stale declaration and raises the same missing-column error; + `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. The relation name in both messages is schema-qualified: + unqualified, a HINT for a table outside the reader's `search_path` told them to + run a statement that fails with `relation "t" does not exist`. + + **Every other failure is contained too.** The resolves-check covers one cause, + and `materialize_projection` can raise for others, so it runs in an internal + subtransaction. A failure is rolled back and reported as a WARNING carrying the + original SQLSTATE, and the user's statement continues. + + **The repair decides under `AccessShareLock` and escalates only when there is + work.** It is reached for every `AlterTableStmt` on a relation with a declared + projection, not only for one that rewrote it, so an unconditional `ShareLock` + blocked concurrent writers on statements that rewrite nothing: `ALTER COLUMN SET + STATISTICS` and `SET (autovacuum_enabled)` take only + `ShareUpdateExclusiveLock`, and each opened the relation with `ShareLock` with + nothing to repair. + + **The rewritten relations are recorded, not re-derived from the statement.** A + `TRUNCATE ... CASCADE` rewrites tables it never names, so the table-AM callback + records each relation whose storage it retires and `ProcessUtility` drains that + list. The list survives a nested utility statement -- a cascade whose trigger + runs one -- because it is cleared only for the outermost statement, and the + drain removes only the relids it repaired rather than emptying it. - The `42704` hint no longer names a rewrite as the likely cause, since a rewrite now re-records. It names the two cases that remain: a declaration that no longer @@ -110,7 +141,7 @@ true until the next version shipped. ### Added -- `test/projection_rewrite.sh`, 53 checks. Nothing in the tree asserted that a +- `test/projection_rewrite.sh`, 84 checks. Nothing in the tree asserted that a projection answers after a rewrite, which is why this was silent. Every arm compares a `pgc_set_hash` of `read_projection` against the base table diff --git a/docs/limitations.md b/docs/limitations.md index edafd722..f4bf3886 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -510,13 +510,33 @@ or an explicit `VACUUM` marks the group. Turn the feature off with ## Projections -`ALTER TABLE ... RENAME COLUMN` does not rename the column inside a projection's -declaration. The projection itself keeps working, because its storage records -attnums rather than names. What breaks is anything that reads the declaration -back. There are two such readers. `pgcolumnar.rebuild_projections()` needs the -declaration after a logical restore. The automatic re-record needs it after a -rewrite, and reports an unusable declaration as a WARNING. To recover, rename the -column back, or drop and re-declare the projection. +A declaration can name a column the table no longer has. `ALTER TABLE ... RENAME +COLUMN` used to leave one behind, and no longer does: the rename is carried into +the declaration. A database created before that fix can still hold one. + +An unusable declaration does not break the projection itself, because its storage +records attnums rather than names. It breaks the two readers of the declaration. +`pgcolumnar.rebuild_projections()` reads it after a logical restore. The automatic +re-record reads it after a rewrite, and reports one it cannot resolve as a +WARNING. + +To recover, call `pgcolumnar.add_projection()` again with the same projection name +and columns the table has. That replaces the declaration and materialises the +projection. `pgcolumnar.rebuild_projections()` cannot recover this state, because +it re-runs the same declaration and raises the same missing-column error, and +`pgcolumnar.drop_projection()` cannot either, because it refuses with `42704` when +the projection row is exactly what is absent. + +A rewrite that does not pass through `ProcessUtility` is not re-recorded, and needs +`pgcolumnar.rebuild_projections()` by hand. + +One such rewrite ships with PostgreSQL. A `TRUNCATE` replicated to a subscriber is +applied by the logical replication worker, which calls `ExecuteTruncateGuts` +directly rather than going through `ProcessUtility` +(`src/backend/replication/logical/worker.c`, `apply_handle_truncate`, checked +against PostgreSQL 18.4). The subscriber's table is rewritten and nothing observes +it, so a projection on a subscriber table is lost by a replicated `TRUNCATE`. Run +`pgcolumnar.rebuild_projections()` on the subscriber after one. A projection is an additional sorted copy. Each projection therefore adds write cost and storage cost. `pgcolumnar.vacuum` builds the projections again. diff --git a/docs/sql-reference.md b/docs/sql-reference.md index 4df2c785..abc28571 100644 --- a/docs/sql-reference.md +++ b/docs/sql-reference.md @@ -470,17 +470,26 @@ each table in the database. Run this after a logical restore. A second run builds nothing, so it is safe to run at any time. -You no longer need it after a rewrite. `TRUNCATE`, a rewriting `ALTER TABLE`, and -the maintenance rewrites re-record their projections themselves. Two cases still -need this function. The first is a logical restore. The second is a declaration -that names a column the table no longer has, because -`ALTER TABLE ... RENAME COLUMN` does not yet carry the rename into the -declaration. A rewrite that meets such a declaration reports it as - - WARNING: could not restore projection "p" on "t" after rewrite +You no longer need it after a rewrite. `TRUNCATE`, a rewriting `ALTER TABLE`, +`REFRESH MATERIALIZED VIEW` and the maintenance rewrites re-record their +projections themselves. + +Two cases still need this function. The first is a logical restore. The second is +a rewrite that does not pass through `ProcessUtility`, and so is never observed: +a `TRUNCATE` replicated to a subscriber is applied by the logical replication +worker calling `ExecuteTruncateGuts` directly. Run this on the subscriber after +one. + +A third case looks like this function's job and is not. A declaration that names a +column the table no longer has is reported by a rewrite as + + WARNING: 42703: could not restore projection "p" on "public.t" after rewrite DETAIL: Its declaration names a column the table no longer has. -Correct the declaration, then run this. +Do not run this function for that. It re-runs the same declaration and raises the +same missing-column error. Call `pgcolumnar.add_projection()` again instead, with +the same projection name and columns the table has, which is what the `HINT` on +that WARNING tells you. ```sql SELECT pgcolumnar.rebuild_projections(); From b12267ab7d02babde54fb1279156ac3aea6fa19c Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 02:58:16 +0000 Subject: [PATCH 10/11] docs: split three sentences over the plain-language limit 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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- docs/limitations.md | 12 ++++++------ docs/sql-reference.md | 7 +++---- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/docs/limitations.md b/docs/limitations.md index f4bf3886..72da7d5f 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -522,20 +522,20 @@ WARNING. To recover, call `pgcolumnar.add_projection()` again with the same projection name and columns the table has. That replaces the declaration and materialises the -projection. `pgcolumnar.rebuild_projections()` cannot recover this state, because -it re-runs the same declaration and raises the same missing-column error, and -`pgcolumnar.drop_projection()` cannot either, because it refuses with `42704` when -the projection row is exactly what is absent. +projection. `pgcolumnar.rebuild_projections()` cannot recover this state. It +re-runs the same declaration and raises the same missing-column error. +`pgcolumnar.drop_projection()` cannot either. It refuses with `42704`, because the +projection row is exactly what is absent. A rewrite that does not pass through `ProcessUtility` is not re-recorded, and needs `pgcolumnar.rebuild_projections()` by hand. One such rewrite ships with PostgreSQL. A `TRUNCATE` replicated to a subscriber is -applied by the logical replication worker, which calls `ExecuteTruncateGuts` +applied by the logical replication worker. That worker calls `ExecuteTruncateGuts` directly rather than going through `ProcessUtility` (`src/backend/replication/logical/worker.c`, `apply_handle_truncate`, checked against PostgreSQL 18.4). The subscriber's table is rewritten and nothing observes -it, so a projection on a subscriber table is lost by a replicated `TRUNCATE`. Run +it. So a replicated `TRUNCATE` loses a projection on a subscriber table. Run `pgcolumnar.rebuild_projections()` on the subscriber after one. A projection is an additional sorted copy. Each projection therefore adds write diff --git a/docs/sql-reference.md b/docs/sql-reference.md index abc28571..e4378afd 100644 --- a/docs/sql-reference.md +++ b/docs/sql-reference.md @@ -475,10 +475,9 @@ You no longer need it after a rewrite. `TRUNCATE`, a rewriting `ALTER TABLE`, projections themselves. Two cases still need this function. The first is a logical restore. The second is -a rewrite that does not pass through `ProcessUtility`, and so is never observed: -a `TRUNCATE` replicated to a subscriber is applied by the logical replication -worker calling `ExecuteTruncateGuts` directly. Run this on the subscriber after -one. +a rewrite that does not pass through `ProcessUtility`, so nothing observes it. A +`TRUNCATE` replicated to a subscriber is one: the logical replication worker calls +`ExecuteTruncateGuts` directly. Run this on the subscriber after one. A third case looks like this function's job and is not. A declaration that names a column the table no longer has is reported by a rewrite as From 02b1ed410ae3867ba4968314872d1e1765c1eab0 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 02:59:44 +0000 Subject: [PATCH 11/11] docs: state the recorded-relid invariant that actually holds @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) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- src/columnar_tableam.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 3168d73d..c18b38ca 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -103,10 +103,26 @@ static const struct config_enum_entry pgcolumnar_compression_options[] = { static const TableAmRoutine pgcolumnar_am_methods; /* - * Relations this statement actually rewrote, recorded by + * Relations whose columnar storage was replaced, recorded by * pgcolumnar_relation_set_new_filelocator and drained by * pgcolumnar_process_utility so their projections can be re-recorded (#876, #887). * + * THE RECORDING IS WIDER THAN THE DRAIN, deliberately, and this is the invariant + * to hold in mind: the callback records every replaced storage, while the drain + * runs only for AlterTableStmt, TruncateStmt and RefreshMatViewStmt. So the list + * can hold a relid nothing will drain -- pgcolumnar.vacuum() rewrites through the + * same callback, and it is a function call, not a utility statement + * (@jdatcmd, #892 review). + * + * Narrowing the recording to match the drain was considered and not done, because + * nothing observable follows from the asymmetry and a change no test can redden is + * worse than a stated invariant. Two reasons it is inert. A relid recorded outside + * a utility statement is cleared by pgcolumnar_forget_rewritten() when the next + * outermost one begins, before that statement records anything of its own. And a + * relid recorded by a nested call -- a vacuum() run from a trigger inside an ALTER + * -- is drained by the enclosing statement, where the repair finds the projection + * already present and returns under AccessShareLock without doing work. + * * Recorded rather than re-derived from the statement, because the statement does * not name everything it rewrites. TRUNCATE ... CASCADE reaches a table through a * foreign key: it is neither listed in TruncateStmt->relations nor an inheritance