Thread: Re: NOT ENFORCED constraint feature

On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote:
> The attached patch proposes adding the ability to define CHECK and
> FOREIGN KEY constraints as NOT ENFORCED.

Thanks for working on this!

> Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch,

I've looked at the 0001 patch and think it looks simple and straight forward.

> but implementing it for FOREIGN KEY constraints requires more code due
> to triggers, see 0002 - 0005 patches.

I can't say that much yet about the code changes in 0002 - 0005 yet,
but I've tested the patches and successfully experimented with the feature.

Also think the documentation is good and sound. Only found a minor typo:
-    Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
+    Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>

> There are various approaches for
> implementing NOT ENFORCED foreign keys, what I thought of:
>
> 1. When defining a NOT ENFORCED foreign key, skip the creation of
> triggers used for referential integrity check, while defining an
> ENFORCED foreign key, remain the same as the current behaviour. If an
> existing foreign key is changed to NOT ENFORCED, the triggers are
> dropped, and when switching it back to ENFORCED, the triggers are
> recreated.
>
> 2. Another approach could be to create the NOT ENFORCED constraint
> with the triggers as usual, but disable those triggers by updating the
> pg_trigger catalog so that they are never executed for the check. And
> enable them when the constraint is changed back to ENFORCED.
>
> 3. Similarly, a final approach would involve updating the logic where
> trigger execution is decided and skipping the execution if the
> constraint is not enforced, rather than modifying the pg_trigger
> catalog.
>
> In the attached patch, the first approach has been implemented. This
> requires more code changes but prevents unused triggers from being
> left in the database and avoids the need for changes all over the
> place to skip trigger execution, which could be missed in future code
> additions.

I also like the first approach, since I think it's nice the pg_trigger
entires are inserted / deleted upon enforced / not enforced.

> The ALTER CONSTRAINT operation in the patch added code to handle
> dropping and recreating triggers. An alternative approach would be to
> simplify the process by dropping and recreating the FK constraint,
> which would automatically handle skipping or creating triggers for NOT
> ENFORCED or ENFORCED FK constraints. However, I wasn't sure if this
> was the right approach, as I couldn't find any existing ALTER
> operations that follow this pattern.

I think the current approach of dropping and recreating the triggers is best,
since if we would instead be dropping and recreating the FK constraint,
that would cause problems if some other future SQL feature would need to
introduce dependencies on the FK constraints via pg_depend.

Best regards,

Joel

On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote:

The attached patch proposes adding the ability to define CHECK and
FOREIGN KEY constraints as NOT ENFORCED.

Thanks for working on this!

Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch,

I've looked at the 0001 patch and think it looks simple and straight forward.

but implementing it for FOREIGN KEY constraints requires more code due
to triggers, see 0002 - 0005 patches.

I can't say that much yet about the code changes in 0002 - 0005 yet,
but I've tested the patches and successfully experimented with the feature.

Also think the documentation is good and sound. Only found a minor typo:
-    Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
+    Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>

There are various approaches for
implementing NOT ENFORCED foreign keys, what I thought of:

1. When defining a NOT ENFORCED foreign key, skip the creation of
triggers used for referential integrity check, while defining an
ENFORCED foreign key, remain the same as the current behaviour. If an
existing foreign key is changed to NOT ENFORCED, the triggers are
dropped, and when switching it back to ENFORCED, the triggers are
recreated.

2. Another approach could be to create the NOT ENFORCED constraint
with the triggers as usual, but disable those triggers by updating the
pg_trigger catalog so that they are never executed for the check. And
enable them when the constraint is changed back to ENFORCED.

3. Similarly, a final approach would involve updating the logic where
trigger execution is decided and skipping the execution if the
constraint is not enforced, rather than modifying the pg_trigger
catalog.

In the attached patch, the first approach has been implemented. This
requires more code changes but prevents unused triggers from being
left in the database and avoids the need for changes all over the
place to skip trigger execution, which could be missed in future code
additions.

I also like the first approach, since I think it's nice the pg_trigger
entires are inserted / deleted upon enforced / not enforced.

I also prefer this, as it gets us out from any possible dance with enabling / disabling triggers.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Oct 9, 2024 at 2:44 PM Joel Jacobson <joel@compiler.org> wrote:
>
> On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote:
> > The attached patch proposes adding the ability to define CHECK and
> > FOREIGN KEY constraints as NOT ENFORCED.
>
> Thanks for working on this!
>
> > Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch,
>
> I've looked at the 0001 patch and think it looks simple and straight forward.
>

Thanks for looking into it.

