Allow NOT VALID foreign key constraints on partitioned tables.

On Thu, Jan 2, 2025, at 5:49 PM, Amul Sul wrote:

When adding a new FK constraint or attaching a partitioned table, where
matching FK constraints are merged, we allow the parent constraint to be NOT
VALID while the child constraint remains VALID, which is harmless. However, the
reverse scenario -- where the parent constraint is VALID and the child is NOT
VALID -- is incorrect. To address this, when merging a NOT VALID FK constraint
from the child with a VALID parent constraint, it implicitly validates the
child constraint against its existing data and marks it as VALID. This behavior
aligns with adding a new FK constraint directly to the child table, which would
also validate the existing data.

Hmm, I'm not sure about this, which may cause surprising delays. Maybe it would be better that the operation fails with an error, so that the user can do VALIDATE CONSTRAINT explicitly and retry the ATTACH once all the partitions have been so processed.

On Fri, Jan 3, 2025 at 12:11 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On Thu, Jan 2, 2025, at 5:49 PM, Amul Sul wrote:

When adding a new FK constraint or attaching a partitioned table, where
matching FK constraints are merged, we allow the parent constraint to be NOT
VALID while the child constraint remains VALID, which is harmless. However, the
reverse scenario -- where the parent constraint is VALID and the child is NOT
VALID -- is incorrect. To address this, when merging a NOT VALID FK constraint
from the child with a VALID parent constraint, it implicitly validates the
child constraint against its existing data and marks it as VALID. This behavior
aligns with adding a new FK constraint directly to the child table, which would
also validate the existing data.

Hmm, I'm not sure about this, which may cause surprising delays. Maybe it would be better that the operation fails with an error, so that the user can do VALIDATE CONSTRAINT explicitly and retry the ATTACH once all the partitions have been so processed.

Error reporting would have made this straightforward, but the delay is
not a new, and the patch does not introduce any additional delay.
Setting the patch aside for a moment, consider the current behavior on
the master branch: if you have a partitioned table(see e.g. below)
where one of the child tables has a NOT VALID foreign key constraint,
adding a new VALID foreign key constraint to the partitioned table
will ignore the existing constraint on the child table. Instead, it
creates a new constraint, which ultimately triggers a scan of the
child table to validate the new constraint. E.g.

create table bar(i int primary key);
create table foo(i int) partition by range(i);
create table foo_p1 partition of foo for values from (0) to (10);
insert into foo_p1 values(1); -- value doesn't exists in bar
alter table foo_p1 add constraint fk_con foreign key(i) references bar
NOT VALID; -- ok

-- following triggers the validation and fails.
alter table foo add constraint fk_con foreign key(i) references bar;

The behavior with the patch remains the same, but instead of creating
a new foreign key constraint, it merges with the existing one and
validates it.

Regards,
Amul

On Mon, Jan 6, 2025 at 9:53 AM Amul Sul <sulamul@gmail.com> wrote:

I made the minor changes to the attached version and rebased it
against the latest master(9a45a89c38f).

Regards,
Amul

On 2025-Jan-15, Amul Sul wrote:

I made the minor changes to the attached version and rebased it
against the latest master(9a45a89c38f).

Pushed 0001, thanks.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

Suppose I have a hierarchy like this

parent
|
child
/\
/ \
/ grandchild2
/
grandchild1

and I have a validated constraint on grandchild1 and an invalid
constraint on child. What happens if I add a constraint on parent? In
my understanding, it should not attempt to revalidate the constraint on
grandchild1, because it's known valid; but I don't think I see code that
would skip validation there. That is, QueueFKConstraintValidation does
its thing unconditionally (esp. recursing to children), which seems
wrong.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this." (Fotis)
/messages/by-id/200606261359.k5QDxE2p004593@auth-smtp.hol.gr

On Wed, Jan 22, 2025 at 2:36 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Suppose I have a hierarchy like this

parent
|
child
/\
/ \
/ grandchild2
/
grandchild1

and I have a validated constraint on grandchild1 and an invalid
constraint on child. What happens if I add a constraint on parent? In
my understanding, it should not attempt to revalidate the constraint on
grandchild1, because it's known valid; but I don't think I see code that
would skip validation there. That is, QueueFKConstraintValidation does
its thing unconditionally (esp. recursing to children), which seems
wrong.

You’re correct; it’s fixed in the attached version, along with an
assert(!convalidated) in QueueFKConstraintValidation(), and I’ve
included tests to cover the change. Thanks for reviewing this patch.

Regards,
Amul

On 2025-Jan-22, Amul Sul wrote:

You’re correct; it’s fixed in the attached version, along with an
assert(!convalidated) in QueueFKConstraintValidation(), and I’ve
included tests to cover the change. Thanks for reviewing this patch.

OK thanks, looks good, I have pushed it now with some trivial
amendments.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Java is clearly an example of money oriented programming" (A. Stepanov)

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:

OK thanks, looks good, I have pushed it now with some trivial
amendments.

Looks like some of the queries need ORDER BY for stability.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=morepork&amp;dt=2025-01-23%2023%3A35%3A57

regards, tom lane

On Fri, Jan 24, 2025 at 7:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:

OK thanks, looks good, I have pushed it now with some trivial
amendments.

Looks like some of the queries need ORDER BY for stability.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=morepork&amp;dt=2025-01-23%2023%3A35%3A57

Attached is the patch to fix this. Apologies for the sloppy work, and
thanks for pointing it out.

Regards,
Amul

Hello Álvaro,

23.01.2025 17:04, Álvaro Herrera wrote:

OK thanks, looks good, I have pushed it now with some trivial
amendments.

Please look at the script that produces an error starting from b663b9436:
CREATE TABLE st (a int, primary key (a));
CREATE TABLE pt (a int,
  FOREIGN KEY (a) REFERENCES st ON DELETE SET NULL ON UPDATE SET NULL,
  FOREIGN KEY (a) REFERENCES st ON DELETE SET NULL ON UPDATE SET NULL
) PARTITION BY LIST (a);
CREATE TABLE tp1 PARTITION OF pt FOR VALUES IN (1, 2);
ALTER TABLE pt DETACH PARTITION tp1;
ALTER TABLE pt ATTACH PARTITION tp1 FOR VALUES IN (1, 2);

ERROR:  XX000: tuple already updated by self
LOCATION:  simple_heap_update, heapam.c:4374

Best regards,
Alexander Lakhin
Neon (https://neon.tech)

On Sat, Jan 25, 2025, at 6:00 AM, Alexander Lakhin wrote:

Hello Álvaro,

Please look at the script that produces an error starting from b663b9436:

Ah yes, this is my bug: I moved a CCI where it became conditional. Will fix, thanks for the test case.

On 2025-Jan-25, Álvaro Herrera wrote:

On Sat, Jan 25, 2025, at 6:00 AM, Alexander Lakhin wrote:

Hello Álvaro,

Please look at the script that produces an error starting from b663b9436:

Ah yes, this is my bug: I moved a CCI where it became conditional.
Will fix, thanks for the test case.

Pushed the fix, thanks.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)

On Sun, Jan 26, 2025 at 10:08 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2025-Jan-25, Álvaro Herrera wrote:

On Sat, Jan 25, 2025, at 6:00 AM, Alexander Lakhin wrote:

Hello Álvaro,

Please look at the script that produces an error starting from b663b9436:

Ah yes, this is my bug: I moved a CCI where it became conditional.
Will fix, thanks for the test case.

Pushed the fix, thanks.

Thank you !

Regards,
Amul