From 3113c30f1e09f084f9f33b4006bed145c52c8573 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= Date: Fri, 28 Feb 2025 22:45:59 +0100 Subject: [PATCH] Alvaro's review --- doc/src/sgml/ref/alter_table.sgml | 2 +- src/backend/commands/tablecmds.c | 199 +++++++++--------------------- src/backend/parser/gram.y | 4 +- src/include/nodes/parsenodes.h | 2 +- 4 files changed, 60 insertions(+), 147 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index fd02c3ca370..a397cdc0792 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -574,7 +574,7 @@ WITH ( MODULUS numeric_literal, REM In addition to changing the inheritability status of the constraint, in the case where a non-inheritable constraint is being marked inheritable, if the table has children, an equivalent constraint - is added to them. If marking an inheritable constraint as + will be added to them. If marking an inheritable constraint as non-inheritable on a table with children, then the corresponding constraint on children will be marked as no longer inherited, but not removed. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3b78b980d47..0e3a9d47720 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -392,15 +392,12 @@ static void AlterSeqNamespaces(Relation classRel, Relation rel, static ObjectAddress ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon, bool recurse, LOCKMODE lockmode); -static bool ATExecAlterConstraintInternal(ATAlterConstraint *cmdcon, Relation conrel, +static bool ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Relation tgrel, Relation rel, HeapTuple contuple, - bool recurse, List **otherrelids, LOCKMODE lockmode, - List **wqueue); + bool recurse, List **otherrelids, LOCKMODE lockmode); static void AlterConstrTriggerDeferrability(Oid conoid, Relation tgrel, Relation rel, bool deferrable, bool initdeferred, List **otherrelids); -static ObjectAddress ATExecSetNotNullNoInherit(Relation rel, char *conName, - char *colName, LOCKMODE lockmode); static ObjectAddress ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, bool recurse, bool recursing, LOCKMODE lockmode); @@ -11863,13 +11860,21 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint", cmdcon->conname, RelationGetRelationName(rel)))); - else if (cmdcon->alterinheritability && - currcon->contype != CONSTRAINT_NOTNULL) + if (cmdcon->alterInheritability && + currcon->contype != CONSTRAINT_NOTNULL) ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("ALTER TABLE \"%s\" ALTER CONSTRAINT \"%s\" SET %s only supports not null constraint", - RelationGetRelationName(rel), cmdcon->conname, - cmdcon->noinherit ? "NO INHERIT" : "INHERIT"))); + errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("constraint \"%s\" of relation \"%s\" is not a not-null constraint", + RelationGetRelationName(rel), cmdcon->conname)); + + /* Refuse to modify inheritability of inherited constraints */ + if (cmdcon->alterInheritability && + cmdcon->noinherit && currcon->coninhcount > 0) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot alter inherited constraint \"%s\" on relation \"%s\"", + NameStr(currcon->conname), + RelationGetRelationName(rel))); /* * If it's not the topmost constraint, raise an error. @@ -11920,8 +11925,8 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon, /* * Do the actual catalog work, and recurse if necessary. */ - if (ATExecAlterConstraintInternal(cmdcon, conrel, tgrel, rel, contuple, - recurse, &otherrelids, lockmode, wqueue)) + if (ATExecAlterConstraintInternal(wqueue, cmdcon, conrel, tgrel, rel, + contuple, recurse, &otherrelids, lockmode)) ObjectAddressSet(address, ConstraintRelationId, currcon->oid); /* @@ -11952,10 +11957,10 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon, * but existing releases don't do that.) */ static bool -ATExecAlterConstraintInternal(ATAlterConstraint *cmdcon, Relation conrel, - Relation tgrel, Relation rel, HeapTuple contuple, - bool recurse, List **otherrelids, LOCKMODE lockmode, - List **wqueue) +ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon, + Relation conrel, Relation tgrel, Relation rel, + HeapTuple contuple, bool recurse, + List **otherrelids, LOCKMODE lockmode) { Form_pg_constraint currcon; Oid refrelid = InvalidOid; @@ -12035,16 +12040,20 @@ ATExecAlterConstraintInternal(ATAlterConstraint *cmdcon, Relation conrel, Relation childrel; childrel = table_open(childcon->conrelid, lockmode); - ATExecAlterConstraintInternal(cmdcon, conrel, tgrel, childrel, childtup, - recurse, otherrelids, lockmode, wqueue); + ATExecAlterConstraintInternal(wqueue, cmdcon, conrel, tgrel, childrel, + childtup, recurse, otherrelids, lockmode); table_close(childrel, NoLock); } systable_endscan(pscan); } - /* Update the catalog for inheritability */ - if (cmdcon->alterinheritability) + /* + * Update the catalog for inheritability. No work if the constraint is + * already in the requested state. + */ + if (cmdcon->alterInheritability && + (cmdcon->noinherit != currcon->connoinherit)) { AttrNumber colNum; char *colName; @@ -12052,59 +12061,60 @@ ATExecAlterConstraintInternal(ATAlterConstraint *cmdcon, Relation conrel, HeapTuple copyTuple; Form_pg_constraint copy_con; - /* Return false if constraint doesn't need updation. */ - if ((cmdcon->noinherit && currcon->connoinherit) || - (!cmdcon->noinherit && !currcon->connoinherit)) - return false; + /* The current implementation only works for NOT NULL constraints */ + Assert(currcon->contype == CONSTRAINT_NOTNULL); copyTuple = heap_copytuple(contuple); copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple); - - if (cmdcon->noinherit) - { - /* Update the constraint tuple and mark connoinherit as true. */ - copy_con->connoinherit = true; - } - else - { - /* Update the constraint tuple and mark connoinherit as false. */ - copy_con->connoinherit = false; - } + copy_con->connoinherit = cmdcon->noinherit; CatalogTupleUpdate(conrel, ©Tuple->t_self, copyTuple); CommandCounterIncrement(); heap_freetuple(copyTuple); + changed = true; /* Fetch the column number and name */ colNum = extractNotNullColumn(contuple); colName = get_attname(currcon->conrelid, colNum, false); /* - * Recurse to propagate the constraint to children that don't have one. + * Propagate the change to children. For SET NO INHERIT, we don't + * recursively affect children, just the immediate level. */ children = find_inheritance_children(RelationGetRelid(rel), lockmode); - foreach_oid(childoid, children) { - Relation childrel = table_open(childoid, NoLock); ObjectAddress addr; - if (!cmdcon->noinherit) + if (cmdcon->noinherit) { + HeapTuple childtup; + Form_pg_constraint childcon; + + childtup = findNotNullConstraint(childoid, colName); + if (!childtup) + elog(ERROR, "cache lookup failed for not-null constraint on column \"%s\" of relation %u", + colName, childoid); + childcon = (Form_pg_constraint) GETSTRUCT(childtup); + Assert(childcon->coninhcount > 0); + childcon->coninhcount--; + childcon->conislocal = true; + CatalogTupleUpdate(conrel, &childtup->t_self, childtup); + heap_freetuple(childtup); + } + else + { + Relation childrel = table_open(childoid, NoLock); + addr = ATExecSetNotNull(wqueue, childrel, NameStr(currcon->conname), colName, true, true, lockmode); if (OidIsValid(addr.objectId)) CommandCounterIncrement(); + table_close(childrel, NoLock); } - else if (cmdcon->noinherit) - ATExecSetNotNullNoInherit(childrel, NameStr(currcon->conname), - colName, lockmode); - table_close(childrel, NoLock); } - - return false; } return changed; @@ -12175,103 +12185,6 @@ AlterConstrTriggerDeferrability(Oid conoid, Relation tgrel, Relation rel, systable_endscan(tgscan); } -/* - * Find out the not null constraint from provided relation and decrement the - * coninhcount count if constraint exists since the parent constraint marked as - * no inherit. - * - * We must recurse to child tables during execution. - */ -static ObjectAddress -ATExecSetNotNullNoInherit(Relation rel, char *conName, - char *colName, LOCKMODE lockmode) -{ - - HeapTuple tuple; - AttrNumber attnum; - ObjectAddress address; - List *children; - - /* Guard against stack overflow due to overly deep inheritance tree. */ - check_stack_depth(); - - ATSimplePermissions(AT_AddConstraint, rel, - ATT_PARTITIONED_TABLE | ATT_TABLE | ATT_FOREIGN_TABLE); - - attnum = get_attnum(RelationGetRelid(rel), colName); - if (attnum == InvalidAttrNumber) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_COLUMN), - errmsg("column \"%s\" of relation \"%s\" does not exist", - colName, RelationGetRelationName(rel)))); - - /* Prevent them from altering a system attribute */ - if (attnum <= 0) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot alter system column \"%s\"", - colName))); - - /* See if there's already a constraint */ - tuple = findNotNullConstraintAttnum(RelationGetRelid(rel), attnum); - if (HeapTupleIsValid(tuple)) - { - Form_pg_constraint conForm = (Form_pg_constraint) GETSTRUCT(tuple); - bool changed = false; - - /* Decrement the constraint inheritance count */ - if (conForm->coninhcount > 0) - { - conForm->coninhcount--; - changed = true; - } - /* Mark the constraint as local defined once coninhcount = 0 */ - if (conForm->coninhcount == 0) - { - conForm->conislocal = true; - changed = true; - } - - if (changed) - { - Relation constr_rel; - - constr_rel = table_open(ConstraintRelationId, RowExclusiveLock); - - CatalogTupleUpdate(constr_rel, &tuple->t_self, tuple); - ObjectAddressSet(address, ConstraintRelationId, conForm->oid); - table_close(constr_rel, RowExclusiveLock); - - /* Make update visible */ - CommandCounterIncrement(); - } - } - else - elog(ERROR, "cache lookup failed for not-null constraint on column \"%s\" of relation %u", - colName, RelationGetRelid(rel)); - - if (tuple) - heap_freetuple(tuple); - - InvokeObjectPostAlterHook(RelationRelationId, - RelationGetRelid(rel), attnum); - - /* - * Recurse to child tables. - */ - children = find_inheritance_children(RelationGetRelid(rel), lockmode); - - foreach_oid(childoid, children) - { - Relation childrel = table_open(childoid, NoLock); - - ATExecSetNotNullNoInherit(childrel, conName, colName, lockmode); - table_close(childrel, NoLock); - } - - return address; -} - /* * ALTER TABLE VALIDATE CONSTRAINT * diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 1509cf61552..b99601e3788 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -2678,7 +2678,7 @@ alter_table_cmd: n->subtype = AT_AlterConstraint; n->def = (Node *) c; c->conname = $3; - c->alterinheritability = true; + c->alterInheritability = true; c->noinherit = false; $$ = (Node *) n; @@ -2692,7 +2692,7 @@ alter_table_cmd: n->subtype = AT_AlterConstraint; n->def = (Node *) c; c->conname = $3; - c->alterinheritability = true; + c->alterInheritability = true; c->noinherit = true; $$ = (Node *) n; diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 1147b573a37..23c9e3c5abf 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2493,7 +2493,7 @@ typedef struct ATAlterConstraint bool alterDeferrability; /* changing deferrability properties? */ bool deferrable; /* DEFERRABLE? */ bool initdeferred; /* INITIALLY DEFERRED? */ - bool alterinheritability; /* changing inheritability properties */ + bool alterInheritability; /* changing inheritability properties */ bool noinherit; } ATAlterConstraint; -- 2.39.5