Re: NOT ENFORCED constraint feature

On Thu, Mar 27, 2025 at 11:05 AM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
>
>
> On Wed, Mar 26, 2025 at 9:33 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>>
>> On Tue, Mar 25, 2025 at 10:18 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>> >
>> > On 21.03.25 06:58, Amul Sul wrote:
>> > >
>> > > [....]
>> > > Attached is the updated version, where the commit messages for patch
>> > > 0005 and 0006 have been slightly corrected. Additionally, a few code
>> > > comments have been updated to consistently use the ENFORCED/NOT
>> > > ENFORCED keywords. The rest of the patches and all the code are
>> > > unchanged.
>> >
>> > I have committed patches 0001 through 0003. I made some small changes:
>> >
>>
>> Thank you very much !
>>
> I wanted to run my dump and restore test on this patch set but it doesn't apply cleanly. I tried applying 0004-0006. 0004 applies but not 0005.
>

Yes, I failed to notice that due to the previous patch commit with
Peter's improvements, this has started failing to apply. I've attached
the rebased version.

> I reviewed the tests to see if they leave objects in enough varied states for 002_pg_upgrade to test it well. The test frequently drops and recreates same partitioned and non-partitioned table inducing different states. So I have a feeling that we are just leaving one state combination behind and not all. It's an existing problem but probably with enforced and valid states it becomes more serious. I ended up reviewing the tests. Here are some comments.
>

Thanks for the review.

> +-- Reverting it back to ENFORCED will result in failure because constraint validation will be triggered,
> +-- as it was previously in a valid state.
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
> +ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_ftest1_fkey"
> +DETAIL: Key (ftest1)=(1) is not present in table "pktable".
>
> The text of error is misleading. There's no INSERT or UPDATE happening it's ALTER. That's what VALIDATE CONSTRAINT also reports, so not fault of this patch. But thought of writing it here since I noticed this now.
>

Yes, this is an existing issue, and Peter too mentioned the same in his
previous review[1].

> +-- Remove any existing rows that violate the constraint, then attempt to change
> +-- it.
> +TRUNCATE FKTABLE;
>
> I think, it will be good if we don't remove all the data; we wouldn't know whether constraint was considered validated because there's no data or it actually scanned the table and found no rows violating the constraint. Let's leave/insert one row which obeys the constraint.
>

Makes sense, did it that way.

> +-- Modifying other attributes of a constraint should not affect its enforceability, and vice versa
> +ALTER TABLE FKTABLE ADD CONSTRAINT fk_con FOREIGN KEY(ftest1, ftest2) REFERENCES PKTABLE NOT VALID NOT ENFORCED;
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fk_con DEFERRABLE INITIALLY DEFERRED;
> +SELECT condeferrable, condeferred, conenforced, convalidated
> +FROM pg_constraint WHERE conname = 'fk_con';
> + condeferrable | condeferred | conenforced | convalidated
> +---------------+-------------+-------------+--------------
> + t | t | f | f
> +(1 row)
> +
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fk_con NOT ENFORCED;
> +SELECT condeferrable, condeferred, conenforced, convalidated
> +FROM pg_constraint WHERE conname = 'fk_con';
> + condeferrable | condeferred | conenforced | convalidated
> +---------------+-------------+-------------+--------------
> + t | t | f | f
> +(1 row)
>
> The statement is marking a NOT ENFORCED constraint as NOT ENFORCED. What is it trying to test?
>

There was previously a bug that caused changes to other attributes.
Here, I wanted to verify that nothing changes if the constraint is
already in the desired state.

> +
> +ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT ENFORCED;
> +BEGIN;
> +-- doesn't match FK, no error.
>
> A "but" before no error would make the comment clearer.
>

Done.

> +UPDATE pktable SET id = 10 WHERE id = 5;
> +-- doesn't match PK, no error.
>
> A "but" before no error would make the comment clearer.
>

Done.

> CREATE TABLE fk_partitioned_fk_1 (fdrop1 int, fdrop2 int, a int, fdrop3 int, b int);
> ALTER TABLE fk_partitioned_fk_1 DROP COLUMN fdrop1, DROP COLUMN fdrop2, DROP COLUMN fdrop3;
> ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_1 FOR VALUES FROM (0,0) TO (1000,1000);
> -ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk;
> +ALTER TABLE fk_partitioned_fk ADD CONSTRAINT fk_partitioned_fk_a_b_fkey FOREIGN KEY (a, b)
> + REFERENCES fk_notpartitioned_pk NOT ENFORCED;
> CREATE TABLE fk_partitioned_fk_2 (b int, fdrop1 int, fdrop2 int, a int);
> ALTER TABLE fk_partitioned_fk_2 DROP COLUMN fdrop1, DROP COLUMN fdrop2;
> +ALTER TABLE fk_partitioned_fk_2 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT ENFORCED;
> ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000);
> +ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey ENFORCED;
>
> I don't understand what value this change is bringing? Maybe a comment explaining it. Why did we change name of one constraint and not the other?
>

