On Thu, May 29, 2025 at 12:38 PM jian he <jian.universal...@gmail.com> wrote: > > On Wed, May 28, 2025 at 8:38 PM Tender Wang <tndrw...@gmail.com> wrote: > > > > > > > > Alvaro Herrera <alvhe...@alvh.no-ip.org> 于2025年5月28日周三 20:26写道: > >> > >> On 2025-May-28, Tender Wang wrote: > >> > >> > [...] > The attached *draft* patch is based on your idea. > > The idea is that we only need to conditionally do > ``tab->constraints = lappend(tab->constraints, newcon);`` within > QueueFKConstraintValidation. > but the catalog update needs to be done recursively.
I like this approach, but I don’t think the flag name "recursing" is appropriate, as the flag is meant to indicate whether we want to enqueue constraints for validation or not. Additionally, I noticed that we can skip opening child partitions inside QueueFKConstraintValidation() when the referencing table is not partitioned -- that is, when it is the same as the input rel. Based on that, we can decide whether to queue the validation. However, I am not sure this optimization is worth the added complexity in the code. I have tried this in the attached patch (not the final version), with a brief explanation in the code comments. If we choose to move forward with this patch, I am happy to refine it and add proper tests. Regards, Amul
From b2aaecf0df3c2ea15a84150ef0a91329587e4f20 Mon Sep 17 00:00:00 2001 From: Amul Sul <sulamul@gmail.com> Date: Thu, 29 May 2025 17:22:44 +0530 Subject: [PATCH] POC - FK validation fix --- src/backend/commands/tablecmds.c | 50 +++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 54ad38247aa..f6172618abd 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -431,7 +431,8 @@ static ObjectAddress ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, bool recurse, bool recursing, LOCKMODE lockmode); static void QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, - HeapTuple contuple, LOCKMODE lockmode); + HeapTuple contuple, bool queueValidation, + LOCKMODE lockmode); static void QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relation rel, char *constrName, HeapTuple contuple, bool recurse, bool recursing, LOCKMODE lockmode); @@ -11867,7 +11868,7 @@ AttachPartitionForeignKey(List **wqueue, /* Use the same lock as for AT_ValidateConstraint */ QueueFKConstraintValidation(wqueue, conrel, partition, partcontup, - ShareUpdateExclusiveLock); + true, ShareUpdateExclusiveLock); ReleaseSysCache(partcontup); table_close(conrel, RowExclusiveLock); } @@ -12919,7 +12920,8 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, { if (con->contype == CONSTRAINT_FOREIGN) { - QueueFKConstraintValidation(wqueue, conrel, rel, tuple, lockmode); + QueueFKConstraintValidation(wqueue, conrel, rel, tuple, true, + lockmode); } else if (con->contype == CONSTRAINT_CHECK) { @@ -12953,7 +12955,8 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, */ static void QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, - HeapTuple contuple, LOCKMODE lockmode) + HeapTuple contuple, bool queueValidation, + LOCKMODE lockmode) { Form_pg_constraint con; AlteredTableInfo *tab; @@ -12964,7 +12967,13 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, Assert(con->contype == CONSTRAINT_FOREIGN); Assert(!con->convalidated); - if (rel->rd_rel->relkind == RELKIND_RELATION) + /* + * Sometimes we only want to update the pg_constraint entry without + * enqueueing it for validation; in such cases, queueValidation will be + * false. + */ + if (queueValidation && + rel->rd_rel->relkind == RELKIND_RELATION) { NewConstraint *newcon; Constraint *fkconstraint; @@ -13010,6 +13019,7 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, { Form_pg_constraint childcon; Relation childrel; + bool childValidation = true; childcon = (Form_pg_constraint) GETSTRUCT(childtup); @@ -13021,11 +13031,35 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, if (childcon->convalidated) continue; - childrel = table_open(childcon->conrelid, lockmode); + /* + * When the referenced table is partitioned, the referencing table + * may have multiple foreign key constraints -- one for each child + * table. These individual constraints do not require separate + * validation, as validating the constraint on the root partitioned + * table is sufficient. + * + * In other words, avoid enqueueing the same referencing table for + * validation again. However, we still need to update the + * constraint validation flags, so recursion is necessary. + */ + if (childcon->conrelid == RelationGetRelid(rel)) + { + /* No need to open it again */ + childrel = rel; + childValidation = false; + } + else + childrel = table_open(childcon->conrelid, lockmode); QueueFKConstraintValidation(wqueue, conrel, childrel, childtup, - lockmode); - table_close(childrel, NoLock); + childValidation, lockmode); + + /* + * We skip opening the table if it is the same as the referencing + * table. + */ + if (childcon->conrelid != RelationGetRelid(rel)) + table_close(childrel, NoLock); } systable_endscan(pscan); -- 2.43.5