> > but implementing it for FOREIGN KEY constraints requires more code due
> > to triggers, see 0002 - 0005 patches.
>
> I can't say that much yet about the code changes in 0002 - 0005 yet,
> but I've tested the patches and successfully experimented with the feature.
>
> Also think the documentation is good and sound. Only found a minor typo:
> -    Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
> +    Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
>

Ok, will fix it in the next version.

> > There are various approaches for
> > implementing NOT ENFORCED foreign keys, what I thought of:
> >
> > 1. When defining a NOT ENFORCED foreign key, skip the creation of
> > triggers used for referential integrity check, while defining an
> > ENFORCED foreign key, remain the same as the current behaviour. If an
> > existing foreign key is changed to NOT ENFORCED, the triggers are
> > dropped, and when switching it back to ENFORCED, the triggers are
> > recreated.
> >
> > 2. Another approach could be to create the NOT ENFORCED constraint
> > with the triggers as usual, but disable those triggers by updating the
> > pg_trigger catalog so that they are never executed for the check. And
> > enable them when the constraint is changed back to ENFORCED.
> >
> > 3. Similarly, a final approach would involve updating the logic where
> > trigger execution is decided and skipping the execution if the
> > constraint is not enforced, rather than modifying the pg_trigger
> > catalog.
> >
> > In the attached patch, the first approach has been implemented. This
> > requires more code changes but prevents unused triggers from being
> > left in the database and avoids the need for changes all over the
> > place to skip trigger execution, which could be missed in future code
> > additions.
>
> I also like the first approach, since I think it's nice the pg_trigger
> entires are inserted / deleted upon enforced / not enforced.
>
> > The ALTER CONSTRAINT operation in the patch added code to handle
> > dropping and recreating triggers. An alternative approach would be to
> > simplify the process by dropping and recreating the FK constraint,
> > which would automatically handle skipping or creating triggers for NOT
> > ENFORCED or ENFORCED FK constraints. However, I wasn't sure if this
> > was the right approach, as I couldn't find any existing ALTER
> > operations that follow this pattern.
>
> I think the current approach of dropping and recreating the triggers is best,
> since if we would instead be dropping and recreating the FK constraint,
> that would cause problems if some other future SQL feature would need to
> introduce dependencies on the FK constraints via pg_depend.
>

Yes, that was my initial thought as well, and recreating the
dependencies would be both painful and prone to bugs.

Regards,
Amul

On Wed, Oct 9, 2024 at 6:45 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2024-10-09 We 5:14 AM, Joel Jacobson wrote:
>
> On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote:
>
> The attached patch proposes adding the ability to define CHECK and
> FOREIGN KEY constraints as NOT ENFORCED.
>
> Thanks for working on this!
>
> Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch,
>
> I've looked at the 0001 patch and think it looks simple and straight forward.
>
> but implementing it for FOREIGN KEY constraints requires more code due
> to triggers, see 0002 - 0005 patches.
>
> I can't say that much yet about the code changes in 0002 - 0005 yet,
> but I've tested the patches and successfully experimented with the feature.
>
> Also think the documentation is good and sound. Only found a minor typo:
> -    Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
> +    Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
>
> There are various approaches for
> implementing NOT ENFORCED foreign keys, what I thought of:
>
> 1. When defining a NOT ENFORCED foreign key, skip the creation of
> triggers used for referential integrity check, while defining an
> ENFORCED foreign key, remain the same as the current behaviour. If an
> existing foreign key is changed to NOT ENFORCED, the triggers are
> dropped, and when switching it back to ENFORCED, the triggers are
> recreated.
>
> 2. Another approach could be to create the NOT ENFORCED constraint
> with the triggers as usual, but disable those triggers by updating the
> pg_trigger catalog so that they are never executed for the check. And
> enable them when the constraint is changed back to ENFORCED.
>
> 3. Similarly, a final approach would involve updating the logic where
> trigger execution is decided and skipping the execution if the
> constraint is not enforced, rather than modifying the pg_trigger
> catalog.
>
> In the attached patch, the first approach has been implemented. This
> requires more code changes but prevents unused triggers from being
> left in the database and avoids the need for changes all over the
> place to skip trigger execution, which could be missed in future code
> additions.
>
> I also like the first approach, since I think it's nice the pg_trigger
> entires are inserted / deleted upon enforced / not enforced.
>
>
>
> I also prefer this, as it gets us out from any possible dance with enabling / disabling triggers.
>

Agreed. Thanks for the inputs.

Regards,
Amul.

