Skip to content

Commit 639238b

Browse files
peterepetere
committed
refactor: Split ATExecAlterConstraintInternal()
Split ATExecAlterConstraintInternal() into two functions: ATExecAlterConstrDeferrability() and ATExecAlterConstrInheritability(). This simplifies the code and avoids unnecessary confusion caused by recursive code, which isn't needed for ATExecAlterConstrInheritability(). (This also takes over the changes in commit 64224a8, as the new AlterConstrDeferrabilityRecurse() is essentially the old ATExecAlterChildConstr().) Author: Amul Sul <amul.sul@enterprisedb.com> Discussion: https://www.postgresql.org/message-id/flat/CAAJ_b962c5AcYW9KUt_R_ER5qs3fUGbe4az-SP-vuwPS-w-AGA@mail.gmail.com
1 parent a3280e2 commit 639238b

File tree

1 file changed

+148
-91
lines changed

1 file changed

+148
-91
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 148 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -394,14 +394,21 @@ static ObjectAddress ATExecAlterConstraint(List **wqueue, Relation rel,
394394
bool recurse, LOCKMODE lockmode);
395395
static bool ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel,
396396
Relation tgrel, Relation rel, HeapTuple contuple,
397-
bool recurse, List **otherrelids, LOCKMODE lockmode);
397+
bool recurse, LOCKMODE lockmode);
398+
static bool ATExecAlterConstrDeferrability(List **wqueue, ATAlterConstraint *cmdcon,
399+
Relation conrel, Relation tgrel, Relation rel,
400+
HeapTuple contuple, bool recurse,
401+
List **otherrelids, LOCKMODE lockmode);
402+
static bool ATExecAlterConstrInheritability(List **wqueue, ATAlterConstraint *cmdcon,
403+
Relation conrel, Relation rel,
404+
HeapTuple contuple, LOCKMODE lockmode);
398405
static void AlterConstrTriggerDeferrability(Oid conoid, Relation tgrel, Relation rel,
399406
bool deferrable, bool initdeferred,
400407
List **otherrelids);
401-
static void ATExecAlterChildConstr(List **wqueue, ATAlterConstraint *cmdcon,
402-
Relation conrel, Relation tgrel, Relation rel,
403-
HeapTuple contuple, bool recurse, List **otherrelids,
404-
LOCKMODE lockmode);
408+
static void AlterConstrDeferrabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon,
409+
Relation conrel, Relation tgrel, Relation rel,
410+
HeapTuple contuple, bool recurse,
411+
List **otherrelids, LOCKMODE lockmode);
405412
static void AlterConstrUpdateConstraintEntry(ATAlterConstraint *cmdcon, Relation conrel,
406413
HeapTuple contuple);
407414
static ObjectAddress ATExecValidateConstraint(List **wqueue,
@@ -11925,7 +11932,6 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
1192511932
HeapTuple contuple;
1192611933
Form_pg_constraint currcon;
1192711934
ObjectAddress address;
11928-
List *otherrelids = NIL;
1192911935

1193011936
/*
1193111937
* Disallow altering ONLY a partitioned table, as it would make no sense.
@@ -12038,17 +12044,9 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
1203812044
* Do the actual catalog work, and recurse if necessary.
1203912045
*/
1204012046
if (ATExecAlterConstraintInternal(wqueue, cmdcon, conrel, tgrel, rel,
12041-
contuple, recurse, &otherrelids, lockmode))
12047+
contuple, recurse, lockmode))
1204212048
ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
1204312049

12044-
/*
12045-
* ATExecAlterConstraintInternal already invalidated relcache for the
12046-
* relations having the constraint itself; here we also invalidate for
12047-
* relations that have any triggers that are part of the constraint.
12048-
*/
12049-
foreach_oid(relid, otherrelids)
12050-
CacheInvalidateRelcacheByRelid(relid);
12051-
1205212050
systable_endscan(scan);
1205312051

1205412052
table_close(tgrel, RowExclusiveLock);
@@ -12058,8 +12056,51 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
1205812056
}
1205912057

