Re: NOT ENFORCED constraint feature

On Mon, Nov 18, 2024 at 7:53 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 18.11.24 13:42, jian he wrote:
> > i only played around with
> > v4-0001-Add-support-for-NOT-ENFORCED-in-CHECK-constraints.patch.
> >

Thanks for the review, and sorry for the delay — I was on vacation.

> > create table t(a int);
> > alter table t ADD CONSTRAINT the_constraint CHECK (a > 0) NOT ENFORCED;
> > insert into t select -1;
> > select  conname, contype,condeferrable,condeferred, convalidated,
> > conenforced,conkey,connoinherit
> > from    pg_constraint
> > where   conrelid = 't'::regclass;
> >
> > pg_constraint->convalidated should be set to false for NOT ENFORCED constraint?
> > Am I missing something?
>
> The "validated" status is irrelevant when the constraint is set to not
> enforced.  But it's probably still a good idea to make sure the field is
> set consistently.  I'm also leaning toward setting it to false.  One
> advantage of that would be that if you set the constraint to enforced
> later, then it's automatically in the correct "not validated" state.
>

I have implemented this approach in the attached version and added the
corresponding code comments and documentation. As a result, I moved
the conenforced and similar flags in another structure, placing them
before the respective validation flags.

> >     <varlistentry id="sql-createtable-parms-enforce">
> >      <term><literal>ENFORCED</literal></term>
> >      <term><literal>NOT ENFORCED</literal></term>
> >      <listitem>
> >       <para>
> >        This is currently only allowed for <literal>CHECK</literal> constraints.
> >        If the constraint is <literal>NOT ENFORCED</literal>, this clause
> >        specifies that the constraint check will be skipped.  When the constraint
> >        is <literal>ENFORCED</literal>, check is performed after each statement.
> >        This is the default.
> >       </para>
> >      </listitem>
> >     </varlistentry>
> > "This is the default." kind of ambiguous?
> > I think you mean: by default, all constraints are implicit ENFORCED.
>
> Maybe "the latter is the default" would be clearer.
>

Done.

> > + ereport(ERROR,
> > + (errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("misplaced ENFORCED clause"),
> > + parser_errposition(cxt->pstate, con->location)));
> >
> > + ereport(ERROR,
> > + (errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("misplaced NOT ENFORCED clause"),
> > + parser_errposition(cxt->pstate, con->location)));
> >
> > https://www.merriam-webster.com/dictionary/misplace
> > says:
> > "to put in a wrong or inappropriate place"
> >
> > I found the "misplaced" error message is not helpful.
> > for example:
> > CREATE TABLE UNIQUE_EN_TBL(i int UNIQUE ENFORCED);
> > ERROR:  misplaced ENFORCED clause
> > the error message only tells us thatspecify ENFORCED is wrong.
> > but didn't say why it's wrong.
> >
> > we can saying that
> > "ENFORCED clauses can only be used for CHECK constraints"
>
> This handling is similar to other error messages in
> transformConstraintAttrs().  It could be slightly improved, but it's not
> essential for this patch.
>
> > ------------------------------------------------------------------
> > the following queries is a bug?
> >
> > drop table t;
> > create table t(a int);
> >  NOT ENFORCED;
> > insert into t select -1;
> > alter table t add constraint cc1 check (a > 1) not ENFORCED not ENFORCED;
> > ERROR:  check constraint "cc1" of relation "t" is violated by some row
> > alter table t add constraint cc1 check (a > 1) not ENFORCED;
> > ERROR:  check constraint "cc1" of relation "t" is violated by some row
> >
> > ------------------------------------------------------------------
> > drop table t;
> > create table t(a int);
> > alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED not enforced;
> >
> > seems not easy to make it fail with alter table multiple "not enforced".
> > I guess it should be fine.
> > since we disallow a mix of "not enforced" and "enforced".
> >
> > alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED enforced;
> > ------------------------------------------------------------------
>
> Hmm, these duplicate clauses should have been caught by
> transformConstraintAttrs().
>

transformConstraintAttrs() is used when adding constraints in CREATE
TABLE statements. I can see similar behavior with other flags as well.

Eg: alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT DEFERRABLE NOT DEFERRABLE;

> > typedef struct Constraint
> > {
> >      NodeTag        type;
> >      ConstrType    contype;        /* see above */
> >      char       *conname;        /* Constraint name, or NULL if unnamed */
> >      bool        deferrable;        /* DEFERRABLE? */
> >      bool        initdeferred;    /* INITIALLY DEFERRED? */
> >      bool        skip_validation;    /* skip validation of existing rows? */
> >      bool        initially_valid;    /* mark the new constraint as valid? */
> >      bool        is_enforced;        /* enforced constraint? */
> > }
> > makeNode(Constraint) will default is_enforced to false.
> > Which makes the default value not what we want.
> > That means we may need to pay more attention for the trip from
> > makeNode(Constraint) to finally insert the constraint to the catalog.
> >
> > if we change it to is_not_enforced, makeNode will default to false.
> > is_not_enforced is false, means the constraint is enforced.
> > which is not that intuitive...
>
> Yes, it could be safer to make the field so that the default is false.
> I guess the skip_validation field is like that for a similar reason, but
> I'm not sure.
>

Ok. Initially, I was doing it the same way, but to maintain consistency
with the pg_constraint column and avoid negation in multiple places, I
chose that approach. However, I agree that having the default to false
would be safer. I’ve renamed the flag to is_not_enforced. Other names
I considered were not_enforced or is_unenforced, but since we already
have existing flags with two underscores, is_not_enforced shouldn't be
a problem.

> > ------------------------------------------------------------------
> > do we need to update for "enforced" in
> > https://www.postgresql.org/docs/current/sql-keywords-appendix.html
> > ?
> > ------------------------------------------------------------------
>
> That is generated automatically.
>
> > seems don't have
> > ALTER TABLE <name> VALIDATE CONSTRAINT
> > interacts with not forced sql tests.
> > for example:
> >
> > drop table if exists t;
> > create table t(a int);
> > alter table t add constraint cc check (a <> 1) not enforced NOT VALID;
> > insert into t values(1); ---success.
> > alter table t validate constraint cc;
> >
> > select  conname,convalidated, conenforced
> > from    pg_constraint
> > where   conrelid = 't'::regclass;
> >
> > returns:
> >   conname | convalidated | conenforced
> > ---------+--------------+-------------
> >   cc      | t            | f
> >
> > Now we have a value in the table "t" that violates the check
> > constraint, while convalidated is true.
> > ----------------------------------------------------------------------------
>
> I think we should prevent running VALIDATE for not enforced constraints.
>   I don't know what that would otherwise mean.
>
> It's also questionable whether NOT VALID makes sense to specify.
>

In the attached version, now, throws an error on validation of a
non-enforced constraint, and the documentation has been updated to
describe this behavior.

I encountered an undesirable behavior with the existing code, where a
NOT VALID foreign key is not allowed on a partitioned table. I don’t
think this should be the case, so I tried removing that restriction
and found that the behavior is quite similar to a regular table with a
NOT VALID FK constraint. However, I still need to confirm the
necessity of this restriction. For now, I’ve bypassed the error for
not-enforced FK constraints and added a TODO. This is why patch 0005
is marked as WIP.

Regards,
Amul