I started reviewing patch 0001 for check constraints.  I think it's a
good idea how you structured it so that we can start with this
relatively simple feature and get all the syntax parsing etc. right.

I also looked over the remaining patches a bit.  The general structure
looks right to me.  But I haven't done a detailed review yet.

The 0001 patch needs a rebase over the recently re-committed patch for
catalogued not-null constraints.  This might need a little work to
verify that everything still makes sense.

(I suppose technically we could support not-enforced not-null
constraints.  But I would stay away from that for now.  That not-null
constraints business is very complicated, don't get dragged into
it. ;-) )


Some more detailed comments on the code:

* src/backend/access/common/tupdesc.c

Try to keep the order of the fields consistent.  In tupdesc.h you have
ccenforced before ccnoinherit, here you have it after.  Either way is
fine, but let's keep it consistent.  (If you change it in tupdesc.h,
also check relcache.c.)


* src/backend/commands/tablecmds.c

             cooked->skip_validation = false;
+           cooked->is_enforced = true;
             cooked->is_local = true;    /* not used for defaults */
             cooked->inhcount = 0;   /* ditto */

Add a comment like "not used for defaults" to the new line.

Or maybe this should be rewritten slightly.  There might be more
fields that are not used for defaults, like "skip_validation"?  Maybe
they just shouldn't be set here, seems useless and confusing.

@@ -9481,6 +9484,9 @@ ATAddCheckConstraint(List **wqueue, 
AlteredTableInfo *tab, Relation rel,
     {
         CookedConstraint *ccon = (CookedConstraint *) lfirst(lcon);

+       /* Only CHECK constraint can be not enforced */
+       Assert(ccon->is_enforced || ccon->contype == CONSTRAINT_CHECK);
+

Is this assertion useful, since we are already in a function named
ATAddCheckConstraint()?

@@ -11947,7 +11961,9 @@ ATExecValidateConstraint(List **wqueue, Relation 
rel, char *constrName,
         }

         /*
-        * Now update the catalog, while we have the door open.
+        * Now update the catalog regardless of enforcement; the validated
+        * flag will not take effect until the constraint is marked as
+        * enforced.
          */

Can you clarify what you mean here?  Is the enforced flag set later?
I don't see that in the code.  What is the interaction between
constraint validation and the enforced flag?


* src/backend/commands/typecmds.c

You should also check and error if CONSTR_ATTR_ENFORCED is specified
(even though it's effectively the default).  This matches SQL standard
language: "For every <domain constraint> specified: ... If <constraint
characteristics> is specified, then neither ENFORCED nor NOT ENFORCED
shall be specified."

The error code should be something like
ERRCODE_INVALID_OBJECT_DEFINITION instead of
ERRCODE_FEATURE_NOT_SUPPORTED.  The former is more for features that
are impossible, the latter for features we haven't gotten to yet.


* src/backend/parser/gram.y

Same as above, in processCASbits(), you should add a similar check for
CAS_ENFORCED, meaning that for example specifying UNIQUE ENFORCED is
not allowed (even though it's the default).  This matches SQL standard
language: "If <unique constraint definition> is specified, then
<constraint characteristics> shall not specify a <constraint
enforcement>."


* src/backend/parser/parse_utilcmd.c

@@ -1317,6 +1321,7 @@ expandTableLikeClause(RangeVar *heapRel, 
TableLikeClause *table_like_clause)
             n->is_no_inherit = ccnoinherit;
             n->raw_expr = NULL;
             n->cooked_expr = nodeToString(ccbin_node);
+           n->is_enforced = true;

This has the effect that if you use the LIKE clause with INCLUDING
CONSTRAINTS, the new constraint is always ENFORCED.  Is this what we
want?  Did you have a reason?  I'm not sure what the ideal behavior
might be.  But if we want it like this, maybe we should document this
or at least put a comment here or something.


* src/backend/utils/adt/ruleutils.c

The syntax requires the NOT ENFORCED clause to be after DEFERRABLE
etc., but this code does it the other way around.  You should move the
new code after the switch statement and below the DEFERRABLE stuff.

I wouldn't worry about restricting it based on constraint type.  The
DEFERRABLE stuff doesn't do that either.  We can assume that the
catalog contents are sane.


* src/include/catalog/pg_constraint.h

There needs to be an update in catalogs.sgml for the new catalog column.


* src/test/regress/sql/constraints.sql

Possible additional test cases:
- trying [NOT] ENFORCED with a domain (CREATE and ALTER cases)
- trying [NOT] ENFORCED with an unsupported constraint type (maybe UNIQUE)


A note for the later patches: With patches 0001 through 0005 applied,
I get compiler warnings:

../src/backend/commands/tablecmds.c:10918:17: error: 'deleteTriggerOid' 
may be used uninitialized [-Werror=maybe-uninitialized]
../src/backend/commands/tablecmds.c:10918:17: error: 'updateTriggerOid' 
may be used uninitialized [-Werror=maybe-uninitialized]

(both with gcc and clang).

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.

On 03.12.24 13:00, Amul Sul wrote:
>>> 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.

Let's make it so that ruleutils.c doesn't print the NOT VALID when it's 
already printing NOT ENFORCED.  Otherwise, it gets unnecessarily verbose 
and confusing.

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

I was initially thinking about this as well, but after seeing it now, I 
don't think this is a good change.  Because now we have both "enforced" 
and "not_enforced" sprinkled around the code.  If we were to do this 
consistently everywhere, then it might make sense, but this way it's 
just confusing.  The Constraint struct is only initialized in a few 
places, so I think we can be careful there.  Also note that the field 
initially_valid is equally usually true.

I could of other notes on patch 0001:

Update information_schema table_constraint.enforced (see 
src/backend/catalog/information_schema.sql and 
doc/src/sgml/information_schema.sgml).

The handling of merging check constraints seems incomplete.  What should 
be the behavior of this:

=> create table p1 (a int check (a > 0) not enforced);
CREATE TABLE
=> create table c1 (a int check (a > 0) enforced) inherits (p1);
CREATE TABLE

Or this?

=> create table p2 (a int check (a > 0) enforced);
CREATE TABLE
=> create table c2 () inherits (p1, p2);
CREATE TABLE

Should we catch these and error?

i just only apply v5-0001 for now.

create table t(a int);
alter table t ADD CONSTRAINT cc CHECK (a > 0);
alter table t alter CONSTRAINT cc NOT ENFORCED;
alter table t alter CONSTRAINT cc ENFORCED;

the last two queries will fail, which means
ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [
INITIALLY DEFERRED | INITIALLY IMMEDIATE ] [ ENFORCED | NOT ENFORCED ]
in doc/src/sgml/ref/alter_table.sgml is not correct?
also no code change in ATExecAlterConstraint.

errmsg("cannot validated NOT ENFORCED constraint")));
should be
errmsg("cannot validate NOT ENFORCED constraint")));
?

