v17-0002-refactor-Split-ATExecAlterConstraintInternal.patch

From e4789109b40be236b517b5feeabfd2872fc40033 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Tue, 11 Mar 2025 20:41:14 +0530
Subject: [PATCH v17 2/6] 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.
---
 src/backend/commands/tablecmds.c | 244 +++++++++++++++++++------------
 1 file changed, 150 insertions(+), 94 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index df4e239b651..cc462aa831b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -394,14 +394,21 @@ static ObjectAddress ATExecAlterConstraint(List **wqueue, Relation rel,
 										   bool recurse, LOCKMODE lockmode);
 static bool ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel,
 										  Relation tgrel, Relation rel, HeapTuple contuple,
-										  bool recurse, List **otherrelids, LOCKMODE lockmode);
+										  bool recurse, LOCKMODE lockmode);
+static bool ATExecAlterConstrDeferrability(List **wqueue, ATAlterConstraint *cmdcon,
+										   Relation conrel, Relation tgrel, Relation rel,
+										   HeapTuple contuple, bool recurse,
+										   List **otherrelids, LOCKMODE lockmode);
+static bool ATExecAlterConstrInheritability(List **wqueue, ATAlterConstraint *cmdcon,
+											Relation conrel, Relation rel,
+											HeapTuple contuple, LOCKMODE lockmode);
 static void AlterConstrTriggerDeferrability(Oid conoid, Relation tgrel, Relation rel,
 											bool deferrable, bool initdeferred,
 											List **otherrelids);
-static void ATExecAlterChildConstr(List **wqueue, ATAlterConstraint *cmdcon,
-								   Relation conrel, Relation tgrel, Relation rel,
-								   HeapTuple contuple, bool recurse, List **otherrelids,
-								   LOCKMODE lockmode);
+static void AlterConstrDeferrabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon,
+											Relation conrel, Relation tgrel, Relation rel,
+											HeapTuple contuple, bool recurse,
+											List **otherrelids, LOCKMODE lockmode);
 static void UpdateConstraintEntry(ATAlterConstraint *cmdcon, Relation conrel,
 								  HeapTuple contuple);
 static ObjectAddress ATExecValidateConstraint(List **wqueue,
@@ -11927,7 +11934,6 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
 	HeapTuple	contuple;
 	Form_pg_constraint currcon;
 	ObjectAddress address;
-	List	   *otherrelids = NIL;
 
 	/*
 	 * Disallow altering ONLY a partitioned table, as it would make no sense.
@@ -12040,17 +12046,9 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
 	 * Do the actual catalog work, and recurse if necessary.
 	 */
 	if (ATExecAlterConstraintInternal(wqueue, cmdcon, conrel, tgrel, rel,
-									  contuple, recurse, &otherrelids, lockmode))
+									  contuple, recurse, lockmode))
 		ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
 
-	/*
-	 * ATExecAlterConstraintInternal already invalidated relcache for the
-	 * relations having the constraint itself; here we also invalidate for
-	 * relations that have any triggers that are part of the constraint.
-	 */
-	foreach_oid(relid, otherrelids)
-		CacheInvalidateRelcacheByRelid(relid);
-
 	systable_endscan(scan);
 
 	table_close(tgrel, RowExclusiveLock);
