Re: NOT ENFORCED constraint feature

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.