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
