Re: NOT ENFORCED constraint feature
Peter Eisentraut <peter@eisentraut.org>
From: Peter Eisentraut <peter@eisentraut.org>
To: Amul Sul <sulamul@gmail.com>
Cc: Álvaro Herrera <alvherre@alvh.no-ip.org>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, jian he <jian.universality@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Joel Jacobson <joel@compiler.org>, Alexandra Wang <alexandra.wang.oss@gmail.com>, Suraj Kharage <suraj.kharage@enterprisedb.com>
Date: 2025-03-11T17:43:48Z
Lists: pgsql-hackers
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Add support for NOT ENFORCED in foreign key constraints
- eec0040c4bcd 18.0 landed
-
Expand test a bit
- 5d5f415816a6 18.0 landed
-
refactor: Pass relation OID instead of Relation to createForeignKeyCheckTriggers()
- ef7a5af77d44 18.0 landed
-
refactor: Split ATExecAlterConstraintInternal()
- 639238b978fe 18.0 landed
-
refactor: Move some code that updates pg_constraint to a separate function
- a3280e2a494f 18.0 landed
-
Move RemoveInheritedConstraint() call slightly earlier
- dabccf45139a 18.0 landed
-
refactor: Split tryAttachPartitionForeignKey()
- 1d26c2d2c4b8 18.0 landed
-
refactor: re-add ATExecAlterChildConstr()
- 64224a834ce4 18.0 landed
-
Add ATAlterConstraint struct for ALTER .. CONSTRAINT
- 80d7f990496b 18.0 landed
-
refactor: split ATExecAlterConstrRecurse()
- 7a947ed25b54 18.0 landed
-
Add support for NOT ENFORCED in CHECK constraints
- ca87c415e2fc 18.0 landed
I have committed the first three refactoring patches (v16-0001,
v16-0002, v16-0003). (I guess Álvaro didn't like the first one, so I
suppose I'll revert that one, but it's a simple one, so you can proceed
either way.)
I think the next step here is that you work to fix Álvaro's concerns
about the recursion structure.
I have a few other review comments here in the meantime:
* patch v16-0007 "Ease the restriction that a NOT ENFORCED constraint
must be INVALID."
I don't understand the purpose of this one. And the commit message also
doesn't explain the reason, only what it does. I think we have settled
on three states (not enforced and not valid; enforced but not yet valid;
enforced and valid), so it seems sensible to keep valid as false if
enforced is also false. Did I miss something?
Specifically, this test case change
-ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
(b > 10) NOT ENFORCED; -- succeeds
+ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
(b > 10) NOT ENFORCED; -- fail
+ERROR: check constraint "b_greater_than_ten_not_enforced" of relation
"attmp3" is violated by some row
+ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
(b > 10) NOT VALID NOT ENFORCED; -- succeeds
seems very wrong to me.
* doc/src/sgml/advanced.sgml
Let's skip that. This material is too advanced for a tutorial.
* doc/src/sgml/ddl.sgml
Let's move the material about NOT ENFORCED into a separate section or
paragraph, not in the first paragraph that introduces foreign keys. I
suggest a separate sect2-level section at the end of the "Constraints"
section.
* src/backend/catalog/sql_features.txt
The SQL standard has NOT ENFORCED only for check and foreign-key
constraints, so you could flip this to "YES" here. (Hmm, do we need
to support not-null constraints, though (which are grouped under check
constraints in the standard)? Maybe turn the comment around and say
"except not-null constraints" or something like that.)
* src/backend/commands/tablecmds.c
I would omit this detail message:
errdetail("Enforceability can only be altered for foreign key constraints.")
We have generally tried to get rid of detail messages that say "cannot
do this on this object type, but you could do it on a different object
type", since that is not actually useful.
* src/test/regress/expected/foreign_key.out
This error message is confusing, since no insert or update is happening:
+ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
+ERROR: insert or update on table "fktable" violates foreign key
constraint "fktable_ftest1_fkey"
Could we give a differently worded error message in this case?
Here, you are relying on the automatic constraint naming, which seems
fragile and confusing:
+ALTER TABLE FKTABLE ADD FOREIGN KEY(ftest1, ftest2) REFERENCES PKTABLE
NOT VALID NOT ENFORCED;
+ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_ftest2_fkey
DEFERRABLE INITIALLY DEFERRED;
Better name the constraint explicitly in the first command.