Re: NOT ENFORCED constraint feature

On Tue, Mar 11, 2025 at 11:13 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> 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.)
>

Sure, thank you !

> I think the next step here is that you work to fix Álvaro's concerns
> about the recursion structure.
>

Yes, I worked on that in the attached version. I refactored
ATExecAlterConstraintInternal() and moved the code that updates the
pg_constraint entry into a separate function (see 0001), so it can be
called from the places where the entry needs to be updated, rather
than revisiting ATExecAlterConstraintInternal(). In 0002,
ATExecAlterConstraintInternal() is split into two functions:
ATExecAlterConstrDeferrability() and
ATExecAlterConstrInheritability(), which handle altering deferrability
and inheritability, respectively. These functions are expected to
recurse on themselves, rather than revisiting
ATExecAlterConstraintInternal() as before. This approach simplifies
things. Similarly can add ATExecAlterConstrEnforceability() which
recurses itself.

> 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?
>

I attempted to implement this [1], but later didn’t switch to your
suggested three-state approach [2] because I hadn’t received
confirmation for it.

Anyway, I’ve now tried the [2] approach in the attached patch. Could
you kindly confirm my understanding of the pg_constraint entry
updates:

When the constraint is changed to NOT ENFORCED, both conenforced and
convalidated should be set to false. Similarly, when the constraint is
changed to ENFORCED, validation must be performed, and both of these
flags should be set to true.

> 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.
>

Agreed. I have dropped this patch since it is no longer needed with
your suggested approach[2].

> * doc/src/sgml/advanced.sgml
>
> Let's skip that. This material is too advanced for a tutorial.
>

Done.

> * 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.
>

I skipped that as well, since I realized that there is no description
regarding deferrability in that patch. This information can be found
on the CREATE TABLE page, which this section references for more
details.

> * 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.)
>

Done.

> * 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
>

Done.

> 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?
>

I noticed a similar error when adding a constraint through ALTER
TABLE, coming from ri_ReportViolation. I don’t have an immediate
solution, but I believe we need to pass some context to
ri_ReportViolation to indicate what has been done when it is called
from RI_PartitionRemove_Check.

> 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.
>

Fixed in the attached version.

Regards,
Amul.

1] http://postgr.es/m/CAExHW5tqoQvkGbYJHQUz0ytVqT7JyT7MSq0xuc4-qSQaNPfRBQ@mail.gmail.com
2] http://postgr.es/m/50f46903-20e1-4e23-918c-a6cfdf1a9f4a@eisentraut.org