On 2025-Feb-21, Suraj Kharage wrote:
> Thanks, Alvaro.
>
> I have revised the patch as per your last update.
> Please find attached v5 for further review.
Hello
I noticed two issues. One is that we are OK to modify a constraint
that's defined in our parent, which breaks everything. We can only
allow a top-level constraint to be modified. I added a check in
ATExecAlterConstraint() for this. Please add a test case for this.
The other one is that when we set a constraint to NO INHERIT on a table
with children and grandchildren, we must only modify the directly
inheriting constraints as not having a parent -- we must not recurse to
also do that in the grandchildren! Otherwise they become disconnected,
which makes no sense. So we just want to locate the constraint for each
child, modify by subtracting 1 from coninhcount and set islocal, and
we're done. The whole ATExecSetNotNullNoInherit() function is based on
the wrong premise that this requires to recurse. I chose to remove it
to keep things simple.
Stylistically, in tablecmds.c we always have the 'List **wqueue'
argument as the first one in functions that take it. So when adding it
to a function that doesn't have it, don't put it last.
This error message:
- errmsg("ALTER TABLE \"%s\" ALTER CONSTRAINT \"%s\" SET %s only
supports not null constraint",
- RelationGetRelationName(rel), cmdcon->conname,
- cmdcon->noinherit ? "NO INHERIT" : "INHERIT")));
seems a bit excessive. Looking at other examples, it doesn't look like
we need to cite the complete message in so much detail (especially if,
say, the user specified a schema-qualified table name in the command
which won't show up in the error message, this would look just weird).
I simplified it.
Please verify that the tests are still working correctly and resubmit.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)
>From 3113c30f1e09f084f9f33b4006bed145c52c8573 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@alvh.no-ip.org>
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 <replaceable
class="parameter">numeric_literal</replaceable>, 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
