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

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.

On Tue, Oct 15, 2024 at 10:00 AM Amul Sul <sulamul@gmail.com> wrote:

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.

Attached is the rebased version on the latest master(5b0c46ea093),
including the aforesaid fix.

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 Tue, Nov 12, 2024 at 3:48 PM Peter Eisentraut <peter@eisentraut.org> wrote:

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.

Thank you for your feedback and suggestions.

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

True. I had a quick conversation with Álvaro at PGConf.EU about my
current work and the pending ALTER CONSTRAINT support for CHECK
constraints. He mentioned that we might also need the same support for
NULL constraints. I'll look into that as well while working on ALTER
CONSTRAINT.

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

Done.

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

Yeah, setting here is confusing, I removed that we do not need.

@@ -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()?

Yes, Removed this. Assertion in CreateConstraintEntry() is more than enough.

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

I revised the comment in the 0005 patch for clarity. My intent is
that, to trigger validation, the constraint must be enforced.
Additionally, when changing a constraint from non-enforced to
enforced, similar validation is triggered only if the constraint is
valid; otherwise, we simply update the constraint enforceability only.

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

Understood. Fixed.

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

Done. In processCASbits(), the error code will be
ERRCODE_FEATURE_NOT_SUPPORTED for consistency with the previous error.
Additionally, is_enforced = true is updated again in the CAS_ENFORCED
check block to align with the existing code style, which I believe is
reasonable.

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

You are correct; this is a bug. It has been fixed in the attached
version, and tests have been added for it.

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

Done.

* src/include/catalog/pg_constraint.h

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

Done.

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

Added. I thought about adding tests for all other constraints, but it
seemed excessive, so I decided not to.

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

For some reason, my GCC 11.5 on the CentOS machine isn’t showing this
warning. However, I found the unassigned variable, fixed it, and tried
compiling on macOS, where it's now clean.

Updated version attached.

Regards,
Amul

On Thu, Nov 14, 2024 at 7:10 PM Amul Sul <sulamul@gmail.com> wrote:

On Tue, Nov 12, 2024 at 3:48 PM Peter Eisentraut <peter@eisentraut.org> wrote:

Updated version attached.

In the attached version, did minor corrections in the document and
improved test coverage.

Regards,
Amul

On Fri, Nov 15, 2024 at 6:21 PM Amul Sul <sulamul@gmail.com> wrote:

Updated version attached.

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

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

+ 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"
------------------------------------------------------------------
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;
------------------------------------------------------------------
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...
------------------------------------------------------------------
do we need to update for "enforced" in
https://www.postgresql.org/docs/current/sql-keywords-appendix.html
?
------------------------------------------------------------------
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 add more tests where it should fail.
also add a test case for `create table like INCLUDING CONSTRAINTS`
please check attached.

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

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

On Tue, Dec 3, 2024 at 6:29 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 03.12.24 13:00, Amul Sul wrote:
[....]

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.

Ok, reverted and returned to using the is_enforced flag as before.

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

Done.

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 don't see any issue with treating ENFORCED and NOT ENFORCED as
distinct constraints if the names are different. However, if the names
are the same but the enforceability differs, I think we should throw
an error for now.

A better implementation I can think of would be to have behavior
similar to the NULL constraint: if the same column in one parent is
NOT NULL and another is not, the inherited child should always have
the NOT NULL column constraint, (in our case it should be ENFORCED),
eg: in the following case, c2 will have the column set as NOT NULL.

create table p1 (a int NOT NULL);
create table p2 (a int);
create table c2 () inherits (p1, p2);

But, I am a bit skeptical about following where it gets NOT NULL as
well but I expected it shouldn't be, is that right behaviour?
create table c3 (a int NULL) inherits (p1, p2);

So, if we want the child constraint enforceability to be overridden by
the child specification, we should handle it accordingly, unlike the
above NULL case. Thoughts?

Attached is the updated version that includes fixes based on Jian He's
review comments. 0005 is still WIP for the same reasons mentioned in
the v5 version. Alvaro also confirmed to me of-list, that the NOT
VALID FK constraint hasn't been implemented, and simply dropping the
restriction (the error check) may not be sufficient for its
implementation. I still need to investigate what else is required,
which is why this version remains WIP.

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] /messages/by-id/CACJufxHitd5LGLBSSAPShhtDWxT0ViVKTHinkYW-skBX93TcpA@mail.gmail.com