@@ -12060,8 +12058,50 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
 }
 
 /*
- * Recursive subroutine of ATExecAlterConstraint.  Returns true if the
- * constraint is altered.
+ * A subroutine of ATExecAlterConstraint that calls the respective routines for
+ * altering constraint attributes.
+ */
+static bool
+ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
+							  Relation conrel, Relation tgrel, Relation rel,
+							  HeapTuple contuple, bool recurse,
+							  LOCKMODE lockmode)
+{
+	bool		changed = false;
+	List	   *otherrelids = NIL;
+
+	/*
+	 * Do the catalog work for the deferrability change, recurse if necessary.
+	 */
+	if (cmdcon->alterDeferrability &&
+		ATExecAlterConstrDeferrability(wqueue, cmdcon, conrel, tgrel, rel,
+									   contuple, recurse, &otherrelids,
+									   lockmode))
+	{
+		/*
+		 * UpdateConstraintEntry already invalidated relcache for the
+		 * relations having the constraint itself; here we also invalidate for
+		 * relations that have any triggers that are part of the constraint.
+		 */
+		foreach_oid(relid, otherrelids)
+			CacheInvalidateRelcacheByRelid(relid);
+
+		changed = true;
+	}
+
+	/*
+	 * Do the catalog work for the inheritability change.
+	 */
+	if (cmdcon->alterInheritability &&
+		ATExecAlterConstrInheritability(wqueue, cmdcon, conrel, rel, contuple,
+										lockmode))
+		changed = true;
+
+	return changed;
+}
+
+/*
+ * Returns true if the constraint's deferrability is altered.
  *
  * *otherrelids is appended OIDs of relations containing affected triggers.
  *
@@ -12071,31 +12111,32 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
  * but existing releases don't do that.)
  */
 static bool
-ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
-							  Relation conrel, Relation tgrel, Relation rel,
-							  HeapTuple contuple, bool recurse,
-							  List **otherrelids, LOCKMODE lockmode)
+ATExecAlterConstrDeferrability(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;
+	Oid			refrelid;
 	bool		changed = false;
 
 	/* since this function recurses, it could be driven to stack overflow */
 	check_stack_depth();
 
+	Assert(cmdcon->alterDeferrability);
+
 	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
-	if (currcon->contype == CONSTRAINT_FOREIGN)
-		refrelid = currcon->confrelid;
+	refrelid = currcon->confrelid;
+
+	/* Should be foreign key constraint */
+	Assert(currcon->contype == CONSTRAINT_FOREIGN);
 
 	/*
-	 * Update pg_constraint with the flags from cmdcon.
-	 *
 	 * If called to modify a constraint that's already in the desired state,
 	 * silently do nothing.
 	 */
-	if (cmdcon->alterDeferrability &&
-		(currcon->condeferrable != cmdcon->deferrable ||
-		 currcon->condeferred != cmdcon->initdeferred))
+	if (currcon->condeferrable != cmdcon->deferrable ||
+		currcon->condeferred != cmdcon->initdeferred)
 	{
 		UpdateConstraintEntry(cmdcon, conrel, contuple);
 		changed = true;
@@ -12113,75 +12154,89 @@ ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
 	 * If the table at either end of the constraint is partitioned, we need to
 	 * handle every constraint that is a child of this one.
 	 */
-	if (recurse && changed &&
+	if (recurse &&
 		(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
-		 (OidIsValid(refrelid) &&
-		  get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)))
-		ATExecAlterChildConstr(wqueue, cmdcon, conrel, tgrel, rel, contuple,
-							   recurse, otherrelids, lockmode);
+		 get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE))
+		AlterConstrDeferrabilityRecurse(wqueue, cmdcon, conrel, tgrel, rel,
+										contuple, recurse, otherrelids,
+										lockmode);
+
+	return changed;
+}
+
+/*
+ * Returns true if the constraint's inheritability is altered.
+ */
+static bool
+ATExecAlterConstrInheritability(List **wqueue, ATAlterConstraint *cmdcon,
+								Relation conrel, Relation rel,
+								HeapTuple contuple, LOCKMODE lockmode)
+{
+	Form_pg_constraint currcon;
+	AttrNumber	colNum;
+	char	   *colName;
+	List	   *children;
+
+	Assert(cmdcon->alterInheritability);
+
+	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
+
+	/* The current implementation only works for NOT NULL constraints */
+	Assert(currcon->contype == CONSTRAINT_NOTNULL);
+
+	/*
+	 * If called to modify a constraint that's already in the desired state,
+	 * silently do nothing.
+	 */
+	if (cmdcon->noinherit == currcon->connoinherit)
+		return false;
+
+	UpdateConstraintEntry(cmdcon, conrel, contuple);
+	CommandCounterIncrement();
+
+	/* Fetch the column number and name */
+	colNum = extractNotNullColumn(contuple);
+	colName = get_attname(currcon->conrelid, colNum, false);
 
 	/*
-	 * Update the catalog for inheritability.  No work if the constraint is
-	 * already in the requested state.
+	 * Propagate the change to children.  For SET NO INHERIT, we don't
+	 * recursively affect children, just the immediate level.
 	 */
-	if (cmdcon->alterInheritability &&
-		(cmdcon->noinherit != currcon->connoinherit))
+	children = find_inheritance_children(RelationGetRelid(rel),
+										 lockmode);
+	foreach_oid(childoid, children)
 	{
-		AttrNumber	colNum;
-		char	   *colName;
-		List	   *children;
+		ObjectAddress addr;
 
-		/* The current implementation only works for NOT NULL constraints */
-		Assert(currcon->contype == CONSTRAINT_NOTNULL);
-
-		UpdateConstraintEntry(cmdcon, conrel, contuple);
-		CommandCounterIncrement();
-		changed = true;
-
-		/* Fetch the column number and name */
-		colNum = extractNotNullColumn(contuple);
-		colName = get_attname(currcon->conrelid, colNum, false);
-
-		/*
-		 * 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)
+		if (cmdcon->noinherit)
 		{
-			ObjectAddress addr;
+			HeapTuple	childtup;
+			Form_pg_constraint childcon;
 
-			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);
+			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);
-			}
+			addr = ATExecSetNotNull(wqueue, childrel, NameStr(currcon->conname),
+									colName, true, true, lockmode);
+			if (OidIsValid(addr.objectId))
+				CommandCounterIncrement();
+			table_close(childrel, NoLock);
 		}
 	}
 
-	return changed;
+	return true;
 }
 
 /*
@@ -12250,7 +12305,7 @@ AlterConstrTriggerDeferrability(Oid conoid, Relation tgrel, Relation rel,
 }
 
 /*
- * Invokes ATExecAlterConstraintInternal for each constraint that is a child of
+ * Invokes ATExecAlterConstrDeferrability for each constraint that is a child of
  * the specified constraint.
  *
  * Note that this doesn't handle recursion the normal way, viz. by scanning the
@@ -12258,13 +12313,13 @@ AlterConstrTriggerDeferrability(Oid conoid, Relation tgrel, Relation rel,
  * relationships.  This may need to be reconsidered.
  *
  * The arguments to this function have the same meaning as the arguments to
- * ATExecAlterConstraintInternal.
+ * ATExecAlterConstrDeferrability.
  */
 static void
-ATExecAlterChildConstr(List **wqueue, ATAlterConstraint *cmdcon,
-					   Relation conrel, Relation tgrel, Relation rel,
-					   HeapTuple contuple, bool recurse, List **otherrelids,
-					   LOCKMODE lockmode)
+AlterConstrDeferrabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon,
+								Relation conrel, Relation tgrel, Relation rel,
+								HeapTuple contuple, bool recurse,
+								List **otherrelids, LOCKMODE lockmode)
 {
 	Form_pg_constraint currcon;
 	Oid			conoid;
@@ -12289,8 +12344,9 @@ ATExecAlterChildConstr(List **wqueue, ATAlterConstraint *cmdcon,
 		Relation	childrel;
 
 		childrel = table_open(childcon->conrelid, lockmode);
-		ATExecAlterConstraintInternal(wqueue, cmdcon, conrel, tgrel, childrel,
-									  childtup, recurse, otherrelids, lockmode);
+
+		ATExecAlterConstrDeferrability(wqueue, cmdcon, conrel, tgrel, childrel,
+									   childtup, recurse, otherrelids, lockmode);
 		table_close(childrel, NoLock);
 	}
 
-- 
2.43.5