Earlier, I used the system-generated constraint name in the ALTER
TABLE command, but I was advised[1] to use an explicit name instead.
As a result, I started using an explicit name when creating the
constraint on the fk_partitioned_fk table, which I plan to modify
later. However, I didn't take into account the other table that wasn't
being modified, such as the constraint for fk_partitioned_fk_2.

> ERROR: insert or update on table "fk_partitioned_fk_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
> DETAIL: Key (a, b)=(500, 501) is not present in table "fk_notpartitioned_pk".
> INSERT INTO fk_partitioned_fk (a,b) VALUES (1500, 1501);
> -ERROR: insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
> +ERROR: insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_b_fkey"
> DETAIL: Key (a, b)=(1500, 1501) is not present in table "fk_notpartitioned_pk".
> INSERT INTO fk_partitioned_fk_2 (a,b) VALUES (1500, 1501);
> -ERROR: insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
> +ERROR: insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_b_fkey"
> DETAIL: Key (a, b)=(1500, 1501) is not present in table "fk_notpartitioned_pk".
> INSERT INTO fk_partitioned_fk (a,b) VALUES (2500, 2502);
> ERROR: insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
>
> These diffs would go away if we didn't rename the constraint.
>

The next patch, 0006, will revert this diff where it removes the ALTER
command that adds the constraint to fk_partitioned_fk_2. Previously,
the fk_partitioned_fk_2 table inherited its constraint from the parent
via the ATTACH operation, resulting in the same name as the parent
constraint. However, this patch explicitly created the constraint,
which led to a different name than when it was inherited.
Nevertheless, I fix this by explicitly specifying the constraint
name to match the parent constraint in the ALTER command to minimize
the diff in the 0005 patch.

> @@ -1667,6 +1753,37 @@ Indexes:
> Referenced by:
> TABLE "fk_partitioned_fk" CONSTRAINT "fk_partitioned_fk_a_b_fkey" FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk(a, b)
>
> +-- Check the exsting FK trigger
> +CREATE TEMP TABLE tmp_trg_info AS SELECT tgtype, tgrelid::regclass, tgconstraint
> +FROM pg_trigger
> +WHERE tgrelid IN (SELECT relid FROM pg_partition_tree('fk_partitioned_fk'::regclass)
> + UNION ALL SELECT 'fk_notpartitioned_pk'::regclass);
> +SELECT count(1) FROM tmp_trg_info;
> + count
> +-------
> + 14
> +(1 row)
> +
> +ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey NOT ENFORCED;
> +-- No triggers
> +SELECT count(1) FROM pg_trigger
> +WHERE tgrelid IN (SELECT relid FROM pg_partition_tree('fk_partitioned_fk'::regclass)
> + UNION ALL SELECT 'fk_notpartitioned_pk'::regclass);
> + count
> +-------
> + 0
> +(1 row)
> +
> +-- Changing it back to ENFORCED will recreate the necessary triggers.
> +ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey ENFORCED;
> +-- Should be exactly the same number of triggers found as before
> +SELECT COUNT(1) FROM pg_trigger pgt JOIN tmp_trg_info tt
> +ON (pgt.tgtype = tt.tgtype AND pgt.tgrelid = tt.tgrelid AND pgt.tgconstraint = tt.tgconstraint);
> + count
> +-------
> + 14
> +(1 row)
>
> Why don't we just print the relevant triggers. Just matching counts could be misleading - we may have added one and dropped another keeping the count same.
>

I am not sure how to make such tests stable enough since the trigger
name involves OIDs. In count check, I tried adjusting the join
condition to ensure that I get the exact same type of constraint
w.r.t. trigger relation and the constraint.

> +BEGIN;
> +ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey NOT ENFORCED;
> +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES IN (1500,1502);
> +-- should have only one constraint
> +\d fk_partitioned_fk_2
> + Table "public.fk_partitioned_fk_2"
> + Column | Type | Collation | Nullable | Default
> +--------+---------+-----------+----------+---------
> + b | integer | | |
> + a | integer | | |
> +Partition of: fk_partitioned_fk FOR VALUES IN (1500, 1502)
> +Foreign-key constraints:
> + TABLE "fk_partitioned_fk" CONSTRAINT "fk_partitioned_fk_a_b_fkey" FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk(a, b) ON UPDATE CASCADE ON DELETE CASCADE NOT ENFORCED
>
> Did it just rename an existing constraint on fk_partitioned_fk_2? Is that ok? I thought we just mark the constraint as inherited.
>

This is the existing behavior of the psql meta command, which hides
the child constraint name when it inherits the parent constraint.

Regards,
Amul

1] http://postgr.es/m/CAAJ_b94PTcHzp2BqOxQZ7S-Zxp2hGV192a=4V8dTifjypDPpEw@mail.gmail.com