Foreign key validation failure in 18beta1
I've come across an unexpected ERROR during validation of FK constraint in PG
18beta1. The same works in PG 17:
drop table if exists fk;
drop table if exists pk;
create table pk(i int primary key) partition by range (i);
create table pk_1 partition of pk for values from (0) to (1);
create table pk_2 partition of pk for values from (1) to (2);
insert into pk values (0), (1);
create table fk(i int);
insert into fk values (1);
-- Works if the FK constraint is created as valid.
--alter table fk add foreign key(i) references pk;
-- Fails if the FK constraint is created as NOT VALID and validated
-- afterwards.
alter table fk add foreign key(i) references pk not valid;
alter table fk validate constraint fk_i_fkey;
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
Antonin Houska <ah@cybertec.at> 于2025年5月28日周三 15:51写道：
I've come across an unexpected ERROR during validation of FK constraint in
PG
18beta1. The same works in PG 17:drop table if exists fk;
drop table if exists pk;
create table pk(i int primary key) partition by range (i);
create table pk_1 partition of pk for values from (0) to (1);
create table pk_2 partition of pk for values from (1) to (2);
insert into pk values (0), (1);
create table fk(i int);
insert into fk values (1);-- Works if the FK constraint is created as valid.
--alter table fk add foreign key(i) references pk;-- Fails if the FK constraint is created as NOT VALID and validated
-- afterwards.
alter table fk add foreign key(i) references pk not valid;
alter table fk validate constraint fk_i_fkey;
git bisect shows since below commit, the failure started.
commit b663b9436e7509b5e73c8c372539f067cd6e66c1
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu Jan 23 15:54:38 2025 +0100
Allow NOT VALID foreign key constraints on partitioned tables
--
Thanks,
Tender Wang
Tender Wang <tndrwang@gmail.com> 于2025年5月28日周三 18:54写道：
Antonin Houska <ah@cybertec.at> 于2025年5月28日周三 15:51写道：
I've come across an unexpected ERROR during validation of FK constraint
in PG
18beta1. The same works in PG 17:drop table if exists fk;
drop table if exists pk;
create table pk(i int primary key) partition by range (i);
create table pk_1 partition of pk for values from (0) to (1);
create table pk_2 partition of pk for values from (1) to (2);
insert into pk values (0), (1);
create table fk(i int);
insert into fk values (1);-- Works if the FK constraint is created as valid.
--alter table fk add foreign key(i) references pk;-- Fails if the FK constraint is created as NOT VALID and validated
-- afterwards.
alter table fk add foreign key(i) references pk not valid;
alter table fk validate constraint fk_i_fkey;git bisect shows since below commit, the failure started.
commit b663b9436e7509b5e73c8c372539f067cd6e66c1
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu Jan 23 15:54:38 2025 +0100Allow NOT VALID foreign key constraints on partitioned tables
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.
--
Thanks,
Tender Wang
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.
Clearly b663b9436e75 lacked tests.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Sallah, I said NO camels! That's FIVE camels; can't you count?"
(Indiana Jones)
Attachments:
0001-Don-t-validate-referenced-side-FK-subconstraints.patchtext/x-diff; charset=utf-8Download+22-3
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?
--
Thanks,
Tender Wang
Attachments:
fk_val.difftext/plain; charset=US-ASCII; name=fk_val.diffDownload+10-7
Tender Wang <tndrwang@gmail.com> 于2025年5月28日周三 20:38写道：
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 inpk_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 found a case that proves what I said above.
create table pk(i int, j int, primary key(i,j)) partition by range (i);
create table pk_1 partition of pk for values from (0) to (1) partition by
list(j);
create table pk_2 partition of pk for values from (1) to (2) partition by
list(j);
create table pk_1_1 partition of pk_1 for values in (1);
create table pk_2_1 partition of pk_2 for values in (2);
create table fk(i int, j int);
alter table fk add foreign key(i, j) references pk not valid;
postgres=# select oid ,conname , contype,
convalidated,conrelid,conparentid,confrelid from pg_constraint where oid >=
16384;
oid | conname | contype | convalidated | conrelid | conparentid |
confrelid
-------+---------------+---------+--------------+----------+-------------+-----------
16422 | fk_i_j_fkey | f | f | 16419 |
0 | 16384
16425 | fk_i_j_fkey_1 | f | f | 16419 | 16422 |
16391
16428 | fk_i_j_fkey_2 | f | f | 16419 | 16425 |
16405
16431 | fk_i_j_fkey_3 | f | f | 16419 | 16422 |
16398
16434 | fk_i_j_fkey_4 | f | f | 16419 | 16431 |
16412
alter table fk validate constraint fk_i_j_fkey;
postgres=# select oid ,conname , contype,
convalidated,conrelid,conparentid,confrelid from pg_constraint where oid >=
16384;
oid | conname | contype | convalidated | conrelid | conparentid |
confrelid
-------+---------------+---------+--------------+----------+-------------+-----------
16428 | fk_i_j_fkey_2 | f | f | 16419 | 16425 |
16405
16434 | fk_i_j_fkey_4 | f | f | 16419 | 16431 |
16412
16425 | fk_i_j_fkey_1 | f | t | 16419 | 16422 |
16391
16431 | fk_i_j_fkey_3 | f | t | 16419 | 16422 |
16398
16422 | fk_i_j_fkey | f | t | 16419 |
0 | 16384
The fk_i_j_fkey_2 and fk_i_j_fkey_4 are still invalid with your patch.
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?
--
Thanks,
Tender Wang
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.
Attachments:
v2-0001-foreign_key_validation.difftext/x-patch; charset=US-ASCII; name=v2-0001-foreign_key_validation.diffDownload+12-8
On Thu, May 29, 2025 at 12:38 PM jian he <jian.universality@gmail.com> wrote:
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:
[...]
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
Attachments:
v3-0001-foreign_key_validation-WIP.patchapplication/x-patch; name=v3-0001-foreign_key_validation-WIP.patchDownload+42-9
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.
""
Attachments:
On Fri, May 30, 2025 at 1:37 PM jian he <jian.universality@gmail.com> wrote:
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.
Thanks for the checking.
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.
""
I would prefer to use 'referenced' and 'referencing' tables instead of
'PK' and 'FK' tables, to stay consistent with the existing style. I
tried to improve the comment in the attached version with the
appropriate reason.
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.
Regards,
Amul
Attachments:
v4-0001-Skip-adding-action-based-foreign-key-constraints-.patchapplication/octet-stream; name=v4-0001-Skip-adding-action-based-foreign-key-constraints-.patchDownload+90-24
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
Attachments:
v5-0001-Skip-adding-action-based-foreign-key-constraints-.patchapplication/x-patch; name=v5-0001-Skip-adding-action-based-foreign-key-constraints-.patchDownload+90-24
Hello
I find this coding somewhat confusing. Now you have a function
"QueueFkConstraintValidation" which may queue a FK queue constraint
validation, if the flag "queueValidation" is given, but it may also do
something else -- makes no sense IMO. I think it's better to split this
function in two; one does indeed validate, the other is a simple helper
that only sets the catalog flag, and does its own recursion. This
causes a bit of duplicate code, but is simpler to follow. (We could
also refactor the duplicate code by adding another helper routine, but
I'm not sure it's worth the trouble.)
I would propose something like this instead, what do you think?
I hate that pg_constraint.conparentid doesn't have a syscache or at
least an index :-(
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"El miedo atento y previsor es la madre de la seguridad" (E. Burke)
Attachments:
v6-0001-Skip-adding-action-based-foreign-key-constraints-.patchtext/x-diff; charset=utf-8Download+148-24
On Mon, Jun 2, 2025 at 9:56 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Hello
I find this coding somewhat confusing. Now you have a function
"QueueFkConstraintValidation" which may queue a FK queue constraint
validation, if the flag "queueValidation" is given, but it may also do
something else -- makes no sense IMO.
I wouldn’t disagree with that.
I think it's better to split this
function in two; one does indeed validate, the other is a simple helper
that only sets the catalog flag, and does its own recursion. This
causes a bit of duplicate code, but is simpler to follow. (We could
also refactor the duplicate code by adding another helper routine, but
I'm not sure it's worth the trouble.)I would propose something like this instead, what do you think?
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.
Adding an extra parameter (i.e., pkrelid) doesn’t seem out of place,
as it aligns with other functions in the same file, such as
ATExecAlterConstraintInternal() and ATExecAlterConstrEnforceability().
Kindly take a look at the attached version and share your thoughts.
Regards,
Amul
Attachments:
v7-0001-Skip-adding-action-based-foreign-key-constraints-.patchapplication/x-patch; name=v7-0001-Skip-adding-action-based-foreign-key-constraints-.patchDownload+84-28
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