typedef struct ConstrCheck
{
    char       *ccname;
    char       *ccbin;            /* nodeToString representation of expr */
    bool        ccenforced;
    bool        ccvalid;
    bool        ccnoinherit;    /* this is a non-inheritable constraint */
} ConstrCheck

ConstraintImpliedByRelConstraint,
get_relation_constraints
need skip notenforced check constraint?


put domain related tests from constraints.sql to domain.sql would be better.

>
> errmsg("cannot validated NOT ENFORCED constraint")));
> should be
> errmsg("cannot validate NOT ENFORCED constraint")));
> ?
>

looking at it again.

    if (!con->conenforced)
        ereport(ERROR,
                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                 errmsg("cannot validated NOT ENFORCED constraint")));

ERRCODE_WRONG_OBJECT_TYPE is not that ok? maybe
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
or
ERRCODE_INVALID_TABLE_DEFINITION


if (!con->conenforced)
       ereport(ERROR,
                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                 errmsg("cannot validated NOT ENFORCED constraint")));
if (!con->convalidated)
{
....
if (con->contype == CONSTRAINT_FOREIGN)
 {
            /*
             * Queue validation for phase 3 only if constraint is enforced;
             * otherwise, adding it to the validation queue won't be very
             * effective, as the verification will be skipped.
             */
            if (con->conenforced)
             ......
 }

in ATExecValidateConstraint "" if (con->conenforced)"""  will always be true?

On Wed, Dec 4, 2024 at 1:40 PM jian he <jian.universality@gmail.com> wrote:
>
> i just only apply v5-0001 for now.
>
> create table t(a int);
> alter table t ADD CONSTRAINT cc CHECK (a > 0);
> alter table t alter CONSTRAINT cc NOT ENFORCED;
> alter table t alter CONSTRAINT cc ENFORCED;
>
> the last two queries will fail, which means
> ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [
> INITIALLY DEFERRED | INITIALLY IMMEDIATE ] [ ENFORCED | NOT ENFORCED ]
> in doc/src/sgml/ref/alter_table.sgml is not correct?
> also no code change in ATExecAlterConstraint.
>

Your are correct, will move this to 0005 patch.

> errmsg("cannot validated NOT ENFORCED constraint")));
> should be
> errmsg("cannot validate NOT ENFORCED constraint")));
> ?
>

Yes, I realized that while working on Peter's last review comments.

> typedef struct ConstrCheck
> {
>     char       *ccname;
>     char       *ccbin;            /* nodeToString representation of expr */
>     bool        ccenforced;
>     bool        ccvalid;
>     bool        ccnoinherit;    /* this is a non-inheritable constraint */
> } ConstrCheck
>
> ConstraintImpliedByRelConstraint,
> get_relation_constraints
> need skip notenforced check constraint?
>

That gets skipped since ccvalid is false for NOT ENFORCED constraints.
However, for better readability, I've added an assertion with a
comment in my local changes.

>
> put domain related tests from constraints.sql to domain.sql would be better.

Ok.

> looking at it again.
>
>     if (!con->conenforced)
>         ereport(ERROR,
>                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                  errmsg("cannot validated NOT ENFORCED constraint")));
>
> ERRCODE_WRONG_OBJECT_TYPE is not that ok? maybe
> ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
> or
> ERRCODE_INVALID_TABLE_DEFINITION
>

I think ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE would be much suitable.

>
> if (!con->conenforced)
>        ereport(ERROR,
>                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                  errmsg("cannot validated NOT ENFORCED constraint")));
> if (!con->convalidated)
> {
> ....
> if (con->contype == CONSTRAINT_FOREIGN)
>  {
>             /*
>              * Queue validation for phase 3 only if constraint is enforced;
>              * otherwise, adding it to the validation queue won't be very
>              * effective, as the verification will be skipped.
>              */
>             if (con->conenforced)
>              ......
>  }
>
> in ATExecValidateConstraint "" if (con->conenforced)"""  will always be true?

Yes, the changes from that patch have been reverted in my local code, which
I will post soon.

Thanks again for your review comments; they were very helpful.

Regards,
Amul

hi.
accidentally hit segfault.
create table c11 (a int not enforced);
create table c11 (a int enforced);
we can solve it via the following or changing SUPPORTS_ATTRS accordingly.

diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index 5ab44149e5..fe1116c092 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3965,7 +3965,7 @@ transformConstraintAttrs(CreateStmtContext *cxt,
List *constraintList)
                                break;
                        case CONSTR_ATTR_ENFORCED:
-                               if (lastprimarycon &&
+                               if (lastprimarycon == NULL ||
                                        lastprimarycon->contype != CONSTR_CHECK)
                                        ereport(ERROR,

(errcode(ERRCODE_SYNTAX_ERROR),
@@ -3981,7 +3981,7 @@ transformConstraintAttrs(CreateStmtContext *cxt,
List *constraintList)
                                break;
                        case CONSTR_ATTR_NOT_ENFORCED:
-                               if (lastprimarycon &&
+                               if (lastprimarycon == NULL ||
                                        lastprimarycon->contype != CONSTR_CHECK)
                                        ereport(ERROR,

(errcode(ERRCODE_SYNTAX_ERROR),


ALTER DOMAIN constraint_comments_dom ADD CONSTRAINT the_constraint
CHECK (value > 0) NOT ENFORCED;
ERROR:  CHECK constraints cannot be marked NOT ENFORCED

the error message is not good?  maybe better option would be:
ERROR:  DOMAIN CHECK constraints cannot be marked NOT ENFORCED

we can do it like:
index 833b3be02b..4a7ab0c2a3 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4342,7 +4342,7 @@ DomainConstraintElem:
                                        n->location = @1;
                                        n->raw_expr = $3;
                                        n->cooked_expr = NULL;
-                                       processCASbits($5, @5, "CHECK",
+                                      processCASbits($5, @5, "DOMAIN CHECK",

NULL, NULL, NULL, &n->skip_validation,

&n->is_no_inherit, yyscanner);

On 2024-Dec-03, Peter Eisentraut wrote:

> The handling of merging check constraints seems incomplete.  What
> should be the behavior of this:
> 
> => create table p1 (a int check (a > 0) not enforced);
> CREATE TABLE
> => create table c1 (a int check (a > 0) enforced) inherits (p1);
> CREATE TABLE

Hmm.  Because the constraints are unnamed, and the chosen names are
different, I don't think they should be merged; I tried with 0001 in
place, and I think it does the right thing.  If c1's creation specifies
a name that matches the parent name, we get this:

55432 18devel 61349=# create table c1 (a int constraint p1_a_check check (a > 0)) inherits (p1);
NOTICE:  merging column "a" with inherited definition
ERROR:  constraint "p1_a_check" conflicts with NOT VALID constraint on relation "c1"

I think this is bogus on two counts.  First, NOT VALID has nowhere been
specified, so the error shouldn't be about that.  But second, the child
should have the constraint marked as enforced as requested, and marked
as conislocal=t, coninhcount=1; the user can turn it into NOT ENFORCED
if they want, and no expectation breaks, because the parent is also
already marked NOT ENFORCED.

The other way around shall not be accepted: if the parent has it as
ENFORCED, then the child is not allowed to have it as NOT ENFORCED,
neither during creation nor during ALTER TABLE.  The only way to mark
c1's constraint as NOT ENFORCED is to mark p1's constraint as NOINHERIT,
so that c1's constraint's inhcount becomes 0.  Then, the constraint has
no parent with an enforced constraint, so it's okay to mark it as not
enforced.

> Or this?
> 
> => create table p2 (a int check (a > 0) enforced);
> CREATE TABLE
> => create table c2 () inherits (p1, p2);
> CREATE TABLE
> 
> Should we catch these and error?

Here we end up with constraints p1_a_check and p2_a_check, which have
identical definitions except the NOT ENFORCED bits differ.  I think this
is okay, since we don't attempt to match these constraints when the
names differ.  If both parents had the constraint with the same name, we
should try to consider them as one and merge them.  In that case, c2's
constraint inhcount should be 2, and at least one of the parent
constraints is marked enforced, so the child shall have it as enforce
also.  Trying to mark c2's constraint as NOT ENFORCED shall give an
error because it inherits from p2.  But if you deinherit from p2, or
mark the constraint in p2 as NOINHERIT, then c2's constraint can become
NOT ENFORCE if the user asks for it.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Thu, Dec 5, 2024 at 11:02 AM jian he <jian.universality@gmail.com> wrote:
>
> hi.
> accidentally hit segfault.
> create table c11 (a int not enforced);
> create table c11 (a int enforced);
> we can solve it via the following or changing SUPPORTS_ATTRS accordingly.
>
> diff --git a/src/backend/parser/parse_utilcmd.c
> b/src/backend/parser/parse_utilcmd.c
> index 5ab44149e5..fe1116c092 100644
> --- a/src/backend/parser/parse_utilcmd.c
> +++ b/src/backend/parser/parse_utilcmd.c
> @@ -3965,7 +3965,7 @@ transformConstraintAttrs(CreateStmtContext *cxt,
> List *constraintList)
>                                 break;
>                         case CONSTR_ATTR_ENFORCED:
> -                               if (lastprimarycon &&
> +                               if (lastprimarycon == NULL ||
>                                         lastprimarycon->contype != CONSTR_CHECK)
>                                         ereport(ERROR,
>
> (errcode(ERRCODE_SYNTAX_ERROR),
> @@ -3981,7 +3981,7 @@ transformConstraintAttrs(CreateStmtContext *cxt,
> List *constraintList)
>                                 break;
>                         case CONSTR_ATTR_NOT_ENFORCED:
> -                               if (lastprimarycon &&
> +                               if (lastprimarycon == NULL ||
>                                         lastprimarycon->contype != CONSTR_CHECK)
>                                         ereport(ERROR,
>
> (errcode(ERRCODE_SYNTAX_ERROR),
>

Yes, that was a logical oversight on my part. Your suggestion looks
good to me, thanks.

>
> ALTER DOMAIN constraint_comments_dom ADD CONSTRAINT the_constraint
> CHECK (value > 0) NOT ENFORCED;
> ERROR:  CHECK constraints cannot be marked NOT ENFORCED
>
> the error message is not good?  maybe better option would be:
> ERROR:  DOMAIN CHECK constraints cannot be marked NOT ENFORCED
>
> we can do it like:
> index 833b3be02b..4a7ab0c2a3 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -4342,7 +4342,7 @@ DomainConstraintElem:
>                                         n->location = @1;
>                                         n->raw_expr = $3;
>                                         n->cooked_expr = NULL;
> -                                       processCASbits($5, @5, "CHECK",
> +                                      processCASbits($5, @5, "DOMAIN CHECK",
>
> NULL, NULL, NULL, &n->skip_validation,
>
> &n->is_no_inherit, yyscanner);

I believe this should either be a separate patch or potentially
included in your "Refactor AlterDomainAddConstraint" proposal[1].

Regards,
Amul

1] https://postgr.es/m/CACJufxHitd5LGLBSSAPShhtDWxT0ViVKTHinkYW-skBX93TcpA@mail.gmail.com

hi.
only applied v7-0001.

alter_table.sgml says we can specify enforceability
for  ALTER TABLE ADD column_constraint
and ALTER TABLE ADD column_constraint table_constraint.
but we didn't have a test for column_constraint in alter_table.sql

so segmental fault happened again:

create table tx(a int);
alter table tx add column b text collate "C" constraint cc check (a >
1) not enforced;
alter table tx add column b text collate "C" constraint cc check (b <>
'h') not enforced;
------------------------------------------------------------------------
errmsg("multiple ENFORCED/NOT ENFORCED clauses not allowed"),
never tested.
here are the tests:
CREATE TABLE t5(x int CHECK (x > 3) NOT ENFORCED enforced , b int);
CREATE TABLE t5(x int CHECK (x > 3) ENFORCED not enforced , b int);

------------------------------------------------------------------------
create foreign table with column_constraint, segmental fault also

reproduce:
DO $d$
    BEGIN
        EXECUTE $$CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
            OPTIONS (dbname '$$||current_database()||$$',
                     port '$$||current_setting('port')||$$'
            )$$;
        EXECUTE $$CREATE SERVER loopback2 FOREIGN DATA WRAPPER postgres_fdw
            OPTIONS (dbname '$$||current_database()||$$',
                     port '$$||current_setting('port')||$$'
            )$$;
        EXECUTE $$CREATE SERVER loopback3 FOREIGN DATA WRAPPER postgres_fdw
            OPTIONS (dbname '$$||current_database()||$$',
                     port '$$||current_setting('port')||$$'
            )$$;
    END;
$d$;
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
CREATE FOREIGN TABLE ft1 (c0 int constraint cc check (c0 > 1) not
enforced) SERVER loopback;

On Mon, Dec 9, 2024 at 9:40 PM jian he <jian.universality@gmail.com> wrote:
>
> hi.
> only applied v7-0001.
>
> alter_table.sgml says we can specify enforceability
> for  ALTER TABLE ADD column_constraint
> and ALTER TABLE ADD column_constraint table_constraint.
> but we didn't have a test for column_constraint in alter_table.sql
>
> so segmental fault happened again:
>

This is an assertion failure introduced by the patch to ensure that a
NOT ENFORCED constraint is marked as invalid. The failure occurs
because skip_validation and initially_valid were not set inside
transformConstraintAttrs(). I will post an updated version of the
patch tomorrow after conducting some additional testing. Thanks for
the test.

Regards,
Amul

hi. some minor issue about v7-0001.

there are 5 appearances of "sizeof(CookedConstraint)"
to make it safe, it would be nice to manual do
`
cooked->is_enforced = true;
`
for other kinds of constraints.


static bool
 MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
  bool allow_merge, bool is_local,
+ bool is_enforced,
  bool is_initially_valid,
  bool is_no_inherit)
 {
@@ -2729,12 +2738,24 @@ MergeWithExistingConstraint(Relation rel,
const char *ccname, Node *expr,
  * If the child constraint is "not valid" then cannot merge with a
  * valid parent constraint.
  */
- if (is_initially_valid && !con->convalidated)
+ if (is_initially_valid && con->conenforced && !con->convalidated)
  ereport(ERROR,
  (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
  errmsg("constraint \"%s\" conflicts with NOT VALID constraint on
relation \"%s\"",
  ccname, RelationGetRelationName(rel))));

There are no tests for this change. I think this change is not necessary.


- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
...
+ALTER TABLE attmp3 VALIDATE CONSTRAINT b_greater_than_ten_not_enforced; -- fail
+ERROR:  cannot validated NOT ENFORCED constraint

there should be
ERROR:  cannot validate NOT ENFORCED constraint
?

Do we need to update create_foreign_table.sgml
and alter_foreign_table.sgml?

On Tue, Dec 10, 2024 at 7:48 PM Amul Sul <sulamul@gmail.com> wrote:
>
> >
> > static bool
> >  MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
> >   bool allow_merge, bool is_local,
> > + bool is_enforced,
> >   bool is_initially_valid,
> >   bool is_no_inherit)
> >  {
> > @@ -2729,12 +2738,24 @@ MergeWithExistingConstraint(Relation rel,
> > const char *ccname, Node *expr,
> >   * If the child constraint is "not valid" then cannot merge with a
> >   * valid parent constraint.
> >   */
> > - if (is_initially_valid && !con->convalidated)
> > + if (is_initially_valid && con->conenforced && !con->convalidated)
> >   ereport(ERROR,
> >   (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> >   errmsg("constraint \"%s\" conflicts with NOT VALID constraint on
> > relation \"%s\"",
> >   ccname, RelationGetRelationName(rel))));
> >
> > There are no tests for this change. I think this change is not necessary.
> >
>
> It is necessary; otherwise, it would raise an error for a NOT ENFORCED
> constraint, which is NOT VALID by default.
>
got it.
overall v8-0001 looks good to me!

do you have a patch for
alter check constraint set [not] enforced?
If not, I will probably try to work on it.


I am playing around with the remaining patch.

ATExecAlterConstrRecurse
ATExecAlterConstrEnforceability
ATExecAlterChildConstr
AlterConstrTriggerDeferrability
These four functions take a lot of arguments.
more comments about these arguments would be helpful.
we only need to mention it at ATExecAlterConstrRecurse.

for example:
ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
                         const Oid fkrelid, const Oid pkrelid,
                         HeapTuple contuple, List **otherrelids,
                         LOCKMODE lockmode, Oid ReferencedParentDelTrigger,
                         Oid ReferencedParentUpdTrigger,
                         Oid ReferencingParentInsTrigger,
                         Oid ReferencingParentUpdTrigger)
the comments only explained otherrelids.

LOCKMODE lockmode,
Oid ReferencedParentDelTrigger,
Oid ReferencedParentUpdTrigger,
Oid ReferencingParentInsTrigger,
Oid ReferencingParentUpdTrigger

The above arguments are pretty intuitive.

Constraint *cmdcon
Relation conrel
Relation tgrel
HeapTuple contuple

but these arguments are not that very intuitive,
especially these arguments passing to another function.

On Wed, Dec 11, 2024 at 6:12 PM jian he <jian.universality@gmail.com> wrote:
>
> On Tue, Dec 10, 2024 at 7:48 PM Amul Sul <sulamul@gmail.com> wrote:
> >
> > >
> > > static bool
> > >  MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
> > >   bool allow_merge, bool is_local,
> > > + bool is_enforced,
> > >   bool is_initially_valid,
> > >   bool is_no_inherit)
> > >  {
> > > @@ -2729,12 +2738,24 @@ MergeWithExistingConstraint(Relation rel,
> > > const char *ccname, Node *expr,
> > >   * If the child constraint is "not valid" then cannot merge with a
> > >   * valid parent constraint.
> > >   */
> > > - if (is_initially_valid && !con->convalidated)
> > > + if (is_initially_valid && con->conenforced && !con->convalidated)
> > >   ereport(ERROR,
> > >   (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > >   errmsg("constraint \"%s\" conflicts with NOT VALID constraint on
> > > relation \"%s\"",
> > >   ccname, RelationGetRelationName(rel))));
> > >
> > > There are no tests for this change. I think this change is not necessary.
> > >
> >
> > It is necessary; otherwise, it would raise an error for a NOT ENFORCED
> > constraint, which is NOT VALID by default.
> >
> got it.
> overall v8-0001 looks good to me!
>

Thank you.

> do you have a patch for
> alter check constraint set [not] enforced?
> If not, I will probably try to work on it.
>

Not yet; I believe I need to first look into allowing NOT VALID
foreign key constraints on partitioned tables.

>
> I am playing around with the remaining patch.
>
> ATExecAlterConstrRecurse
> ATExecAlterConstrEnforceability
> ATExecAlterChildConstr
> AlterConstrTriggerDeferrability
> These four functions take a lot of arguments.
> more comments about these arguments would be helpful.
> we only need to mention it at ATExecAlterConstrRecurse.
>
> for example:
> ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
>                          const Oid fkrelid, const Oid pkrelid,
>                          HeapTuple contuple, List **otherrelids,
>                          LOCKMODE lockmode, Oid ReferencedParentDelTrigger,
>                          Oid ReferencedParentUpdTrigger,
>                          Oid ReferencingParentInsTrigger,
>                          Oid ReferencingParentUpdTrigger)
> the comments only explained otherrelids.
>
> LOCKMODE lockmode,
> Oid ReferencedParentDelTrigger,
> Oid ReferencedParentUpdTrigger,
> Oid ReferencingParentInsTrigger,
> Oid ReferencingParentUpdTrigger
>
> The above arguments are pretty intuitive.
>
> Constraint *cmdcon
> Relation conrel
> Relation tgrel
> HeapTuple contuple
>
> but these arguments are not that very intuitive,
> especially these arguments passing to another function.

Those are the existing ones; let me think what can be done with them.

Regards,
Amul