diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 145c55db..aebae5a1 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -2468,6 +2468,138 @@ pgcolumnar_reject_set_am_to_columnar(AlterTableStmt *stmt) } } +/* + * pgcolumnar_reject_drop_projected_column + * Keep DROP COLUMN from leaving a projection pointed at a dropped attribute. + * + * Projection definitions are extension metadata, not pg_depend objects, so core + * cannot discover this dependency. Without this check DROP succeeds, the + * projection retains the old attnum, and its next sorted write asks typcache for + * the dropped pg_attribute's type OID zero. Dropping a stored column that is not + * a sort key is quieter but still wrong: the projection subsequently reads + * empty. Refuse either shape at the DDL boundary until projections can + * participate in DROP ... CASCADE. + */ +static void +pgcolumnar_reject_drop_projected_column(AlterTableStmt *stmt) +{ + Oid relid; + List *relations; + ListCell *cmdCell; + bool hasDrop = false; + + foreach(cmdCell, stmt->cmds) + { + AlterTableCmd *cmd = (AlterTableCmd *) lfirst(cmdCell); + + if (cmd->subtype == AT_DropColumn) + { + hasDrop = true; + break; + } + } + if (!hasDrop) + return; + + /* + * Check ownership before taking our lock or reading projection metadata. + * standard_ProcessUtility has not run yet, so without this ordering a caller + * with no rights on the table can distinguish projected from unprojected + * columns by SQLSTATE and retain a ShareUpdateExclusiveLock until transaction + * end. + */ + relid = RangeVarGetRelid(stmt->relation, NoLock, true); + if (!OidIsValid(relid)) + return; + PgColumnarRequireTableOwnerByOid(relid); + + /* + * add_projection() takes ShareLock, which conflicts with our + * ShareUpdateExclusiveLock. Retain this lock while core upgrades to + * AccessExclusiveLock for the ALTER, so a projection cannot be added between + * this check and DROP. Recheck ownership after locking because ALTER OWNER + * could have raced the unlocked check above. + */ + relid = RangeVarGetRelid(stmt->relation, ShareUpdateExclusiveLock, true); + if (!OidIsValid(relid)) + return; + PgColumnarRequireTableOwnerByOid(relid); + + if (stmt->relation->inh) + relations = find_all_inheritors(relid, ShareUpdateExclusiveLock, NULL); + else + relations = list_make1_oid(relid); + + foreach(cmdCell, stmt->cmds) + { + AlterTableCmd *cmd = (AlterTableCmd *) lfirst(cmdCell); + ListCell *relCell; + + if (cmd->subtype != AT_DropColumn || cmd->name == NULL) + continue; + + foreach(relCell, relations) + { + Oid kid = lfirst_oid(relCell); + AttrNumber attnum; + Relation rel; + uint64 storageId; + List *projections; + ListCell *projectionCell; + + if (get_rel_relkind(kid) != RELKIND_RELATION || + !PgColumnarIsColumnarRelation(kid)) + continue; + + attnum = get_attnum(kid, cmd->name); + if (attnum == InvalidAttrNumber) + continue; /* DROP IF EXISTS of a missing column */ + + rel = table_open(kid, NoLock); + storageId = PgColumnarStorageId(rel); + projections = PgColumnarListProjections(storageId); + + foreach(projectionCell, projections) + { + PgColumnarProjection *projection = + (PgColumnarProjection *) lfirst(projectionCell); + int i; + + if (projection->projectionId == 0) + continue; + + /* + * add_projection() requires every sort-key column to appear in + * columns, so this one loop covers both stored-only columns and + * the sort keys whose dropped type would break the writer. + */ + for (i = 0; i < projection->columnsLen; i++) + { + if (projection->columns[i] != attnum) + continue; + + ereport(ERROR, + (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST), + errmsg("cannot drop column \"%s\" because projection \"%s\" depends on it", + cmd->name, projection->name), + errdetail("Projection \"%s\" on table \"%s\" stores this column.", + projection->name, + RelationGetRelationName(rel)), + errhint("Drop the projection with pgcolumnar.drop_projection() before dropping the column."))); + } + } + + /* + * Keep the lock until transaction end; standard_ProcessUtility will + * upgrade it for ALTER TABLE after this hook returns. + */ + table_close(rel, NoLock); + } + } + + list_free(relations); +} + static void pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, bool readOnlyTree, ProcessUtilityContext context, @@ -2478,7 +2610,12 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, /* read-only inspection, so readOnlyTree needs no copy of the tree */ if (parsetree != NULL && IsA(parsetree, AlterTableStmt)) - pgcolumnar_reject_set_am_to_columnar((AlterTableStmt *) parsetree); + { + AlterTableStmt *stmt = (AlterTableStmt *) parsetree; + + pgcolumnar_reject_drop_projected_column(stmt); + pgcolumnar_reject_set_am_to_columnar(stmt); + } if (prev_process_utility_hook) prev_process_utility_hook(pstmt, queryString, readOnlyTree, context, diff --git a/test/projection_drop_column.sh b/test/projection_drop_column.sh new file mode 100755 index 00000000..72463166 --- /dev/null +++ b/test/projection_drop_column.sh @@ -0,0 +1,80 @@ +#!/usr/bin/env bash +# +# pgColumnar: DROP COLUMN must not invalidate a materialized projection. +# +# Projections are extension metadata rather than pg_depend objects. PostgreSQL +# therefore accepted DROP COLUMN even when a projection stored that column. +# Dropping its sort key left the projection's attnum pointing at a dropped +# pg_attribute row; the next INSERT failed in lookup_type_cache with +# "type with OID 0 does not exist", making the table unwritable. +# +# Until projections can participate in DROP ... CASCADE, reject the dependent +# DROP at its public DDL boundary and tell the operator to drop the projection. +# +# Usage: test/projection_drop_column.sh [PG_CONFIG] +# Written fresh for pgColumnar. + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +sqlstate_as() { + local role="$1" sql="$2" out code + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" \ + -U "$role" -d "$PGC_DB" -Atq -v VERBOSITY=verbose \ + -c "\\set VERBOSITY verbose" -c "$sql" 2>&1)" + code="$(printf '%s\n' "$out" | + sed -n 's/.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\):.*/\1/p' | + head -1)" + if [ -n "$code" ]; then printf '%s\n' "$code"; else printf '00000\n'; fi +} +sqlstate() { sqlstate_as postgres "$1"; } + +psql_run "CREATE TABLE pdc (id int, payload text, sort_key int) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.add_projection( + 'pdc', 'by_sort', ARRAY['id','sort_key'], ARRAY['sort_key']);" +psql_run "INSERT INTO pdc + SELECT g, 'row-' || g, g % 10 FROM generate_series(1,1000) g;" + +# The pre-statement dependency check must not tell a stranger which column the +# projection covers, or let that role take the lock retained by the owner path. +psql_run "CREATE ROLE pdc_nobody LOGIN;" +check "a non-owner learns nothing from a projected column" \ + "$(sqlstate_as pdc_nobody 'ALTER TABLE pdc DROP COLUMN sort_key;')" "42501" +check "the same non-owner error is returned for an unprojected column" \ + "$(sqlstate_as pdc_nobody 'ALTER TABLE pdc DROP COLUMN payload;')" "42501" + +# Red before the fix: PostgreSQL accepts this (00000), and the next INSERT +# reaches the projection writer with a type OID of zero. +check "dropping a projected column is refused as a dependency" \ + "$(sqlstate 'ALTER TABLE pdc DROP COLUMN sort_key;')" "2BP01" + +psql_run "INSERT INTO pdc VALUES (1001, 'still-writable', 1);" +check "the rejected DDL leaves the table writable" \ + "$(q 'SELECT count(*) FROM pdc')" "1001" +check "and the projection still receives the row" \ + "$(q "SELECT count(*) FROM pgcolumnar.read_projection('pdc','by_sort')")" \ + "1001" + +# A column outside every projection remains ordinary DDL. +psql_run "ALTER TABLE pdc DROP COLUMN payload;" +psql_run "INSERT INTO pdc VALUES (1002, 2);" +check "dropping an unrelated column remains allowed" \ + "$(q 'SELECT count(*) FROM pdc')" "1002" + +# A partitioned parent has no storage itself, but DROP COLUMN recurses into its +# columnar partitions. The dependency check must walk the same hierarchy. +psql_run "CREATE TABLE pdc_parent (id int, sort_key int) + PARTITION BY RANGE (id);" +psql_run "CREATE TABLE pdc_child PARTITION OF pdc_parent + FOR VALUES FROM (0) TO (100) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.add_projection( + 'pdc_child', 'child_sort', ARRAY['id','sort_key'], ARRAY['sort_key']);" +psql_run "INSERT INTO pdc_parent VALUES (1, 1);" +check "a parent DROP sees projections on columnar partitions" \ + "$(sqlstate 'ALTER TABLE pdc_parent DROP COLUMN sort_key;')" "2BP01" +psql_run "INSERT INTO pdc_parent VALUES (2, 2);" +check "the rejected parent DDL leaves its partition writable" \ + "$(q 'SELECT count(*) FROM pdc_parent')" "2" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 39ecc7df..eef7b39c 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -240,6 +240,7 @@ SUITES=( phase6 planner_choice_quality preimage_rewrite + projection_drop_column projection_privilege projection_update projections