Обсуждение: Re: Foreign key validation failure in 18beta1

On Wed, May 28, 2025 at 8:38 PM Tender Wang <tndrwang@gmail.com> wrote:
>
>
>
> Alvaro Herrera <alvherre@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.

On Thu, May 29, 2025 at 8:12 PM Amul Sul <sulamul@gmail.com> 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.
>

Later, I came up with "need_validate", but it seems "queueValidation"
is better.

I just realized we have the same problem with ALTER FOREIGN KEY ENFORCED.
for example:
begin;
drop table if exists fk;
drop table if exists pk;
create table pk(i int, b int, primary key(i ,b)) partition by range (i);
create table pk_1 partition of pk for values from (0) to (10)
partition by list(b);
CREATE TABLE pk_1_p1 PARTITION OF pk_1 FOR VALUES IN (0, 1, 2);
create table pk_2 partition of pk for values from (10) to (12);
insert into pk values (0, 1), (1,2);
create table fk(i int, b int);
insert into fk values (0, 1), (1,3);
-- alter table fk add foreign key(i) references pk;
alter table fk add constraint fk_i_fkey foreign key(i, b) references
pk not enforced;
commit;

alter table fk alter constraint fk_i_fkey enforced; --error
delete from fk where i = 1 and b = 3;
alter table fk alter constraint fk_i_fkey enforced; --ok

On Thu, May 29, 2025 at 5:57 PM jian he <jian.universality@gmail.com> wrote:
>
> On Thu, May 29, 2025 at 8:12 PM Amul Sul <sulamul@gmail.com> 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.
> >
>
> Later, I came up with "need_validate", but it seems "queueValidation"
> is better.
>
> I just realized we have the same problem with ALTER FOREIGN KEY ENFORCED.
> for example:

Yeah, I think adding a "currcon->confrelid == pkrelid" check before
enqueueing validation in ATExecAlterConstrEnforceability() would
address the issue, though I still need to test it thoroughly.

Regards,
Amul

On Thu, May 29, 2025 at 8:58 PM Amul Sul <sulamul@gmail.com> wrote:
> >
> > I just realized we have the same problem with ALTER FOREIGN KEY ENFORCED.
> > for example:
>
> Yeah, I think adding a "currcon->confrelid == pkrelid" check before
> enqueueing validation in ATExecAlterConstrEnforceability() would
> address the issue, though I still need to test it thoroughly.
>

adding a "currcon->confrelid == pkrelid"  will fix the problem.
I have used the attached scripts to test foreign key functionality:
ALTER TABLE VALIDATE CONSTRAINT and
ALTER TABLE ALTER CONSTRAINT ENFORCED.


v3-0001-foreign_key_validation-WIP also works fine.
but, I have one minor issue with the comment.

+ /*
+ * 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.

The last sentence doesn't seem to explain why.
The following is what I came up with (my understanding):
""
If the primary key (PK) is partitioned, we *can't* queue validation (add a
NewConstraint on PK partition and validate it in phase3) for its partitions.
Validating foreign key constraints between primary key (PK) partitions and
foreign keys (FK) can fail. This is because some PK partitions may legitimately
lack corresponding FK values, which is acceptable but causes validation errors.
""

On Fri, May 30, 2025 at 6:32 PM Amul Sul <sulamul@gmail.com> wrote:
>
>
> Kindly take a look at the attached version. I've also added the tests.
> Thanks for your script -- all tests are passing with this patch.
>

hi.

+ * Note that validation should be performed against the referencing
+ * root table only, not its child partitions. See
+ * QueueFKConstraintValidation() for more details.
  */
        if (rel->rd_rel->relkind == RELKIND_RELATION &&
            currcon->confrelid == pkrelid)
        {
            AlteredTableInfo *tab;
            NewConstraint *newcon;
            newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
            ....
        }

in the comments "referencing" should be "referenced"?

other than that, it looks good.

On Sun, Jun 1, 2025 at 6:05 PM jian he <jian.universality@gmail.com> wrote:
>
> On Fri, May 30, 2025 at 6:32 PM Amul Sul <sulamul@gmail.com> wrote:
> >
[...]
>
> + * Note that validation should be performed against the referencing
> + * root table only, not its child partitions. See
> + * QueueFKConstraintValidation() for more details.
>   */
>         if (rel->rd_rel->relkind == RELKIND_RELATION &&
>             currcon->confrelid == pkrelid)
>         {
>             AlteredTableInfo *tab;
>             NewConstraint *newcon;
>             newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
>             ....
>         }
>
> in the comments "referencing" should be "referenced"?
>
Yes, fixed in the attached version.

> other than that, it looks good.
>
Thanks for the review.

Regards,
Amul

On Tue, Jun 3, 2025 at 12:14 PM Amul Sul <sulamul@gmail.com> wrote:
> >
>
> I found a third approach that requires only a few changes. The key
> idea is to determine the root referenced table and pass it to
> QueueFKConstraintValidation(). We would then enqueue phase 3
> validation only if the constraint tuple’s confrelid matches that root
> table -- similar to what is doing in ATExecAlterConstrEnforceability().
>
> This would also ensure that the logic for adding/skipping phase 3
> validation is consistent in both places.
>
indeed!

v7 is way more intuitive compared with v5, v6.
The commit message also looks fine.

On 2025-Jun-04, jian he wrote:

> On Tue, Jun 3, 2025 at 12:14 PM Amul Sul <sulamul@gmail.com> wrote:
>
> > I found a third approach that requires only a few changes. The key
> > idea is to determine the root referenced table and pass it to
> > QueueFKConstraintValidation(). We would then enqueue phase 3
> > validation only if the constraint tuple’s confrelid matches that root
> > table -- similar to what is doing in ATExecAlterConstrEnforceability().
> >
> > This would also ensure that the logic for adding/skipping phase 3
> > validation is consistent in both places.
>
> indeed!
> 
> v7 is way more intuitive compared with v5, v6.

Agreed, this version is better than the previous ones.  I split it as
two commits, for NOT VALID and NOT ENFORCED separately, and modified some
comments a bit as well as the commit messages.

Thank you!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)

On Thu, Jun 5, 2025 at 10:31 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Jun-04, jian he wrote:
>
> > On Tue, Jun 3, 2025 at 12:14 PM Amul Sul <sulamul@gmail.com> wrote:
> >
> >
> > v7 is way more intuitive compared with v5, v6.
>
> Agreed, this version is better than the previous ones.  I split it as
> two commits, for NOT VALID and NOT ENFORCED separately, and modified some
> comments a bit as well as the commit messages.
>

Yeah, it makes sense to have this as a separate commit. Thank you!

Regards,
Amul