diff --git a/CHANGELOG.md b/CHANGELOG.md index b467fe31..4aba729d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,112 @@ 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. + + **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 + 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.** 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: 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 + resolves, and the implicit base projection, which is not readable by name at all. + +### Added + +- `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 + 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..72da7d5f 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -510,6 +510,34 @@ or an explicit `VACUUM` marks the group. Turn the feature off with ## Projections +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. 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. 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 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 cost and storage cost. `pgcolumnar.vacuum` builds the projections again. diff --git a/docs/sql-reference.md b/docs/sql-reference.md index d96ad26f..e4378afd 100644 --- a/docs/sql-reference.md +++ b/docs/sql-reference.md @@ -470,6 +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`, +`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`, 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 + + WARNING: 42703: could not restore projection "p" on "public.t" after rewrite + DETAIL: Its declaration names a column the table no longer has. + +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(); ``` 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..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" @@ -143,60 +144,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 +271,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 +283,304 @@ 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(); } +/* + * 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 + * 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; + + /* + * 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); + + 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, projection_hint_relname(relid)), + errdetail("Its declaration names a column the table no longer has."), + /* + * 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; + } + + /* + * 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, projection_hint_relname(relid)), + errdetail("%s", edata->message), + errhint("Call pgcolumnar.rebuild_projections(%s) once the cause is fixed.", + quote_literal_cstr(projection_hint_relname(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)); + } + + table_close(rel, ShareLock); +} + /* * pgcolumnar.drop_projection(rel, name) * Drop a declared projection. The base projection cannot be dropped. @@ -342,11 +642,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 +786,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 +980,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..c18b38ca 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -102,6 +102,108 @@ 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 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 + * 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; + +/* + * 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) +{ + 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(); + pgcolumnar_utility_depth = 0; +} + +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 +920,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 +2716,15 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, { Node *parsetree = pstmt->utilityStmt; + /* + * 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. + */ + 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)) { @@ -2617,12 +2734,125 @@ 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 + * (#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) || + IsA(parsetree, RefreshMatViewStmt))) + { + 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)); + + /* + * 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)) + { + RangeVar *rv; + Oid relid; + + /* + * 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); + } + + 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); + } + + /* + * 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_utility_depth--; /* * A column rename must be carried through the ordering mark (#778). The diff --git a/test/projection_rewrite.sh b/test/projection_rewrite.sh new file mode 100755 index 00000000..9ef23072 --- /dev/null +++ b/test/projection_rewrite.sh @@ -0,0 +1,510 @@ +#!/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() { + # 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 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" + +# --------------------------------------------------------------------------- +# 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")" + +# 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 + +# 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 +# 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 "-- 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 + 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. 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. +# +# 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']);" +# 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" = "{gone,b}" ]; then + pgc_pass "PREMISE the declaration names a column the table lacks" +else + check_unrunnable "stale: the statement survives a stale declaration" \ + 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, so it is not stale" + check_unrunnable "stale: the unrelated type change took effect" \ + UNMET_PRECONDITION "declaration is $STALE_DECL, so it is not stale" +fi +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 + 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 "UPDATE pgcolumnar.projection_declaration SET columns = ARRAY['gone','b'], + sort_key = ARRAY['gone'] WHERE rel = 'stale2'::regclass;" >/dev/null + # 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 + # 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 +# 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")" + +# 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']);" +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 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 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