1206012058
/*
12061-
* Recursive subroutine of ATExecAlterConstraint. Returns true if the
12062-
* constraint is altered.
12059+
* A subroutine of ATExecAlterConstraint that calls the respective routines for
12060+
* altering constraint attributes.
12061+
*/
12062+
static bool
12063+
ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
12064+
Relation conrel, Relation tgrel, Relation rel,
12065+
HeapTuple contuple, bool recurse,
12066+
LOCKMODE lockmode)
12067+
{
12068+
bool changed = false;
12069+
List *otherrelids = NIL;
12070+
12071+
/*
12072+
* Do the catalog work for the deferrability change, recurse if necessary.
12073+
*/
12074+
if (cmdcon->alterDeferrability &&
12075+
ATExecAlterConstrDeferrability(wqueue, cmdcon, conrel, tgrel, rel,
12076+
contuple, recurse, &otherrelids,
12077+
lockmode))
12078+
{
12079+
/*
12080+
* AlterConstrUpdateConstraintEntry already invalidated relcache for
12081+
* the relations having the constraint itself; here we also invalidate
12082+
* for relations that have any triggers that are part of the
12083+
* constraint.
12084+
*/
12085+
foreach_oid(relid, otherrelids)
12086+
CacheInvalidateRelcacheByRelid(relid);
12087+
12088+
changed = true;
12089+
}
12090+
12091+
/*
12092+
* Do the catalog work for the inheritability change.
12093+
*/
12094+
if (cmdcon->alterInheritability &&
12095+
ATExecAlterConstrInheritability(wqueue, cmdcon, conrel, rel, contuple,
12096+
lockmode))
12097+
changed = true;
12098+
12099+
return changed;
12100+
}
12101+
12102+
/*
12103+
* Returns true if the constraint's deferrability is altered.
1206312104
*
1206412105
* *otherrelids is appended OIDs of relations containing affected triggers.
1206512106
*
@@ -12069,31 +12110,32 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
1206912110
* but existing releases don't do that.)
1207012111
*/
1207112112
static bool
12072-
ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
12073-
Relation conrel, Relation tgrel, Relation rel,
12074-
HeapTuple contuple, bool recurse,
12075-
List **otherrelids, LOCKMODE lockmode)
12113+
ATExecAlterConstrDeferrability(List **wqueue, ATAlterConstraint *cmdcon,
12114+
Relation conrel, Relation tgrel, Relation rel,
12115+
HeapTuple contuple, bool recurse,
12116+
List **otherrelids, LOCKMODE lockmode)
1207612117
{
1207712118
Form_pg_constraint currcon;
12078-
Oid refrelid = InvalidOid;
12119+
Oid refrelid;
1207912120
bool changed = false;
1208012121

1208112122
/* since this function recurses, it could be driven to stack overflow */
1208212123
check_stack_depth();
1208312124

12125+
Assert(cmdcon->alterDeferrability);
12126+
1208412127
currcon = (Form_pg_constraint) GETSTRUCT(contuple);
12085-
if (currcon->contype == CONSTRAINT_FOREIGN)
12086-
refrelid = currcon->confrelid;
12128+
refrelid = currcon->confrelid;
12129+
12130+
/* Should be foreign key constraint */
12131+
Assert(currcon->contype == CONSTRAINT_FOREIGN);
1208712132

1208812133
/*
12089-
* Update pg_constraint with the flags from cmdcon.
12090-
*
1209112134
* If called to modify a constraint that's already in the desired state,
1209212135
* silently do nothing.
1209312136
*/
12094-
if (cmdcon->alterDeferrability &&
12095-
(currcon->condeferrable != cmdcon->deferrable ||
12096-
currcon->condeferred != cmdcon->initdeferred))
12137+
if (currcon->condeferrable != cmdcon->deferrable ||
12138+
currcon->condeferred != cmdcon->initdeferred)
1209712139
{
1209812140
AlterConstrUpdateConstraintEntry(cmdcon, conrel, contuple);
1209912141
changed = true;
@@ -12113,73 +12155,87 @@ ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
1211312155
*/
1211412156
if (recurse && changed &&
1211512157
(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
12116-
(OidIsValid(refrelid) &&
12117-
get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)))
12118-
ATExecAlterChildConstr(wqueue, cmdcon, conrel, tgrel, rel, contuple,
12119-
recurse, otherrelids, lockmode);
12158+
get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE))
12159+
AlterConstrDeferrabilityRecurse(wqueue, cmdcon, conrel, tgrel, rel,
12160+
contuple, recurse, otherrelids,
12161+
lockmode);
12162+
12163+
return changed;
12164+
}
12165+
12166+
/*
12167+
* Returns true if the constraint's inheritability is altered.
12168+
*/
12169+
static bool
12170+
ATExecAlterConstrInheritability(List **wqueue, ATAlterConstraint *cmdcon,
12171+
Relation conrel, Relation rel,
12172+
HeapTuple contuple, LOCKMODE lockmode)
12173+
{
12174+
Form_pg_constraint currcon;
12175+
AttrNumber colNum;
12176+
char *colName;
12177+
List *children;
12178+
12179+
Assert(cmdcon->alterInheritability);
12180+
12181+
currcon = (Form_pg_constraint) GETSTRUCT(contuple);
12182+
12183+
/* The current implementation only works for NOT NULL constraints */
12184+
Assert(currcon->contype == CONSTRAINT_NOTNULL);
1212012185

1212112186
/*
12122-
* Update the catalog for inheritability. No work if the constraint is
12123-
* already in the requested state.
12187+
* If called to modify a constraint that's already in the desired state,
12188+
* silently do nothing.
1212412189
*/
12125-
if (cmdcon->alterInheritability &&
12126-
(cmdcon->noinherit != currcon->connoinherit))
12127-
{
12128-
AttrNumber colNum;
12129-
char *colName;
12130-
List *children;
12190+
if (cmdcon->noinherit == currcon->connoinherit)
12191+
return false;
1213112192

12132-
/* The current implementation only works for NOT NULL constraints */
12133-
Assert(currcon->contype == CONSTRAINT_NOTNULL);
12193+
AlterConstrUpdateConstraintEntry(cmdcon, conrel, contuple);
12194+
CommandCounterIncrement();
1213412195

12135-
AlterConstrUpdateConstraintEntry(cmdcon, conrel, contuple);
12136-
CommandCounterIncrement();
12137-
changed = true;
12196+
/* Fetch the column number and name */
12197+
colNum = extractNotNullColumn(contuple);
12198+
colName = get_attname(currcon->conrelid, colNum, false);
1213812199

12139-
/* Fetch the column number and name */
12140-
colNum = extractNotNullColumn(contuple);
12141-
colName = get_attname(currcon->conrelid, colNum, false);
12200+
/*
12201+
* Propagate the change to children. For SET NO INHERIT, we don't
12202+
* recursively affect children, just the immediate level.
12203+
*/
12204+
children = find_inheritance_children(RelationGetRelid(rel),
12205+
lockmode);
12206+
foreach_oid(childoid, children)
12207+
{
12208+
ObjectAddress addr;
1214212209

12143-
/*
12144-
* Propagate the change to children. For SET NO INHERIT, we don't
12145-
* recursively affect children, just the immediate level.
12146-
*/
12147-
children = find_inheritance_children(RelationGetRelid(rel),
12148-
lockmode);
12149-
foreach_oid(childoid, children)
12210+
if (cmdcon->noinherit)
1215012211
{
12151-
ObjectAddress addr;
12212+
HeapTuple childtup;
12213+
Form_pg_constraint childcon;
1215212214

12153-
if (cmdcon->noinherit)
12154-
{
12155-
HeapTuple childtup;
12156-
Form_pg_constraint childcon;
12157-
12158-
childtup = findNotNullConstraint(childoid, colName);
12159-
if (!childtup)
12160-
elog(ERROR, "cache lookup failed for not-null constraint on column \"%s\" of relation %u",
12161-
colName, childoid);
12162-
childcon = (Form_pg_constraint) GETSTRUCT(childtup);
12163-
Assert(childcon->coninhcount > 0);
12164-
childcon->coninhcount--;
12165-
childcon->conislocal = true;
12166-
CatalogTupleUpdate(conrel, &childtup->t_self, childtup);
12167-
heap_freetuple(childtup);
12168-
}
12169-
else
12170-
{
12171-
Relation childrel = table_open(childoid, NoLock);
12215+
childtup = findNotNullConstraint(childoid, colName);
12216+
if (!childtup)
12217+
elog(ERROR, "cache lookup failed for not-null constraint on column \"%s\" of relation %u",
12218+
colName, childoid);
12219+
childcon = (Form_pg_constraint) GETSTRUCT(childtup);
12220+
Assert(childcon->coninhcount > 0);
12221+
childcon->coninhcount--;
12222+
childcon->conislocal = true;
12223+
CatalogTupleUpdate(conrel, &childtup->t_self, childtup);
12224+
heap_freetuple(childtup);
12225+
}
12226+
else
12227+
{
12228+
Relation childrel = table_open(childoid, NoLock);
1217212229

12173-
addr = ATExecSetNotNull(wqueue, childrel, NameStr(currcon->conname),
12174-
colName, true, true, lockmode);
12175-
if (OidIsValid(addr.objectId))
12176-
CommandCounterIncrement();
12177-
table_close(childrel, NoLock);
12178-
}
12230+
addr = ATExecSetNotNull(wqueue, childrel, NameStr(currcon->conname),
12231+
colName, true, true, lockmode);
12232+
if (OidIsValid(addr.objectId))
12233+
CommandCounterIncrement();
12234+
table_close(childrel, NoLock);
1217912235
}
1218012236
}
1218112237

12182-
return changed;
12238+
return true;
1218312239
}
1218412240

1218512241
/*
@@ -12248,21 +12304,21 @@ AlterConstrTriggerDeferrability(Oid conoid, Relation tgrel, Relation rel,
1224812304
}
1224912305

1225012306
/*
12251-
* Invokes ATExecAlterConstraintInternal for each constraint that is a child of
12307+
* Invokes ATExecAlterConstrDeferrability for each constraint that is a child of
1225212308
* the specified constraint.
1225312309
*
1225412310
* Note that this doesn't handle recursion the normal way, viz. by scanning the
1225512311
* list of child relations and recursing; instead it uses the conparentid
1225612312
* relationships. This may need to be reconsidered.
1225712313
*
1225812314
* The arguments to this function have the same meaning as the arguments to
12259-
* ATExecAlterConstraintInternal.
12315+
* ATExecAlterConstrDeferrability.
1226012316
*/
1226112317
static void
12262-
ATExecAlterChildConstr(List **wqueue, ATAlterConstraint *cmdcon,
12263-
Relation conrel, Relation tgrel, Relation rel,
12264-
HeapTuple contuple, bool recurse, List **otherrelids,
12265-
LOCKMODE lockmode)
12318+
AlterConstrDeferrabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon,
12319+
Relation conrel, Relation tgrel, Relation rel,
12320+
HeapTuple contuple, bool recurse,
12321+
List **otherrelids, LOCKMODE lockmode)
1226612322
{
1226712323
Form_pg_constraint currcon;
1226812324
Oid conoid;
@@ -12287,8 +12343,9 @@ ATExecAlterChildConstr(List **wqueue, ATAlterConstraint *cmdcon,
1228712343
Relation childrel;
1228812344

1228912345
childrel = table_open(childcon->conrelid, lockmode);
12290-
ATExecAlterConstraintInternal(wqueue, cmdcon, conrel, tgrel, childrel,
12291-
childtup, recurse, otherrelids, lockmode);
12346+
12347+
ATExecAlterConstrDeferrability(wqueue, cmdcon, conrel, tgrel, childrel,
12348+
childtup, recurse, otherrelids, lockmode);
1229212349
table_close(childrel, NoLock);
1229312350
}
1229412351

0 commit comments

Comments
 (0)