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:
>>
>> > I dided the codes, in QueueFKConstraintValidation(), we add three
>> > newconstraint for the
>> > fk rel, because the pk rel is partition table.
>> >
>> > During phase 3 of AlterTable, in ATRewriteTables(),
>> > call validateForeignKeyConstraint() three times.
>> > The first time the pk rel is pk, and it's ok.
>> > The second time the pk rel is only pk_1, and the type(1) is not in pk_1, so
>> > an error is reported.
>> >
>> > In this case, the two children newconstraint should not be added to the
>> > queue.
>>
>> Yeah, I reached the same conclusion and this is the preliminary fix I
>> had written for it. I don't like that I had to duplicate a few lines of
>> code, but maybe it's not too bad. Also the comments need to be
>> clarified a bit more.
>
>
> If the child table is still a partitioned table, the patch seems not work.
>
> I figure out a quick fix as the attached. I add a bool argument into the
> QueueFKConstraintValidation().
> If it is true, it means we recursively call QueueFKConstraintValidation(),
> then we don't add the newconstraint to the queue.
>
> I'm not sure about this fix. Any thoughts?
it will fail for the following case:
begin;
drop table if exists fk;
drop table if exists pk;
create table pk(i int primary key);
insert into pk values (0), (1);
create table fk(i int) partition by range (i);
create table fk_1 partition of fk for values from (0) to (1);
create table fk_2 partition of fk for values from (1) to (3);
insert into fk values (1),(2);
alter table fk add foreign key(i) references pk not valid;
commit;
alter table fk validate constraint fk_i_fkey; --error, should fail.
-----------
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.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 54ad38247aa..df8e02f1060 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -431,7 +431,7 @@ 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, LOCKMODE lockmode, bool recursing);
static void QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relation rel,
char *constrName, HeapTuple contuple,
bool recurse, bool recursing, LOCKMODE lockmode);
@@ -11867,7 +11867,7 @@ AttachPartitionForeignKey(List **wqueue,
/* Use the same lock as for AT_ValidateConstraint */
QueueFKConstraintValidation(wqueue, conrel, partition, partcontup,
- ShareUpdateExclusiveLock);
+ ShareUpdateExclusiveLock, false);
ReleaseSysCache(partcontup);
table_close(conrel, RowExclusiveLock);
}
@@ -12919,7 +12919,7 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName,
{
if (con->contype == CONSTRAINT_FOREIGN)
{
- QueueFKConstraintValidation(wqueue, conrel, rel, tuple, lockmode);
+ QueueFKConstraintValidation(wqueue, conrel, rel, tuple, lockmode, false);
}
else if (con->contype == CONSTRAINT_CHECK)
{
@@ -12953,18 +12953,23 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName,
*/
static void
QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel,
- HeapTuple contuple, LOCKMODE lockmode)
+ HeapTuple contuple, LOCKMODE lockmode, bool recursing)
{
Form_pg_constraint con;
AlteredTableInfo *tab;
HeapTuple copyTuple;
Form_pg_constraint copy_con;
+ bool pk_is_partitioned;
+ bool fk_is_partitioned;
con = (Form_pg_constraint) GETSTRUCT(contuple);
Assert(con->contype == CONSTRAINT_FOREIGN);
Assert(!con->convalidated);
- if (rel->rd_rel->relkind == RELKIND_RELATION)
+ pk_is_partitioned = (get_rel_relkind(con->confrelid) == RELKIND_PARTITIONED_TABLE);
+ fk_is_partitioned = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
+
+ if (rel->rd_rel->relkind == RELKIND_RELATION && !recursing)
{
NewConstraint *newcon;
Constraint *fkconstraint;
@@ -12991,8 +12996,7 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel,
* If the table at either end of the constraint is partitioned, we need to
* recurse and handle every constraint that is a child of this constraint.
*/
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
- get_rel_relkind(con->confrelid) == RELKIND_PARTITIONED_TABLE)
+ if (pk_is_partitioned || fk_is_partitioned)
{
ScanKeyData pkey;
SysScanDesc pscan;
@@ -13024,7 +13028,7 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel,
childrel = table_open(childcon->conrelid, lockmode);
QueueFKConstraintValidation(wqueue, conrel, childrel, childtup,
- lockmode);
+ lockmode, fk_is_partitioned ? false : true);
table_close(childrel, NoLock);
}
