handling multiple matching constraints in DetachPartitionFinalize()

Started by Zhihong Yuover 3 years ago5 messages
#1Zhihong Yu
zyu@yugabyte.com

Hi,
I was looking at the following code in DetachPartitionFinalize():

/* If there's a constraint associated with the index, detach it too
*/
constrOid =
get_relation_idx_constraint_oid(RelationGetRelid(partRel),
idxid);

As mentioned in email thread `parenting a PK constraint to a self-FK one`,
there may be multiple matching constraints, I think we should
call ConstraintSetParentConstraint() for each of them.

This means adding a helper method similar to
get_relation_idx_constraint_oid() which finds constraint and calls
ConstraintSetParentConstraint().

I am preparing a patch.
Please let me know if my proposal makes sense.

Thanks

#2Zhihong Yu
zyu@yugabyte.com
In reply to: Zhihong Yu (#1)
1 attachment(s)
Re: handling multiple matching constraints in DetachPartitionFinalize()

On Tue, Aug 23, 2022 at 10:10 AM Zhihong Yu <zyu@yugabyte.com> wrote:

Hi,
I was looking at the following code in DetachPartitionFinalize():

/* If there's a constraint associated with the index, detach it
too */
constrOid =
get_relation_idx_constraint_oid(RelationGetRelid(partRel),
idxid);

As mentioned in email thread `parenting a PK constraint to a self-FK one`,
there may be multiple matching constraints, I think we should
call ConstraintSetParentConstraint() for each of them.

This means adding a helper method similar to
get_relation_idx_constraint_oid() which finds constraint and calls
ConstraintSetParentConstraint().

I am preparing a patch.
Please let me know if my proposal makes sense.

Thanks

This is what I came up with.

Attachments:

detach-constraints.patchapplication/octet-stream; name=detach-constraints.patchDownload
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index bb65fb1e0a..bd7aba63c7 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -984,6 +984,46 @@ get_relation_constraint_attnos(Oid relid, const char *conname,
 	return conattnos;
 }
 
+/*
+ * Detaches the constraint(s) associated with the given index in the
+ * given relation.
+ * Returns the number of detached constraints or 0 if no such index is catalogued.
+ */
+int
+detach_relation_idx_constraints(Oid relationId, Oid indexId)
+{
+	Relation	pg_constraint;
+	SysScanDesc scan;
+	ScanKeyData key;
+	HeapTuple	tuple;
+	int			count = 0;
+
+	pg_constraint = table_open(ConstraintRelationId, AccessShareLock);
+
+	ScanKeyInit(&key,
+				Anum_pg_constraint_conrelid,
+				BTEqualStrategyNumber,
+				F_OIDEQ,
+				ObjectIdGetDatum(relationId));
+	scan = systable_beginscan(pg_constraint, ConstraintRelidTypidNameIndexId,
+							  true, NULL, 1, &key);
+	while ((tuple = systable_getnext(scan)) != NULL)
+	{
+		Form_pg_constraint constrForm;
+
+		constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
+		if (constrForm->conindid == indexId)
+		{
+			ConstraintSetParentConstraint(constrForm->oid, InvalidOid, InvalidOid);
+			count++;
+		}
+	}
+	systable_endscan(scan);
+
+	table_close(pg_constraint, AccessShareLock);
+	return count;
+}
+
 /*
  * Return the OID of the constraint associated with the given index in the
  * given relation; or InvalidOid if no such index is catalogued.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9be04c8a1e..4ecf74f851 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18578,7 +18578,6 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 	{
 		Oid			idxid = lfirst_oid(cell);
 		Relation	idx;
-		Oid			constrOid;
 
 		if (!has_superclass(idxid))
 			continue;
@@ -18590,10 +18589,7 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 		IndexSetParentIndex(idx, InvalidOid);
 
 		/* If there's a constraint associated with the index, detach it too */
-		constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel),
-													idxid);
-		if (OidIsValid(constrOid))
-			ConstraintSetParentConstraint(constrOid, InvalidOid, InvalidOid);
+		detach_relation_idx_constraints(RelationGetRelid(partRel), idxid);
 
 		index_close(idx, NoLock);
 	}
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index e7d967f137..a1933c5c53 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -257,6 +257,7 @@ extern Bitmapset *get_relation_constraint_attnos(Oid relid, const char *conname,
 												 bool missing_ok, Oid *constraintOid);
 extern Oid	get_domain_constraint_oid(Oid typid, const char *conname, bool missing_ok);
 extern Oid	get_relation_idx_constraint_oid(Oid relationId, Oid indexId);
+extern int	detach_relation_idx_constraints(Oid relationId, Oid indexId);
 
 extern Bitmapset *get_primary_key_attnos(Oid relid, bool deferrableOk,
 										 Oid *constraintOid);
#3Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Zhihong Yu (#2)
Re: handling multiple matching constraints in DetachPartitionFinalize()

On 2022-Aug-23, Zhihong Yu wrote:

This is what I came up with.

I suggest you provide a set of SQL commands that provoke some wrong
behavior with the original code, and show that they generate good
behavior after the patch. Otherwise, it's hard to evaluate the
usefulness of this.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

#4Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Alvaro Herrera (#3)
Re: handling multiple matching constraints in DetachPartitionFinalize()

On 2022-Aug-23, Zhihong Yu wrote:

Toggling enable_seqscan on / off using the example from `parenting a PK
constraint to a self-FK one` thread, it can be shown that different
constraint Id would be detached which is incorrect.
However, I am not sure whether toggling enable_seqscan mid-test is
legitimate.

Well, let's see it in action.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

#5Zhihong Yu
zyu@yugabyte.com
In reply to: Alvaro Herrera (#3)
Re: handling multiple matching constraints in DetachPartitionFinalize()

On Tue, Aug 23, 2022 at 10:53 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2022-Aug-23, Zhihong Yu wrote:

This is what I came up with.

I suggest you provide a set of SQL commands that provoke some wrong
behavior with the original code, and show that they generate good
behavior after the patch. Otherwise, it's hard to evaluate the
usefulness of this.

--
Álvaro Herrera Breisgau, Deutschland —
https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

Toggling enable_seqscan on / off using the example from `parenting a PK
constraint to a self-FK one` thread, it can be shown that different
constraint Id would be detached which is incorrect.
However, I am not sure whether toggling enable_seqscan mid-test is
legitimate.

Cheers