Re: NOT ENFORCED constraint feature - Mailing list pgsql-hackers

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

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

> + 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);
> alter table t ADD CONSTRAINT cc CHECK (a > 0) 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().

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

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