From edf09b4d7da937d946f5c6ef6ca6806019d37fe0 Mon Sep 17 00:00:00 2001 From: jian he Date: Mon, 15 Sep 2025 15:32:49 +0800 Subject: [PATCH v4 1/3] ALTER TABLE DROP COLUMN drop wholerow referenced object CREATE TABLE ts (a int, c int, b int constraint cc check((ts = ROW(1,1,1)))); CREATE INDEX tsi3 on ts ((ts is null)); CREATE INDEX tsi4 on ts (b) where ts is not null; ALTER TABLE ts DROP COLUMN a CASCADE; will drop above all indexes, constraints on the table ts. now \d ts Table "public.ts" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- c | integer | | | b | integer | | | discussion: https://postgr.es/m/CACJufxGA6KVQy7DbHGLVw9s9KKmpGyZt5ME6C7kEfjDpr2wZCw@mail.gmail.com --- src/backend/commands/tablecmds.c | 170 +++++++++++++++++++++- src/test/regress/expected/constraints.out | 17 +++ src/test/regress/expected/indexing.out | 13 ++ src/test/regress/sql/constraints.sql | 11 ++ src/test/regress/sql/indexing.sql | 8 + 5 files changed, 216 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3be2e051d32..7f5e58da62f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -740,6 +740,7 @@ static List *GetParentedForeignKeyRefs(Relation partition); static void ATDetachCheckNoForeignKeyRefs(Relation partition); static char GetAttributeCompression(Oid atttypid, const char *compression); static char GetAttributeStorage(Oid atttypid, const char *storagemode); +static void recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, bool error_out); /* ---------------------------------------------------------------- @@ -9333,6 +9334,26 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, ReleaseSysCache(tuple); + object.classId = RelationRelationId; + object.objectId = RelationGetRelid(rel); + object.objectSubId = attnum; + + /* + * ALTER TABLE DROP COLUMN must also remove indexes or constraints that + * contain whole-row Var reference expressions. Since there is no direct + * dependency recorded between whole-row Vars and individual columns, and + * creating such dependencies would cause catalog bloat (see + * find_expr_references_walker). + * + * Here we handle this explicitly. We call recordWholeRowDependencyOnOrError + * to establish a dependency between the column and any constraint or index + * involving whole-row Vars. performMultipleDeletions will then take care + * of removing them. + */ + recordWholeRowDependencyOnOrError(rel, &object, false); + + CommandCounterIncrement(); + /* * Propagate to children as appropriate. Unlike most other ALTER * routines, we have to do this one level of recursion at a time; we can't @@ -9426,9 +9447,6 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, } /* Add object to delete */ - object.classId = RelationRelationId; - object.objectId = RelationGetRelid(rel); - object.objectSubId = attnum; add_exact_object_address(&object, addrs); if (!recursing) @@ -22062,3 +22080,149 @@ GetAttributeStorage(Oid atttypid, const char *storagemode) return cstorage; } + +/* + * Record dependencies between whole-row objects (indexes, constraints) + * associated with relation and relation's ObjectAddress. + * error_out means can not install such dependency, we have to error out explicitly. + */ +static void +recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, bool error_out) +{ + Node *expr; + List *indexlist = NIL; + ObjectAddress con_obj; + ObjectAddress idx_obj; + bool find_wholerow = false; + + TupleConstr *constr = RelationGetDescr(rel)->constr; + + if (constr && constr->num_check > 0) + { + ConstrCheck *check = constr->check; + + for (int i = 0; i < constr->num_check; i++) + { + Bitmapset *expr_attrs = NULL; + char *constr_name = check[i].ccname; + + expr = stringToNode(check[i].ccbin); + + /* Find all attributes referenced */ + pull_varattnos(expr, 1, &expr_attrs); + + find_wholerow = bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, + expr_attrs); + if (find_wholerow && !error_out) + { + Relation conDesc; + SysScanDesc conscan; + ScanKeyData skey[3]; + HeapTuple contuple; + + /* Search for a pg_constraint entry with same name and relation */ + conDesc = table_open(ConstraintRelationId, AccessShareLock); + + ScanKeyInit(&skey[0], + Anum_pg_constraint_conrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(rel))); + ScanKeyInit(&skey[1], + Anum_pg_constraint_contypid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(InvalidOid)); + ScanKeyInit(&skey[2], + Anum_pg_constraint_conname, + BTEqualStrategyNumber, F_NAMEEQ, + CStringGetDatum(constr_name)); + + conscan = systable_beginscan(conDesc, ConstraintRelidTypidNameIndexId, true, + NULL, 3, skey); + if (!HeapTupleIsValid(contuple = systable_getnext(conscan))) + elog(ERROR, "constraint \"%s\" of relation \"%s\" does not exist", + constr_name, RelationGetRelationName(rel)); + + con_obj.classId = ConstraintRelationId; + con_obj.objectId = ((Form_pg_constraint) GETSTRUCT(contuple))->oid; + con_obj.objectSubId = 0; + + /* record dependency for constraints that references whole-row */ + recordDependencyOn(&con_obj, object, DEPENDENCY_AUTO); + + systable_endscan(conscan); + table_close(conDesc, AccessShareLock); + } + } + } + + find_wholerow = false; + indexlist = RelationGetIndexList(rel); + foreach_oid(indexoid, indexlist) + { + HeapTuple indexTuple; + Form_pg_index indexStruct; + Node *node; + + indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexoid)); + if (!HeapTupleIsValid(indexTuple)) + elog(ERROR, "cache lookup failed for index %u", indexoid); + indexStruct = (Form_pg_index) GETSTRUCT(indexTuple); + + if (!heap_attisnull(indexTuple, Anum_pg_index_indpred, NULL)) + { + Datum predDatum; + char *predString; + Bitmapset *expr_attrs = NULL; + + /* Convert text string to node tree */ + predDatum = SysCacheGetAttrNotNull(INDEXRELID, indexTuple, + Anum_pg_index_indpred); + predString = TextDatumGetCString(predDatum); + node = (Node *) stringToNode(predString); + pfree(predString); + + pull_varattnos(node, 1, &expr_attrs); + find_wholerow = bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, + expr_attrs); + if (find_wholerow && !error_out) + { + idx_obj.classId = RelationRelationId; + idx_obj.objectId = indexStruct->indexrelid; + idx_obj.objectSubId = 0; + /* record dependency for indexes that references whole-row */ + recordDependencyOn(&idx_obj, object, DEPENDENCY_AUTO); + + ReleaseSysCache(indexTuple); + continue; + } + } + + if (!heap_attisnull(indexTuple, Anum_pg_index_indexprs, NULL)) + { + Datum exprDatum; + char *exprString; + Bitmapset *expr_attrs = NULL; + + /* Convert text string to node tree */ + exprDatum = SysCacheGetAttrNotNull(INDEXRELID, indexTuple, + Anum_pg_index_indexprs); + exprString = TextDatumGetCString(exprDatum); + node = (Node *) stringToNode(exprString); + pfree(exprString); + + pull_varattnos(node, 1, &expr_attrs); + + find_wholerow = bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, + expr_attrs); + if (find_wholerow && !error_out) + { + idx_obj.classId = RelationRelationId; + idx_obj.objectId = indexStruct->indexrelid; + idx_obj.objectSubId = 0; + /* record dependency for indexes that references whole-row */ + recordDependencyOn(&idx_obj, object, DEPENDENCY_AUTO); + } + } + ReleaseSysCache(indexTuple); + } +} diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out index 3590d3274f0..8930b4da7ce 100644 --- a/src/test/regress/expected/constraints.out +++ b/src/test/regress/expected/constraints.out @@ -254,6 +254,23 @@ ERROR: system column "ctid" reference in check constraint is invalid LINE 3: CHECK (NOT (is_capital AND ctid::text = 'sys_col_check... ^ -- +-- Drop column also drop check constraints that have whole-row reference +-- +CREATE TABLE DROP_COL_CHECK_TBL ( + city text, state text, is_capital bool, altitude int, + CONSTRAINT cc CHECK (city is not null), + CONSTRAINT cc1 CHECK (DROP_COL_CHECK_TBL is not null)); +ALTER TABLE DROP_COL_CHECK_TBL DROP COLUMN city; +\d DROP_COL_CHECK_TBL + Table "public.drop_col_check_tbl" + Column | Type | Collation | Nullable | Default +------------+---------+-----------+----------+--------- + state | text | | | + is_capital | boolean | | | + altitude | integer | | | + +DROP TABLE DROP_COL_CHECK_TBL; +-- -- Check inheritance of defaults and constraints -- CREATE TABLE INSERT_CHILD (cx INT default 42, diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index 4d29fb85293..cac1cca3a1f 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -654,6 +654,19 @@ alter table idxpart2 drop column c; b | integer | | | drop table idxpart, idxpart2; +create table idxpart (a int, b int, c int); +create index on idxpart(c); +create index on idxpart((idxpart is not null)); +create index on idxpart(a) where idxpart is not null; +alter table idxpart drop column c; +\d idxpart + Table "public.idxpart" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + +drop table idxpart; -- Verify that expression indexes inherit correctly create table idxpart (a int, b int) partition by range (a); create table idxpart1 (like idxpart); diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql index 1f6dc8fd69f..676d8736101 100644 --- a/src/test/regress/sql/constraints.sql +++ b/src/test/regress/sql/constraints.sql @@ -165,6 +165,17 @@ CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool, altitude int, CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl'))); +-- +-- Drop column also drop check constraints that have whole-row reference +-- +CREATE TABLE DROP_COL_CHECK_TBL ( + city text, state text, is_capital bool, altitude int, + CONSTRAINT cc CHECK (city is not null), + CONSTRAINT cc1 CHECK (DROP_COL_CHECK_TBL is not null)); +ALTER TABLE DROP_COL_CHECK_TBL DROP COLUMN city; +\d DROP_COL_CHECK_TBL +DROP TABLE DROP_COL_CHECK_TBL; + -- -- Check inheritance of defaults and constraints -- diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index b5cb01c2d70..2bad9555112 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -295,6 +295,14 @@ alter table idxpart2 drop column c; \d idxpart2 drop table idxpart, idxpart2; +create table idxpart (a int, b int, c int); +create index on idxpart(c); +create index on idxpart((idxpart is not null)); +create index on idxpart(a) where idxpart is not null; +alter table idxpart drop column c; +\d idxpart +drop table idxpart; + -- Verify that expression indexes inherit correctly create table idxpart (a int, b int) partition by range (a); create table idxpart1 (like idxpart); -- 2.34.1