Re: NOT ENFORCED constraint feature

Hi,

In SQL Standard 2023, a table's check or referential constraint can be
either ENFORCED or NOT ENFORCED.

Currently, when a DML statement is executed, all enforced constraints
are validated. If any constraint is violated, an exception is raised,
and the SQL transaction is rolled back. These are referred to as
ENFORCED constraints.

On the other hand, a NOT ENFORCED constraint is a rule defined in the
database but not checked when data is inserted or updated. This can
help speed up large data imports, improve performance when strict
validation isn't required, or handle cases where constraints are
enforced externally (e.g., by application logic). It also allows the
rule to be documented without enforcing it during normal operations.

The attached patch proposes adding the ability to define CHECK and
FOREIGN KEY constraints as NOT ENFORCED. If neither ENFORCED nor
NOT ENFORCED is explicitly specified when defining a constraint, the
default setting is that the constraint is ENFORCED. Note that this
addition differs from the properties of NOT VALID and DEFERRABLE
constraints, which skip checks only for existing data and determine
when to perform checks, respectively. In contrast, NOT ENFORCED
completely skips the checks altogether.

Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch,
but implementing it for FOREIGN KEY constraints requires more code due
to triggers, see 0002 - 0005 patches. 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.

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.

Also note that the existing CHECK constraints currently do not support
ALTER operations. This functionality may be essential for modifying a
constraint's enforcement status; otherwise, users must drop and
recreate the CHECK constraint to change its enforceability. I have not
yet begun work on this, as it would involve significant code
refactoring and updates to the documentation. I plan to start this
once we finalise the design and reach a common understanding regarding
this proposal.

Any comments, suggestions, or assistance would be greatly appreciated.
Thank you.

--
Regards,
Amul Sul
EDB: http://www.enterprisedb.com

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(at)compiler(dot)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(at)dunslane(dot)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(at)gmail(dot)com> wrote:
>
> On Wed, Oct 9, 2024 at 2:44 PM Joel Jacobson <joel(at)compiler(dot)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(at)eisentraut(dot)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(at)gmail(dot)com> wrote:
>
> On Tue, Nov 12, 2024 at 3:48 PM Peter Eisentraut <peter(at)eisentraut(dot)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(at)gmail(dot)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
/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
> /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(at)eisentraut(dot)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
> > /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(dot)universality(at)gmail(dot)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(at)eisentraut(dot)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(dot)universality(at)gmail(dot)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

On Thu, Dec 5, 2024 at 4:40 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> 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.
>

Makes sense, agreed.

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

Agreed to this as well. I have made the changes to align with the
suggested behavior in the attached version. Thank you.

Regards,
Amul

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(dot)universality(at)gmail(dot)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 1:21 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> 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.
>

I am not sure if it's necessary, but it doesn't seem like a bad idea,
did the same in the attached version.

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

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

Are you sure you're looking at the latest patch? The error string is
already the same as you suggested.

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

Yes, I think we just need to add the syntax; a description isn't
necessary, imo, since constraints on foreign constraints are never
enforced.

Regards,
Amul

On Tue, Dec 10, 2024 at 7:48 PM Amul Sul <sulamul(at)gmail(dot)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(dot)universality(at)gmail(dot)com> wrote:
>
> On Tue, Dec 10, 2024 at 7:48 PM Amul Sul <sulamul(at)gmail(dot)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

Hi,
I have tested different scenarios involving CHECK constraint with NOT
ENFORCED specification on Inherited and Partitioned tables. Additionally, I
explored various situations with foreign key constraints. I have also
examined how pg_dump, pg_dumpall, pg_basebackup, and pg_upgrade handle NOT
ENFORCED constraints.

On Tue, Dec 17, 2024 at 12:23 PM tushar <tushar(dot)ahuja(at)enterprisedb(dot)com>
wrote:

> FYI
>
> ---------- Forwarded message ---------
> From: Amul Sul <sulamul(at)gmail(dot)com>
> Date: Tue, Dec 10, 2024 at 5:18 PM
> Subject: Re: NOT ENFORCED constraint feature
> To: jian he <jian(dot)universality(at)gmail(dot)com>
> Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Peter Eisentraut <
> peter(at)eisentraut(dot)org>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Joel
> Jacobson <joel(at)compiler(dot)org>
>
>
> On Tue, Dec 10, 2024 at 1:21 PM jian he <jian(dot)universality(at)gmail(dot)com>
> wrote:
> >
> > 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.
> >
>
> I am not sure if it's necessary, but it doesn't seem like a bad idea,
> did the same in the attached version.
>
> >
> > 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.
>
> >
> > - 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
> > ?
> >
>
> Are you sure you're looking at the latest patch? The error string is
> already the same as you suggested.
>
> > Do we need to update create_foreign_table.sgml
> > and alter_foreign_table.sgml?
>
> Yes, I think we just need to add the syntax; a description isn't
> necessary, imo, since constraints on foreign constraints are never
> enforced.
>
> Regards,
> Amul
>

--
Warm regards,
Triveni

I have applied v8-0001, with some editing of the documentation and in
the tests. I'll continue reviewing the subsequent patches.

On Saturday, 11 January 2025, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> I have applied v8-0001, with some editing of the documentation and in the
> tests. I'll continue reviewing the subsequent patches.
>

Thank you for the improvement and commit.

Regards,
Amul

--
Regards,
Amul Sul
EDB: http://www.enterprisedb.com

On 11.01.25 18:26, Amul Sul wrote:
> On Saturday, 11 January 2025, Peter Eisentraut <peter(at)eisentraut(dot)org
> <mailto:peter(at)eisentraut(dot)org>> wrote:
>
> I have applied v8-0001, with some editing of the documentation and
> in the tests.  I'll continue reviewing the subsequent patches.
>
>
> Thank you for the improvement and commit.

I have also committed v8-0002, the refactor of ATExecAlterConstrRecurse().

All the remaining patches go together, so w are now waiting on an
updated patch set from you.

On Thu, Jan 16, 2025 at 6:07 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 11.01.25 18:26, Amul Sul wrote:
> > On Saturday, 11 January 2025, Peter Eisentraut <peter(at)eisentraut(dot)org
> > <mailto:peter(at)eisentraut(dot)org>> wrote:
> >
> > I have applied v8-0001, with some editing of the documentation and
> > in the tests. I'll continue reviewing the subsequent patches.
> >
> >
> > Thank you for the improvement and commit.
>
> I have also committed v8-0002, the refactor of ATExecAlterConstrRecurse().
>
> All the remaining patches go together, so w are now waiting on an
> updated patch set from you.
>

Thanks!

Yes, I'm working on it and will post it tomorrow.

Regards,
Amul

On Thu, Jan 16, 2025 at 6:55 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Thu, Jan 16, 2025 at 6:07 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> >
> > On 11.01.25 18:26, Amul Sul wrote:
> > > On Saturday, 11 January 2025, Peter Eisentraut <peter(at)eisentraut(dot)org
> > > <mailto:peter(at)eisentraut(dot)org>> wrote:
> > >
> > > I have applied v8-0001, with some editing of the documentation and
> > > in the tests. I'll continue reviewing the subsequent patches.
> > >
> > >
> > > Thank you for the improvement and commit.
> >
> > I have also committed v8-0002, the refactor of ATExecAlterConstrRecurse().
> >
> > All the remaining patches go together, so w are now waiting on an
> > updated patch set from you.
> >
>
> Thanks!
>
> Yes, I'm working on it and will post it tomorrow.
>

Attached is a new set of patches. Please ignore patch 0001 here, which
was posted separately [1] -- proposes allowing invalid foreign key
constraints on partitioned tables. Patch 0002 refactors
tryAttachPartitionForeignKey(), primarily needed by the new patch
v9-0006 included in this set. This patch handles merging constraints
with different enforceability. Without it, a new constraint would be
created on the child. However, this patch introduces additional code
that may appear somewhat messy or confusing. I've added plenty of
comments to clarify the logic. While I’ve done some testing, it hasn’t
been extensive. I plan to do more testing in the next week.

Please let me know if we should continue with patch 0006 improvement,
or if the feature up to patch 0005, which enforces matching
enforceability before merging constraints, is sufficient.

1] https://postgr.es/m/CAAJ_b96Bp=-ZwihPPtuaNX=SrZ0U6ZsXD3+fgARO0JuKa8v2jQ@mail.gmail.com

Regards,
Amul

On Fri, Jan 17, 2025 at 6:20 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Thu, Jan 16, 2025 at 6:55 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> >
> > On Thu, Jan 16, 2025 at 6:07 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> > >
> > > On 11.01.25 18:26, Amul Sul wrote:
> > > > On Saturday, 11 January 2025, Peter Eisentraut <peter(at)eisentraut(dot)org
> > > > <mailto:peter(at)eisentraut(dot)org>> wrote:
> > > >
> > > > I have applied v8-0001, with some editing of the documentation and
> > > > in the tests. I'll continue reviewing the subsequent patches.
> > > >
> > > >
> > > > Thank you for the improvement and commit.
> > >
> > > I have also committed v8-0002, the refactor of ATExecAlterConstrRecurse().
> > >
> > > All the remaining patches go together, so w are now waiting on an
> > > updated patch set from you.
> > >
> >
> > Thanks!
> >
> > Yes, I'm working on it and will post it tomorrow.
> >
>
> Attached is a new set of patches. Please ignore patch 0001 here, which
> was posted separately [1] -- proposes allowing invalid foreign key
> constraints on partitioned tables. Patch 0002 refactors
> tryAttachPartitionForeignKey(), primarily needed by the new patch
> v9-0006 included in this set. This patch handles merging constraints
> with different enforceability. Without it, a new constraint would be
> created on the child. However, this patch introduces additional code
> that may appear somewhat messy or confusing. I've added plenty of
> comments to clarify the logic. While I’ve done some testing, it hasn’t
> been extensive. I plan to do more testing in the next week.
>
> Please let me know if we should continue with patch 0006 improvement,
> or if the feature up to patch 0005, which enforces matching
> enforceability before merging constraints, is sufficient.
>
> 1] https://postgr.es/m/CAAJ_b96Bp=-ZwihPPtuaNX=SrZ0U6ZsXD3+fgARO0JuKa8v2jQ@mail.gmail.com
>

I made minor updates to the attached version, particularly ensuring
that the order of relation opening and closing remains the same as
before in ATExecAlterConstrRecurse(). Additionally, I’ve added a
refactoring patch to make createForeignKeyActionTriggers() accept a
relation OID instead of a Relation, making this function consistent
with others like createForeignKeyCheckTriggers().

Regards,
Amul

On 20.01.25 17:53, Amul Sul wrote:
>> Attached is a new set of patches. Please ignore patch 0001 here, which
>> was posted separately [1] -- proposes allowing invalid foreign key
>> constraints on partitioned tables. Patch 0002 refactors
>> tryAttachPartitionForeignKey(), primarily needed by the new patch
>> v9-0006 included in this set. This patch handles merging constraints
>> with different enforceability. Without it, a new constraint would be
>> created on the child. However, this patch introduces additional code
>> that may appear somewhat messy or confusing. I've added plenty of
>> comments to clarify the logic. While I’ve done some testing, it hasn’t
>> been extensive. I plan to do more testing in the next week.
>>
>> Please let me know if we should continue with patch 0006 improvement,
>> or if the feature up to patch 0005, which enforces matching
>> enforceability before merging constraints, is sufficient.
>>
>> 1] https://postgr.es/m/CAAJ_b96Bp=-ZwihPPtuaNX=SrZ0U6ZsXD3+fgARO0JuKa8v2jQ@mail.gmail.com
>>
>
> I made minor updates to the attached version, particularly ensuring
> that the order of relation opening and closing remains the same as
> before in ATExecAlterConstrRecurse(). Additionally, I’ve added a
> refactoring patch to make createForeignKeyActionTriggers() accept a
> relation OID instead of a Relation, making this function consistent
> with others like createForeignKeyCheckTriggers().

I think v10-0001 has been committed separately now. I can't
successfully apply the remaining patches though. Could you supply an
updated patch set?

Just from looking at them, the refactoring patches 0002, 0003, 0004 seem ok.

0005 also seems ok.

In 0006, this change in the test output should be improved:

-- XXX: error message is misleading here
ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
-ERROR: ALTER CONSTRAINT statement constraints cannot be marked ENFORCED
-LINE 1: ...TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
- ^
+ERROR: constraint "unique_tbl_i_key" of relation "unique_tbl" is not a
foreign key constraint

Maybe this should be along the lines of "ALTER CONSTRAINT ... ENFORCED
is not supported for %s constraints" or something like that.

This behavior is not correct:

+-- Changing it back to ENFORCED will leave the constraint in the NOT
VALID state
+ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
+-- Which needs to be explicitly validated.
+ALTER TABLE FKTABLE VALIDATE CONSTRAINT fktable_ftest1_fkey;

Setting the constraint to enforced should enforce it immediately. This
SQL statement is covered by the SQL standard. Also, I think it's a
better user experience if you don't require two steps.

On Tue, Jan 28, 2025 at 4:01 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 20.01.25 17:53, Amul Sul wrote:
> >> Attached is a new set of patches. Please ignore patch 0001 here, which
> >> was posted separately [1] -- proposes allowing invalid foreign key
> >> constraints on partitioned tables. Patch 0002 refactors
> >> tryAttachPartitionForeignKey(), primarily needed by the new patch
> >> v9-0006 included in this set. This patch handles merging constraints
> >> with different enforceability. Without it, a new constraint would be
> >> created on the child. However, this patch introduces additional code
> >> that may appear somewhat messy or confusing. I've added plenty of
> >> comments to clarify the logic. While I’ve done some testing, it hasn’t
> >> been extensive. I plan to do more testing in the next week.
> >>
> >> Please let me know if we should continue with patch 0006 improvement,
> >> or if the feature up to patch 0005, which enforces matching
> >> enforceability before merging constraints, is sufficient.
> >>
> >> 1] https://postgr.es/m/CAAJ_b96Bp=-ZwihPPtuaNX=SrZ0U6ZsXD3+fgARO0JuKa8v2jQ@mail.gmail.com
> >>
> >
> > I made minor updates to the attached version, particularly ensuring
> > that the order of relation opening and closing remains the same as
> > before in ATExecAlterConstrRecurse(). Additionally, I’ve added a
> > refactoring patch to make createForeignKeyActionTriggers() accept a
> > relation OID instead of a Relation, making this function consistent
> > with others like createForeignKeyCheckTriggers().
>
> I think v10-0001 has been committed separately now. I can't
> successfully apply the remaining patches though. Could you supply an
> updated patch set?
>

Sure, I plan to work on that tomorrow.

> Just from looking at them, the refactoring patches 0002, 0003, 0004 seem ok.
>
> 0005 also seems ok.
>

Thank you.

>
> In 0006, this change in the test output should be improved:
>
> -- XXX: error message is misleading here
> ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
> -ERROR: ALTER CONSTRAINT statement constraints cannot be marked ENFORCED
> -LINE 1: ...TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
> - ^
> +ERROR: constraint "unique_tbl_i_key" of relation "unique_tbl" is not a
> foreign key constraint
>
> Maybe this should be along the lines of "ALTER CONSTRAINT ... ENFORCED
> is not supported for %s constraints" or something like that.
>

Ok, let me see what can be done here.

>
> This behavior is not correct:
>
> +-- Changing it back to ENFORCED will leave the constraint in the NOT
> VALID state
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
> +-- Which needs to be explicitly validated.
> +ALTER TABLE FKTABLE VALIDATE CONSTRAINT fktable_ftest1_fkey;
>
> Setting the constraint to enforced should enforce it immediately. This
> SQL statement is covered by the SQL standard. Also, I think it's a
> better user experience if you don't require two steps.
>

Let me clarify: the constraint will be enforced for new inserts and
updates, but it won't be validated against existing data, so those
will remain marked as invalid.

Regards,

On 28.01.25 11:58, Amul Sul wrote:
>> This behavior is not correct:
>>
>> +-- Changing it back to ENFORCED will leave the constraint in the NOT
>> VALID state
>> +ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
>> +-- Which needs to be explicitly validated.
>> +ALTER TABLE FKTABLE VALIDATE CONSTRAINT fktable_ftest1_fkey;
>>
>> Setting the constraint to enforced should enforce it immediately. This
>> SQL statement is covered by the SQL standard. Also, I think it's a
>> better user experience if you don't require two steps.
>>
> Let me clarify: the constraint will be enforced for new inserts and
> updates, but it won't be validated against existing data, so those
> will remain marked as invalid.

Yes, I understand, but that is the not the correct behavior of this
command per SQL standard.

On Tue, Jan 28, 2025 at 9:47 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> > In 0006, this change in the test output should be improved:
> >
> > -- XXX: error message is misleading here
> > ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
> > -ERROR: ALTER CONSTRAINT statement constraints cannot be marked ENFORCED
> > -LINE 1: ...TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
> > - ^
> > +ERROR: constraint "unique_tbl_i_key" of relation "unique_tbl" is not a
> > foreign key constraint
> >
> > Maybe this should be along the lines of "ALTER CONSTRAINT ... ENFORCED
> > is not supported for %s constraints" or something like that.
> >
>
> Ok, let me see what can be done here.

I tried to improve the error message by adding the following details
for this case in the attached version:

+ERROR: cannot alter enforceability of constraint "unique_tbl_i_key"
of relation "unique_tbl"
+DETAIL: Enforceability can only be altered for foreign key constraints.

> On 28.01.25 11:58, Amul Sul wrote:
> >> This behavior is not correct:
> >>
> >> +-- Changing it back to ENFORCED will leave the constraint in the NOT
> >> VALID state
> >> +ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
> >> +-- Which needs to be explicitly validated.
> >> +ALTER TABLE FKTABLE VALIDATE CONSTRAINT fktable_ftest1_fkey;
> >>
> >> Setting the constraint to enforced should enforce it immediately. This
> >> SQL statement is covered by the SQL standard. Also, I think it's a
> >> better user experience if you don't require two steps.
> >>
> > Let me clarify: the constraint will be enforced for new inserts and
> > updates, but it won't be validated against existing data, so those
> > will remain marked as invalid.
>
> Yes, I understand, but that is the not the correct behavior of this
> command per SQL standard.

I haven't worked on this yet due to a lack of clarity, and it requires
further discussion. The behavior will remain the same as the previous
version. The attached set is the rebased version on top of the latest
master head and includes the aforementioned error message
improvements.

Regards,
Amul

On Wed, Jan 29, 2025 at 6:18 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Tue, Jan 28, 2025 at 9:47 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> >
> > > In 0006, this change in the test output should be improved:
> > >
> > > -- XXX: error message is misleading here
> > > ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
> > > -ERROR: ALTER CONSTRAINT statement constraints cannot be marked ENFORCED
> > > -LINE 1: ...TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
> > > - ^
> > > +ERROR: constraint "unique_tbl_i_key" of relation "unique_tbl" is not a
> > > foreign key constraint
> > >
> > > Maybe this should be along the lines of "ALTER CONSTRAINT ... ENFORCED
> > > is not supported for %s constraints" or something like that.
> > >
> >
> > Ok, let me see what can be done here.
>
> I tried to improve the error message by adding the following details
> for this case in the attached version:
>
> +ERROR: cannot alter enforceability of constraint "unique_tbl_i_key"
> of relation "unique_tbl"
> +DETAIL: Enforceability can only be altered for foreign key constraints.
>
> > On 28.01.25 11:58, Amul Sul wrote:
> > >> This behavior is not correct:
> > >>
> > >> +-- Changing it back to ENFORCED will leave the constraint in the NOT
> > >> VALID state
> > >> +ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
> > >> +-- Which needs to be explicitly validated.
> > >> +ALTER TABLE FKTABLE VALIDATE CONSTRAINT fktable_ftest1_fkey;
> > >>
> > >> Setting the constraint to enforced should enforce it immediately. This
> > >> SQL statement is covered by the SQL standard. Also, I think it's a
> > >> better user experience if you don't require two steps.
> > >>
> > > Let me clarify: the constraint will be enforced for new inserts and
> > > updates, but it won't be validated against existing data, so those
> > > will remain marked as invalid.
> >
> > Yes, I understand, but that is the not the correct behavior of this
> > command per SQL standard.

If the constraint is VALID and later marked as NOT ENFORCED, changing
it to ENFORCED should also keep it VALID. But if the constraint is NOT
VALID and later marked as NOT ENFORCED, what is expected behaviour
while changing it to ENFORCED? Should it be kept NOT VALID or it
should turn into VALID? I think a user would expect it to be NOT
VALID. When we didn't support the ENFORCED/NOT ENFORCED option, we had
NOT VALID + ENFORCED behaviour.

Now the problem I see is when we set NOT ENFORCED, the constraint is
also set to NOT VALID, which is arguable. When a user sets a
constraint as NOT ENFORCED, it's their responsibility to make sure
that the data still fits the constraint. In that sense the constraint
is VALID and we shouldn't convert it to NOT VALID. We should validate
all the data when changing a NOT ENFORCED, VALID constraint to
ENFORCED, VALID so that the VALID status is reliable.

--
Best Wishes,
Ashutosh Bapat

On 2025-Jan-31, Ashutosh Bapat wrote:

> But if the constraint is NOT VALID and later marked as NOT ENFORCED,
> what is expected behaviour while changing it to ENFORCED?

I think what you want is a different mode that would be ENFORCED NOT
VALID, which would be an extension of the standard, because the standard
does not support the concept of NOT VALID. So while I think what you
want is nice, I'm not sure that this patch necessarily must implement
it.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

hi.
after applying the v11-0002 to v11-0006.
there is a bug in ATExecAlterConstrRecurse, i think.

in ATExecAlterConstrRecurse, after applying the patch, the code is

if (currcon->condeferrable != cmdcon->deferrable ||
currcon->condeferred != cmdcon->initdeferred ||
currcon->conenforced != cmdcon->is_enforced)
{
}
if (currcon->conenforced != cmdcon->is_enforced)
{
ATExecAlterConstrEnforceability
}
else
{
AlterConstrTriggerDeferrability...
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)
ATExecAlterChildConstr(cmdcon, conrel, tgrel, fkrelid, pkrelid,
contuple, otherrelids, lockmode);
}

drop table if exists PKTABLE, fktable cascade;
CREATE TABLE PKTABLE (ptest1 int PRIMARY KEY, ptest2 text);
CREATE TABLE FKTABLE (ftest1 int REFERENCES PKTABLE MATCH FULL ON
DELETE CASCADE ON UPDATE CASCADE NOT ENFORCED,
ftest2 int);
ALTER TABLE fktable ALTER CONSTRAINT fktable_ftest1_fkey deferrable;

\d fktable
Table "public.fktable"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
ftest1 | integer | | |
ftest2 | integer | | |
Foreign-key constraints:
"fktable_ftest1_fkey" FOREIGN KEY (ftest1) REFERENCES
pktable(ptest1) MATCH FULL ON UPDATE CASCADE ON DELETE CASCADE
DEFERRABLE NOT VALID

Currently "ALTER TABLE fktable ALTER CONSTRAINT fktable_ftest1_fkey deferrable;"
imply the constraint fktable_ftest1_fkey is changing from "not
enforced" to "enforced".
but here we didn't explicitly mean to change the "enforced" status.
We only want to change the deferriability.

So the code should only call AlterConstrTriggerDeferrability,
not call ATExecAlterConstrEnforceability?

On Sat, Feb 1, 2025 at 8:31 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> [...]
> So the code should only call AlterConstrTriggerDeferrability,
> not call ATExecAlterConstrEnforceability?

Right. Thank you for the report. We need to know whether the
enforceability and/or deferability has actually been set or not before
catalog update.

Have you started working on the ALTER ... CONSTRAINT for the check
constraint? I am thinking to start that. To fix this bug, we have two
options: we could either throw an error as we don’t currently support
altering enforceability and deferability together (which I’m not a fan
of), or we could refactor the ALTER ... CONSTRAINT code to include
more information which would allow us to perform the appropriate
update action during the execution stage, and it would also help with
altering check constraints.

Regards,
Amul

On Fri, Jan 31, 2025 at 7:10 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Jan-31, Ashutosh Bapat wrote:
>
> > But if the constraint is NOT VALID and later marked as NOT ENFORCED,
> > what is expected behaviour while changing it to ENFORCED?
>
> I think what you want is a different mode that would be ENFORCED NOT
> VALID, which would be an extension of the standard, because the standard
> does not support the concept of NOT VALID. So while I think what you
> want is nice, I'm not sure that this patch necessarily must implement
> it.
>

Here is my understanding behind this feature implementation -- I am
not claiming to be 100% correct, I am confident I am not entirely
wrong either. Let me explain with an example: imagine a user adds a
VALID constraint to a table that already has data, and the user is
completely sure that all the data complies with the constraint. Even
in this case, the system still runs a validation check. This is
expected behavior because the system can't just take the user's word
for it -- it needs to explicitly confirm that the data is valid
through validation.

Now, with a NOT ENFORCED constraint, it's almost like the constraint
doesn't exist, because no checks are being performed and there is no
visible effect for the user, even though the constraint is technically
still there. So when the constraint is switched to ENFORCED, we should
be careful not to automatically mark it as validated (regardless of
its previous validate status) unless the data is actually checked
against the constraint -- treat as adding a new VALID constraint. Even
if the user is absolutely sure the data complies, we should still run
the validation to ensure reliability.

In response to Ashutosh’s point about the VALID/NOT ENFORCED scenario:
if a constraint is initially VALID, then marked as NOT ENFORCED, and
later switched back to ENFORCED -- IMO, it shouldn't automatically be
considered VALID. I had similar thoughts when working on a patch
before v5, but after further discussion, I now agree that it makes
more sense for the system to keep it as NOT VALID until the data has
been validated against the constraint. This is especially important
when a user adds the constraint, is confident the data complies, and
then needs to enforce it. Validating the data ensures the integrity
and consistency of the constraint.

In short, even if the user is 100% confident, skipping validation is
not an option. We need to make sure the constraint’s VALID status is
accurate and reliable before marking it as validated.

Regards,
Amul

On Mon, Feb 3, 2025 at 9:57 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Fri, Jan 31, 2025 at 7:10 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > On 2025-Jan-31, Ashutosh Bapat wrote:
> >
> > > But if the constraint is NOT VALID and later marked as NOT ENFORCED,
> > > what is expected behaviour while changing it to ENFORCED?
> >
> > I think what you want is a different mode that would be ENFORCED NOT
> > VALID, which would be an extension of the standard, because the standard
> > does not support the concept of NOT VALID. So while I think what you
> > want is nice, I'm not sure that this patch necessarily must implement
> > it.
> >

This way allows VALID/NOT VALID and ENFORCED/NOT ENFORCED states to
work together and also implement behaviour specified by the standard
(ref. Peter's email). If there's some other way to implement the
behaviour, that's fine too.

>
> Here is my understanding behind this feature implementation -- I am
> not claiming to be 100% correct, I am confident I am not entirely
> wrong either. Let me explain with an example: imagine a user adds a
> VALID constraint to a table that already has data, and the user is
> completely sure that all the data complies with the constraint. Even
> in this case, the system still runs a validation check. This is
> expected behavior because the system can't just take the user's word
> for it -- it needs to explicitly confirm that the data is valid
> through validation.
>
> Now, with a NOT ENFORCED constraint, it's almost like the constraint
> doesn't exist, because no checks are being performed and there is no
> visible effect for the user, even though the constraint is technically
> still there. So when the constraint is switched to ENFORCED, we should
> be careful not to automatically mark it as validated (regardless of
> its previous validate status) unless the data is actually checked
> against the constraint -- treat as adding a new VALID constraint. Even
> if the user is absolutely sure the data complies, we should still run
> the validation to ensure reliability.
>
> In response to Ashutosh’s point about the VALID/NOT ENFORCED scenario:
> if a constraint is initially VALID, then marked as NOT ENFORCED, and
> later switched back to ENFORCED -- IMO, it shouldn't automatically be
> considered VALID.

I am suggesting that when a constraint is changed from NOT ENFORCED to
ENFORCED, if it's marked VALID - we run validation checks.

Here's how I see the state conversions happening.

NOT VALID, NOT ENFORCED changed to NOT_VALID, ENFORCED - no data
validation required, constraint is enforced on the new tuples/changes
NOT VALID, ENFORCED changed to NOT VALID, NOT ENFORCED - no data
validation, constraint isn't enforced anymore
VALID, NOT ENFORCED changed to VALID, ENFORCED - data validation
required, constraint is enforced
VALID, ENFORCED changed to VALID, NOT ENFORCED - no data validation
required, constrain isn't enforced anymore, we rely on user to enforce
the constraint on their side

--
Best Wishes,
Ashutosh Bapat

On Mon, Feb 3, 2025 at 10:49 AM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Mon, Feb 3, 2025 at 9:57 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> >
> > On Fri, Jan 31, 2025 at 7:10 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > >
> > > On 2025-Jan-31, Ashutosh Bapat wrote:
> > >
> > > > But if the constraint is NOT VALID and later marked as NOT ENFORCED,
> > > > what is expected behaviour while changing it to ENFORCED?
> > >
> > > I think what you want is a different mode that would be ENFORCED NOT
> > > VALID, which would be an extension of the standard, because the standard
> > > does not support the concept of NOT VALID. So while I think what you
> > > want is nice, I'm not sure that this patch necessarily must implement
> > > it.
> > >
>
> This way allows VALID/NOT VALID and ENFORCED/NOT ENFORCED states to
> work together and also implement behaviour specified by the standard
> (ref. Peter's email). If there's some other way to implement the
> behaviour, that's fine too.
>
> >
> > Here is my understanding behind this feature implementation -- I am
> > not claiming to be 100% correct, I am confident I am not entirely
> > wrong either. Let me explain with an example: imagine a user adds a
> > VALID constraint to a table that already has data, and the user is
> > completely sure that all the data complies with the constraint. Even
> > in this case, the system still runs a validation check. This is
> > expected behavior because the system can't just take the user's word
> > for it -- it needs to explicitly confirm that the data is valid
> > through validation.
> >
> > Now, with a NOT ENFORCED constraint, it's almost like the constraint
> > doesn't exist, because no checks are being performed and there is no
> > visible effect for the user, even though the constraint is technically
> > still there. So when the constraint is switched to ENFORCED, we should
> > be careful not to automatically mark it as validated (regardless of
> > its previous validate status) unless the data is actually checked
> > against the constraint -- treat as adding a new VALID constraint. Even
> > if the user is absolutely sure the data complies, we should still run
> > the validation to ensure reliability.
> >
> > In response to Ashutosh’s point about the VALID/NOT ENFORCED scenario:
> > if a constraint is initially VALID, then marked as NOT ENFORCED, and
> > later switched back to ENFORCED -- IMO, it shouldn't automatically be
> > considered VALID.
>
> I am suggesting that when a constraint is changed from NOT ENFORCED to
> ENFORCED, if it's marked VALID - we run validation checks.
>

Ok.

> Here's how I see the state conversions happening.
>
> NOT VALID, NOT ENFORCED changed to NOT_VALID, ENFORCED - no data
> validation required, constraint is enforced on the new tuples/changes
> NOT VALID, ENFORCED changed to NOT VALID, NOT ENFORCED - no data
> validation, constraint isn't enforced anymore
> VALID, NOT ENFORCED changed to VALID, ENFORCED - data validation
> required, constraint is enforced
> VALID, ENFORCED changed to VALID, NOT ENFORCED - no data validation
> required, constrain isn't enforced anymore, we rely on user to enforce
> the constraint on their side
>

Understood, thanks for the detailed explanation. This is what I had
implemented in the v4 patch, and I agree with this. If we decide to go
with this, I can revert the behavior to the v4 patch set.

Regards,
Amul

On 2025-Feb-03, Ashutosh Bapat wrote:

> VALID, NOT ENFORCED changed to VALID, ENFORCED - data validation
> required, constraint is enforced

There's no such thing as a VALID NOT ENFORCED constraint. It just
cannot exist.

> NOT VALID, NOT ENFORCED changed to NOT_VALID, ENFORCED - no data
> validation required, constraint is enforced on the new tuples/changes

This may make sense, but it needs special nonstandard syntax. If you
start with a NOT VALID NOT ENFORCED constraint (which is the only way to
have a NOT ENFORCED constraint) and apply ALTER TABLE ALTER CONSTRAINT
ENFORCE, you will end up with a VALID ENFORCED constraint, therefore
validation must be run.

If you wanted to add a nonstandard command
ALTER TABLE ALTER CONSTRAINT ENFORCE NO VALIDATE
then maybe the transition you suggest could be made.
It should be a separate patch from regular ALTER CONSTRAINT ENFORCE
though, just in case some problems with it emerge later and we're forced
to revert it, we can still keep the standard command.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Use it up, wear it out, make it do, or do without"

On Mon, Feb 3, 2025 at 1:20 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Feb-03, Ashutosh Bapat wrote:
>
> > VALID, NOT ENFORCED changed to VALID, ENFORCED - data validation
> > required, constraint is enforced
>
> There's no such thing as a VALID NOT ENFORCED constraint. It just
> cannot exist.

The document in the patch says
```
If the
constraint is <literal>NOT ENFORCED</literal>, the database system will
not check the constraint. It is then up to the application code to
ensure that the constraints are satisfied. The database system might
still assume that the data actually satisfies the constraint for
optimization decisions where this does not affect the correctness of the
result.
```
If a constraint is NOT VALID, NOT ENFORCED it can't be used for
optimization. Constraints which are VALID, NOT ENFORCED can be used
for optimizatin. That's a correct state if the application is
faithfully making sure that the constraint is satisfied, as suggested
in our documentation. Otherwise, I don't see how NOT ENFORCED
constraints would be useful.

>
> > NOT VALID, NOT ENFORCED changed to NOT_VALID, ENFORCED - no data
> > validation required, constraint is enforced on the new tuples/changes
>
> This may make sense, but it needs special nonstandard syntax. If you
> start with a NOT VALID NOT ENFORCED constraint (which is the only way to
> have a NOT ENFORCED constraint) and apply ALTER TABLE ALTER CONSTRAINT
> ENFORCE, you will end up with a VALID ENFORCED constraint, therefore
> validation must be run.
>
> If you wanted to add a nonstandard command
> ALTER TABLE ALTER CONSTRAINT ENFORCE NO VALIDATE

Which state transition needs it? ALTER TABLE ALTER CONSTRAINT ENFORCE
is enough to change NOT VALID, NOT ENFORCED constraint to NOT VALID,
ENFORCED constraint; it does not need NO VALIDATE.

--
Best Wishes,
Ashutosh Bapat

On 2025-Feb-03, Ashutosh Bapat wrote:

> ```
> If the
> constraint is <literal>NOT ENFORCED</literal>, the database system will
> not check the constraint. It is then up to the application code to
> ensure that the constraints are satisfied. The database system might
> still assume that the data actually satisfies the constraint for
> optimization decisions where this does not affect the correctness of the
> result.
> ```

IMO the third sentence should be removed because it is bogus. There's
no situation in which a not-enforced constraint can be used for any
query optimizations -- you cannot know if a constraint remains valid
after it's been turned NOT ENFORCED, because anyone could insert data
that violates it milliseconds after it stops being enforced. I think
the expectation that the application is going to correctly enforce the
constraint after it's told the database server not to enforce it, is
going to be problematic. As I recall, we already do this in FDWs for
instance and it's already a problem.

The second sentence is also somewhat bogus, but not as much, because it
doesn't have any implications for the database system. I think we
should simply say that no assumptions can be made about not enforced
constraints from the server side and that if the user wants to enforce
these at the application side, it's up to them to keep it all straight.

IMO if the patch is letting constraints that are marked NOT ENFORCED
continue to be used for query optimization, the patch is bogus and
should be fixed. For instance, I think CheckConstraintFetch() should
skip adding to TupleDesc->constr any constraints that are marked as not
enforced.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Mon, Feb 3, 2025 at 5:55 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Feb-03, Ashutosh Bapat wrote:
>
> > ```
> > If the
> > constraint is <literal>NOT ENFORCED</literal>, the database system will
> > not check the constraint. It is then up to the application code to
> > ensure that the constraints are satisfied. The database system might
> > still assume that the data actually satisfies the constraint for
> > optimization decisions where this does not affect the correctness of the
> > result.
> > ```
>
> IMO the third sentence should be removed because it is bogus. There's
> no situation in which a not-enforced constraint can be used for any
> query optimizations -- you cannot know if a constraint remains valid
> after it's been turned NOT ENFORCED, because anyone could insert data
> that violates it milliseconds after it stops being enforced. I think
> the expectation that the application is going to correctly enforce the
> constraint after it's told the database server not to enforce it, is
> going to be problematic. As I recall, we already do this in FDWs for
> instance and it's already a problem.

What's the use of NOT ENFORCED constraint then?

To me NOT ENFORCED looks similar to informational constraints of IBM
DB2 [1]. It seems that they have TRUSTED/NOT TRUSTED similar to
PostgreSQL's VALID/NOT VALID [2].

[1] https://www.ibm.com/docs/en/db2/11.1?topic=constraints-informational
[2] https://www.ibm.com/docs/en/db2/11.1?topic=statements-create-table

--
Best Wishes,
Ashutosh Bapat

On Mon, Feb 3, 2025 at 9:39 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Sat, Feb 1, 2025 at 8:31 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> >
> > [...]
> > So the code should only call AlterConstrTriggerDeferrability,
> > not call ATExecAlterConstrEnforceability?
>
> Right. Thank you for the report. We need to know whether the
> enforceability and/or deferability has actually been set or not before
> catalog update.
>

Fixed in the attached version. A new patch, 0001, introduces a new
struct, AlterConstraintStmt, which carries the necessary information
for enforceability and deferrability modifications of a constraint, as
implemented in patch 0006.

Regards,
Amul

On 03.02.25 06:19, Ashutosh Bapat wrote:
> Here's how I see the state conversions happening.
>
> NOT VALID, NOT ENFORCED changed to NOT_VALID, ENFORCED - no data
> validation required, constraint is enforced on the new tuples/changes
> NOT VALID, ENFORCED changed to NOT VALID, NOT ENFORCED - no data
> validation, constraint isn't enforced anymore
> VALID, NOT ENFORCED changed to VALID, ENFORCED - data validation
> required, constraint is enforced
> VALID, ENFORCED changed to VALID, NOT ENFORCED - no data validation
> required, constrain isn't enforced anymore, we rely on user to enforce
> the constraint on their side

This looks sensible to me.

On 03.02.25 08:50, Alvaro Herrera wrote:
> On 2025-Feb-03, Ashutosh Bapat wrote:
>
>> VALID, NOT ENFORCED changed to VALID, ENFORCED - data validation
>> required, constraint is enforced
> There's no such thing as a VALID NOT ENFORCED constraint. It just
> cannot exist.

The way I interpret this is that the VALID flag is just recording what
would happen if the constraint was enforced. So you you take a [NOT]
VALID ENFORCED constraint and switch it to NOT ENFORCED and back and you
get back to where you started.

On 03.02.25 13:19, Alvaro Herrera wrote:
> On 2025-Feb-03, Ashutosh Bapat wrote:
>
>> ```
>> If the
>> constraint is <literal>NOT ENFORCED</literal>, the database system will
>> not check the constraint. It is then up to the application code to
>> ensure that the constraints are satisfied. The database system might
>> still assume that the data actually satisfies the constraint for
>> optimization decisions where this does not affect the correctness of the
>> result.
>> ```
>
> IMO the third sentence should be removed because it is bogus. There's
> no situation in which a not-enforced constraint can be used for any
> query optimizations -- you cannot know if a constraint remains valid
> after it's been turned NOT ENFORCED, because anyone could insert data
> that violates it milliseconds after it stops being enforced. I think
> the expectation that the application is going to correctly enforce the
> constraint after it's told the database server not to enforce it, is
> going to be problematic. As I recall, we already do this in FDWs for
> instance and it's already a problem.

The database system could use the presence of a not enforced constraint
for selectivity estimation, for example.

On 2025-Feb-04, Peter Eisentraut wrote:

> On 03.02.25 08:50, Alvaro Herrera wrote:
> > On 2025-Feb-03, Ashutosh Bapat wrote:
> >
> > > VALID, NOT ENFORCED changed to VALID, ENFORCED - data validation
> > > required, constraint is enforced
> > There's no such thing as a VALID NOT ENFORCED constraint. It just
> > cannot exist.
>
> The way I interpret this is that the VALID flag is just recording what would
> happen if the constraint was enforced. So you you take a [NOT] VALID
> ENFORCED constraint and switch it to NOT ENFORCED and back and you get back
> to where you started.

I think it is dangerous. You can easily end up with undetected
violating rows in the table, and then you won't be able to dump/restore
it anymore.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php

On Tue, Feb 4, 2025 at 7:22 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 03.02.25 06:19, Ashutosh Bapat wrote:
> > Here's how I see the state conversions happening.
> >
> > NOT VALID, NOT ENFORCED changed to NOT_VALID, ENFORCED - no data
> > validation required, constraint is enforced on the new tuples/changes
> > NOT VALID, ENFORCED changed to NOT VALID, NOT ENFORCED - no data
> > validation, constraint isn't enforced anymore
> > VALID, NOT ENFORCED changed to VALID, ENFORCED - data validation
> > required, constraint is enforced
> > VALID, ENFORCED changed to VALID, NOT ENFORCED - no data validation
> > required, constrain isn't enforced anymore, we rely on user to enforce
> > the constraint on their side
>
> This looks sensible to me.

Attached patch implemented this behaviour. To achieve this, we have to
revert (see 0007) some committed code and relax the restriction that
the NOT ENFORCED constraint must also be NOT VALID. Now, NOT ENFORCED
and NOT VALID are independent statuses, and the psql-meta meta command
will display NOT VALID alongside the NOT ENFORCED constraint.
Previously, we hid NOT VALID for NOT ENFORCED constraints, assuming it
would be implied, but that is no longer the case.

Apart from this, I have added a few more tests in 0009. Additionally,
I made some minor code rearrangements in the
AttachPartitionForeignKey() function -- was introduced in the 0002
refactoring patch. I kept these rearrangement changes separate in 0003
to highlight them, allowing the reviewer to identify any potential
issues, though I don't believe there are any. Of course, I could be
wrong.

Also, I need more time for additional testing of the 0008 and 0009
patches, which I plan to complete by the end of this week. Thanks.

Regards,
Amul

On Mon, Feb 10, 2025, at 7:03 AM, Amul Sul wrote:
> Attached patch implemented this behaviour. To achieve this, we have to
> revert (see 0007) some committed code and relax the restriction that
> the NOT ENFORCED constraint must also be NOT VALID. Now, NOT ENFORCED
> and NOT VALID are independent statuses, and the psql-meta meta command
> will display NOT VALID alongside the NOT ENFORCED constraint.
> Previously, we hid NOT VALID for NOT ENFORCED constraints, assuming it
> would be implied, but that is no longer the case.

I think this proposed state of affairs is problematic. Current queries that assume that pg_constraint.convalidated means that a constraint is validated would be broken. My suggestion at this point is that instead of adding a separate boolean column to pg_constraint we should be replacing `bool convalidated` with `char convalidity`, with new defines for all the possible states we require: enforced-and-valid ("V"alid), enforced-not-validated ("i"nvalid), not-enforced-and-not-valid (terribly "I"nvalid or maybe "U"nenforced), not-enforced-but-was-valid-before-turning-unenforced ("u"nenforced). Breaking user queries would make all apps reassess what do they actually want to know about the constraint without assumptions of how enforcement worked in existing Postgres releases.

This shouldn't be a very large change from what you currently have, I think.

> • v13-0001-Add-AlterConstraintStmt-struct-for-ALTER-.-CONST.patch

I think the new node name is wrong, because it makes the code looks as if we support ALTER CONSTRAINT as a statement for this, which is false. (This is a repeat of ReplicaIdentityStmt, which I think is a mistake). I would suggest a name like ATAlterConstraint instead. Perhaps we can use that in the patch for ALTER CONSTRAINT ... SET [NO] INHERIT as well, instead of (as I suggested Suraj) creating a separate subcommand number.

On Mon, 10 Feb 2025 at 13:48, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

I think this proposed state of affairs is problematic. Current queries
> that assume that pg_constraint.convalidated means that a constraint is
> validated would be broken. My suggestion at this point is that instead of
> adding a separate boolean column to pg_constraint we should be replacing
> `bool convalidated` with `char convalidity`, with new defines for all the
> possible states we require: enforced-and-valid ("V"alid),
> enforced-not-validated ("i"nvalid), not-enforced-and-not-valid (terribly
> "I"nvalid or maybe "U"nenforced),
> not-enforced-but-was-valid-before-turning-unenforced ("u"nenforced).
> Breaking user queries would make all apps reassess what do they actually
> want to know about the constraint without assumptions of how enforcement
> worked in existing Postgres releases.
>

I'm having a lot of trouble understanding the operational distinction
between your 'u' and 'U'. If it's not enforced, it cannot be assumed to be
valid, regardless of whether it was valid in the past. I'm not sure what I
think of a single character vs. 2 booleans, but there are only 3 sensible
states either way: valid enforced, invalid enforced, and invalid unenforced.

Additionally, if there are officially 4 status possibilities then all code
that looks for unenforced constraints has to look for both valid and
invalid unenforced constraints if we use a char; it's not as bad with 2
booleans because one can just check the "enforced" boolean.

On 2025-Feb-10, Isaac Morland wrote:

> I'm having a lot of trouble understanding the operational distinction
> between your 'u' and 'U'. If it's not enforced, it cannot be assumed to be
> valid, regardless of whether it was valid in the past. I'm not sure what I
> think of a single character vs. 2 booleans, but there are only 3 sensible
> states either way: valid enforced, invalid enforced, and invalid unenforced.

I kinda agree with you and would prefer that things were that way as
well. But look at the discussion starting at
https://postgr.es/m/CAExHW5tV23Sw+Nznv0KpdNg_t7LrXY1WM9atiC=eKKSsKHSnuQ@mail.gmail.com
whereby it was apparently established that if you have a
NOT VALID NOT ENFORCED
constraint, and you make it enforced, then you should somehow end up
with a NOT VALID ENFORCED constraint, which says to me that we need to
store the fact that the constraint was NOT VALID to start with; and
correspondingly if it's VALID NOT ENFORCED and you enforce it, then it
ends up VALID ENFORCED. If we take this view of the world (with which,
I repeat, I disagree) then we must keep track of whether the constraint
was valid or not valid to start with. And this means that we need to
keep convalidated=true _regardless_ of whether conenforced is false.
So in this view of the world there aren't three states but four.

I would prefer there to be three states as well, but apparently I'm
outvoted on this.

> Additionally, if there are officially 4 status possibilities then all code
> that looks for unenforced constraints has to look for both valid and
> invalid unenforced constraints if we use a char; it's not as bad with 2
> booleans because one can just check the "enforced" boolean.

Well, yes. You have kinda the same issue with any other system catalog
column that's a 'char', I guess. Maybe this is more of a problem here
because it's more user-visible than most other catalogs, not sure.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On Tue, 11 Feb 2025 at 08:36, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

> On 2025-Feb-10, Isaac Morland wrote:
>
> > I'm having a lot of trouble understanding the operational distinction
> > between your 'u' and 'U'. If it's not enforced, it cannot be assumed to
> be
> > valid, regardless of whether it was valid in the past. I'm not sure what
> I
> > think of a single character vs. 2 booleans, but there are only 3 sensible
> > states either way: valid enforced, invalid enforced, and invalid
> unenforced.
>
> I kinda agree with you and would prefer that things were that way as
> well. But look at the discussion starting at
>
> https://postgr.es/m/CAExHW5tV23Sw+Nznv0KpdNg_t7LrXY1WM9atiC=eKKSsKHSnuQ@mail.gmail.com
> whereby it was apparently established that if you have a
> NOT VALID NOT ENFORCED
> constraint, and you make it enforced, then you should somehow end up
> with a NOT VALID ENFORCED constraint, which says to me that we need to
> store the fact that the constraint was NOT VALID to start with; and
> correspondingly if it's VALID NOT ENFORCED and you enforce it, then it
> ends up VALID ENFORCED. If we take this view of the world (with which,
> I repeat, I disagree) then we must keep track of whether the constraint
> was valid or not valid to start with. And this means that we need to
> keep convalidated=true _regardless_ of whether conenforced is false.
> So in this view of the world there aren't three states but four.
>
> I would prefer there to be three states as well, but apparently I'm
> outvoted on this.
>

Sounds like we agree. I think the problem is with the statement in the
linked discussion that “If the constraint is VALID and later marked as NOT
ENFORCED, changing it to ENFORCED should also keep it VALID.” This ignores
that if it is changed to NOT ENFORCED that should immediately change it to
NOT VALID if it is not already so.

Has anybody argued for how it makes any sense at all to have a constraint
that is VALID (and therefore will be assumed to be true by the planner),
yet NOT ENFORCED (and therefore may well not be true)? What next, a patch
to the planner so that it only treats as true constraints that are both
VALID and ENFORCED?

Re: the 3 or 4 values for the single character status, there is a similar
issue with relkind, where one can imagine writing "relkind IN ('r')" when
one meant "relkind IN ('r', 'v')" or something else; but on the other hand,
one can easily imagine actually wanting the first one of those. But here,
it's not at all clear to me when you would ever want to distinguish between
'u' and 'U', but it is clear to me that it would be natural to write "… =
'U'" when one actually needs to write "… IN ('u', 'U')", or perhaps "…
ILIKE 'u'" (not what I would want to see).

On Tue, Feb 11, 2025 at 9:09 PM Isaac Morland <isaac(dot)morland(at)gmail(dot)com> wrote:
>
> On Tue, 11 Feb 2025 at 08:36, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>>
>> On 2025-Feb-10, Isaac Morland wrote:
>>
>> > I'm having a lot of trouble understanding the operational distinction
>> > between your 'u' and 'U'. If it's not enforced, it cannot be assumed to be
>> > valid, regardless of whether it was valid in the past. I'm not sure what I
>> > think of a single character vs. 2 booleans, but there are only 3 sensible
>> > states either way: valid enforced, invalid enforced, and invalid unenforced.
>>
>> I kinda agree with you and would prefer that things were that way as
>> well. But look at the discussion starting at
>> https://postgr.es/m/CAExHW5tV23Sw+Nznv0KpdNg_t7LrXY1WM9atiC=eKKSsKHSnuQ@mail.gmail.com
>> whereby it was apparently established that if you have a
>> NOT VALID NOT ENFORCED
>> constraint, and you make it enforced, then you should somehow end up
>> with a NOT VALID ENFORCED constraint, which says to me that we need to
>> store the fact that the constraint was NOT VALID to start with; and
>> correspondingly if it's VALID NOT ENFORCED and you enforce it, then it
>> ends up VALID ENFORCED. If we take this view of the world (with which,
>> I repeat, I disagree) then we must keep track of whether the constraint
>> was valid or not valid to start with. And this means that we need to
>> keep convalidated=true _regardless_ of whether conenforced is false.
>> So in this view of the world there aren't three states but four.
>>
>> I would prefer there to be three states as well, but apparently I'm
>> outvoted on this.
>
>
> Sounds like we agree. I think the problem is with the statement in the linked discussion that “If the constraint is VALID and later marked as NOT ENFORCED, changing it to ENFORCED should also keep it VALID.” This ignores that if it is changed to NOT ENFORCED that should immediately change it to NOT VALID if it is not already so.
>
> Has anybody argued for how it makes any sense at all to have a constraint that is VALID (and therefore will be assumed to be true by the planner), yet NOT ENFORCED (and therefore may well not be true)? What next, a patch to the planner so that it only treats as true constraints that are both VALID and ENFORCED?

I have been asking a different question: What's the use of
not-enforced constraints if we don't allow VALID, NOT ENFORCED state
for them? OTOH, consider an application which "knows" that the
constraint is valid for the data (either because of checks at
application level, or because the data was replicated from some other
system where the cosntraints were applied). It's a natural ask to use
the constraints for, say optimization, but don't take unnecessary
overhead of validating them. VALID, NOT ENFORCED state helps in such a
scenario. Of course an application can misuse it (just like stable
marking on a function), but well ... they will be penalised for their
misuse.

--
Best Wishes,
Ashutosh Bapat

On 2025-Feb-12, Ashutosh Bapat wrote:

> I have been asking a different question: What's the use of
> not-enforced constraints if we don't allow VALID, NOT ENFORCED state
> for them?

That's a question for the SQL standards committee. They may serve
schema documentation purposes, for example.
https://www.postgresql.eu/events/pgconfeu2024/schedule/session/5677-exploring-postgres-databases-with-graphs/

> OTOH, consider an application which "knows" that the constraint is
> valid for the data (either because of checks at application level, or
> because the data was replicated from some other system where the
> cosntraints were applied). It's a natural ask to use the constraints
> for, say optimization, but don't take unnecessary overhead of
> validating them. VALID, NOT ENFORCED state helps in such a scenario.
> Of course an application can misuse it (just like stable marking on a
> function), but well ... they will be penalised for their misuse.

I disagree that we should see a VALID NOT ENFORCED constraint as one
that can be used for query optimization purposes. This is only going to
bring users pain, because it's far too easy to misuse and they will get
wrong query results, possibly without knowing for who knows how long.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"El número de instalaciones de UNIX se ha elevado a 10,
y se espera que este número aumente" (UPM, 1972)

On 11.02.25 14:36, Álvaro Herrera wrote:
> On 2025-Feb-10, Isaac Morland wrote:
>
>> I'm having a lot of trouble understanding the operational distinction
>> between your 'u' and 'U'. If it's not enforced, it cannot be assumed to be
>> valid, regardless of whether it was valid in the past. I'm not sure what I
>> think of a single character vs. 2 booleans, but there are only 3 sensible
>> states either way: valid enforced, invalid enforced, and invalid unenforced.
>
> I kinda agree with you and would prefer that things were that way as
> well. But look at the discussion starting at
> https://postgr.es/m/CAExHW5tV23Sw+Nznv0KpdNg_t7LrXY1WM9atiC=eKKSsKHSnuQ@mail.gmail.com
> whereby it was apparently established that if you have a
> NOT VALID NOT ENFORCED
> constraint, and you make it enforced, then you should somehow end up
> with a NOT VALID ENFORCED constraint, which says to me that we need to
> store the fact that the constraint was NOT VALID to start with; and
> correspondingly if it's VALID NOT ENFORCED and you enforce it, then it
> ends up VALID ENFORCED. If we take this view of the world (with which,
> I repeat, I disagree) then we must keep track of whether the constraint
> was valid or not valid to start with. And this means that we need to
> keep convalidated=true _regardless_ of whether conenforced is false.
> So in this view of the world there aren't three states but four.
>
> I would prefer there to be three states as well, but apparently I'm
> outvoted on this.

Just to make this a bit more confusing, here is another interpretation
of the state NOT ENFORCED VALID (they call it DISABLE VALIDATE):

https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/constraint.html#GUID-1055EA97-BA6F-4764-A15F-1024FD5B6DFE__I1002349

"""
DISABLE VALIDATE disables the constraint and drops the index on the
constraint, but keeps the constraint valid. This feature is most useful
in data warehousing situations, because it lets you load large amounts
of data while also saving space by not having an index. This setting
lets you load data from a nonpartitioned table into a partitioned table
using the exchange_partition_subpart clause of the ALTER TABLE statement
or using SQL*Loader. All other modifications to the table (inserts,
updates, and deletes) by other SQL statements are disallowed.
"""

On 12.02.25 12:13, Álvaro Herrera wrote:
> On 2025-Feb-12, Ashutosh Bapat wrote:
>
>> I have been asking a different question: What's the use of
>> not-enforced constraints if we don't allow VALID, NOT ENFORCED state
>> for them?
>
> That's a question for the SQL standards committee. They may serve
> schema documentation purposes, for example.
> https://www.postgresql.eu/events/pgconfeu2024/schedule/session/5677-exploring-postgres-databases-with-graphs/
>
>> OTOH, consider an application which "knows" that the constraint is
>> valid for the data (either because of checks at application level, or
>> because the data was replicated from some other system where the
>> cosntraints were applied). It's a natural ask to use the constraints
>> for, say optimization, but don't take unnecessary overhead of
>> validating them. VALID, NOT ENFORCED state helps in such a scenario.
>> Of course an application can misuse it (just like stable marking on a
>> function), but well ... they will be penalised for their misuse.
>
> I disagree that we should see a VALID NOT ENFORCED constraint as one
> that can be used for query optimization purposes. This is only going to
> bring users pain, because it's far too easy to misuse and they will get
> wrong query results, possibly without knowing for who knows how long.

I've been digging into the ISO archives for some more background on the
intended meaning of this feature.

Result: "NOT ENFORCED" just means "off" or "disabled", "could contain
anything". You can use this to do data loads, or schema surgery, or
things like that. Or just if you want it for documentation.

This idea that a not-enforced constraint should contain valid data
anyway is not supported by anything I could find written down. I've
heard that in discussions, but those could have been speculations.

(I still think that could be a feature, but it's clearly not this one,
at least not in its default state.)

So considering that, I think a three-state system makes more sense.
Something like:

1) NOT ENFORCED -- no data is checked
2) NOT VALID -- existing data is unchecked, new data is checked
3) ENFORCED -- all data is checked

Transitions:

(1) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> (2)
(1) - [ ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED ] -> (3)
(2) - [ ALTER TABLE ... VALIDATE CONSTRAINT ... ] -> (3)
(2|3) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT ENFORCED ] -> (1)
(3) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> (2)

On Wed, Feb 12, 2025 at 8:15 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 12.02.25 12:13, Álvaro Herrera wrote:
> > On 2025-Feb-12, Ashutosh Bapat wrote:
> >
> >> I have been asking a different question: What's the use of
> >> not-enforced constraints if we don't allow VALID, NOT ENFORCED state
> >> for them?
> >
> > That's a question for the SQL standards committee. They may serve
> > schema documentation purposes, for example.
> > https://www.postgresql.eu/events/pgconfeu2024/schedule/session/5677-exploring-postgres-databases-with-graphs/
> >
> >> OTOH, consider an application which "knows" that the constraint is
> >> valid for the data (either because of checks at application level, or
> >> because the data was replicated from some other system where the
> >> cosntraints were applied). It's a natural ask to use the constraints
> >> for, say optimization, but don't take unnecessary overhead of
> >> validating them. VALID, NOT ENFORCED state helps in such a scenario.
> >> Of course an application can misuse it (just like stable marking on a
> >> function), but well ... they will be penalised for their misuse.
> >
> > I disagree that we should see a VALID NOT ENFORCED constraint as one
> > that can be used for query optimization purposes. This is only going to
> > bring users pain, because it's far too easy to misuse and they will get
> > wrong query results, possibly without knowing for who knows how long.
>
> I've been digging into the ISO archives for some more background on the
> intended meaning of this feature.
>
> Result: "NOT ENFORCED" just means "off" or "disabled", "could contain
> anything". You can use this to do data loads, or schema surgery, or
> things like that. Or just if you want it for documentation.

Hmm, so one can convert an enforced constraint to a not-enforced
constraint, load the data or make changes and then enforce it again.
Makes sense.

>
> This idea that a not-enforced constraint should contain valid data
> anyway is not supported by anything I could find written down. I've
> heard that in discussions, but those could have been speculations.
>
> (I still think that could be a feature, but it's clearly not this one,
> at least not in its default state.)

Thanks for the background.

>
> So considering that, I think a three-state system makes more sense.
> Something like:
>
> 1) NOT ENFORCED -- no data is checked
> 2) NOT VALID -- existing data is unchecked, new data is checked
> 3) ENFORCED -- all data is checked
>
> Transitions:
>
> (1) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> (2)

Per your notation, this means the the constraint is not enforced but
new data is checked - that seems a contradiction, how would we check
the data when the constraint is not being enforced. Or do you suggest
that we convert a NOT ENFORCED constraint to ENFORCED as a result of
converting it to NOT VALID?

> (1) - [ ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED ] -> (3)

Seems ok.

> (2) - [ ALTER TABLE ... VALIDATE CONSTRAINT ... ] -> (3)

As a result of this a not enforced constraint would turn into an
enforced constraint. The user might have intended to just validate the
data but not enforce it to avoid paying price for the checks on new
data.

> (2|3) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT ENFORCED ] -> (1)

Looks fine.

> (3) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> (2)
>

This too seems ok assuming the constraint would remain enforced.

I think, what you intend to say is clearer with 4 state system {NE, E}
* {NV, V} = {(NE, NV), (NE, V), (E, NV), (E, V)} where (NE, V) is
unreachable. Let's name them S1, S2, S3, S4 respectively.

S1 -> [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> S1 - noop
S3 -> [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> S3 - noop
S4 -> [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> S3

S1->[ ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED ]->S4
S3->[ ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED ]->S3 - noop
S4->[ ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED ]->S4 - noop

S1->[ ALTER TABLE ... VALIDATE CONSTRAINT ... ]->S1 - but this is not
noop - the existing data gets validated but no change happens to the
state of the constraint - it is not enforced on the future data and
it's not considered valid. This gives opportunity to the user to just
validate the existing data but not enforce the constraint on new data
thus avoiding some computation on the new data. Of course we will have
to update the documentation to clearly specify the result. I think
VALIDATE CONSTRAINT is postgresql extension so we are free to
interpret it in the context of ENFORCED feature.
S3->[ ALTER TABLE ... VALIDATE CONSTRAINT ... ]->S4
S4->[ ALTER TABLE ... VALIDATE CONSTRAINT ... ]->S4 - noop

S1-[ ALTER TABLE ... ALTER CONSTRAINT ... NOT ENFORCED ]->S1 - noop
S3-[ ALTER TABLE ... ALTER CONSTRAINT ... NOT ENFORCED ]->S1
S4-[[ ALTER TABLE ... ALTER CONSTRAINT ... NOT ENFORCED ]->S1

Notice that there are no edges to and from S2.

--
Best Wishes,
Ashutosh Bapat

On 2025-Feb-13, Ashutosh Bapat wrote:

> > So considering that, I think a three-state system makes more sense.
> > Something like:
> >
> > 1) NOT ENFORCED -- no data is checked
> > 2) NOT VALID -- existing data is unchecked, new data is checked
> > 3) ENFORCED -- all data is checked
> >
> > Transitions:
> >
> > (1) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> (2)
>
> Per your notation, this means the the constraint is not enforced but
> new data is checked - that seems a contradiction, how would we check
> the data when the constraint is not being enforced. Or do you suggest
> that we convert a NOT ENFORCED constraint to ENFORCED as a result of
> converting it to NOT VALID?

I agree this one is a little weird. For this I would have the command
be
ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED NOT VALID
this way it's explicit that what we want is flip the ENFORCED bit while
leaving NOT VALID as-is.

> > (2) - [ ALTER TABLE ... VALIDATE CONSTRAINT ... ] -> (3)
>
> As a result of this a not enforced constraint would turn into an
> enforced constraint. The user might have intended to just validate the
> data but not enforce it to avoid paying price for the checks on new
> data.

I'm not sure there's a use case for validating existing data without
starting to enforce the constraint. The data can become invalid
immediately after you've run the command, so why bother?

> I think, what you intend to say is clearer with 4 state system {NE, E}
> * {NV, V} = {(NE, NV), (NE, V), (E, NV), (E, V)} where (NE, V) is
> unreachable. Let's name them S1, S2, S3, S4 respectively.
[...]
> Notice that there are no edges to and from S2.

So why list it as a possible state?

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Las mujeres son como hondas: mientras más resistencia tienen,
más lejos puedes llegar con ellas" (Jonas Nightingale, Leap of Faith)

On Thu, Feb 13, 2025 at 5:27 PM Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Feb-13, Ashutosh Bapat wrote:
>
> > > So considering that, I think a three-state system makes more sense.
> > > Something like:
> > >
> > > 1) NOT ENFORCED -- no data is checked
> > > 2) NOT VALID -- existing data is unchecked, new data is checked
> > > 3) ENFORCED -- all data is checked
> > >
> > > Transitions:
> > >
> > > (1) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> (2)
> >
> > Per your notation, this means the the constraint is not enforced but
> > new data is checked - that seems a contradiction, how would we check
> > the data when the constraint is not being enforced. Or do you suggest
> > that we convert a NOT ENFORCED constraint to ENFORCED as a result of
> > converting it to NOT VALID?
>
> I agree this one is a little weird. For this I would have the command
> be
> ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED NOT VALID
> this way it's explicit that what we want is flip the ENFORCED bit while
> leaving NOT VALID as-is.
>
> > > (2) - [ ALTER TABLE ... VALIDATE CONSTRAINT ... ] -> (3)
> >
> > As a result of this a not enforced constraint would turn into an
> > enforced constraint. The user might have intended to just validate the
> > data but not enforce it to avoid paying price for the checks on new
> > data.
>
> I'm not sure there's a use case for validating existing data without
> starting to enforce the constraint. The data can become invalid
> immediately after you've run the command, so why bother?

Validating whole table at a time is cheaper than doing it for every
row as it appears. So the ability to validate data in batches at
regular intervals instead of validating every row has some
attractiveness, esp in huge data/analytics cases. And we could
implement it without much cost. But I don't have a concrete usecase.

>
> > I think, what you intend to say is clearer with 4 state system {NE, E}
> > * {NV, V} = {(NE, NV), (NE, V), (E, NV), (E, V)} where (NE, V) is
> > unreachable. Let's name them S1, S2, S3, S4 respectively.
> [...]
> > Notice that there are no edges to and from S2.
>
> So why list it as a possible state?

For the sake of combinatorics. :)

--
Best Wishes,
Ashutosh Bapat

On Fri, 14 Feb 2025 at 10:11, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
wrote:

> > > I think, what you intend to say is clearer with 4 state system {NE, E}
> > > * {NV, V} = {(NE, NV), (NE, V), (E, NV), (E, V)} where (NE, V) is
> > > unreachable. Let's name them S1, S2, S3, S4 respectively.
> > [...]
> > > Notice that there are no edges to and from S2.
> >
> > So why list it as a possible state?
>
> For the sake of combinatorics. :)
>

Just because there are 2^n combinations of n boolean values does not mean
there are 2^n actual meaningful states. That's why we have CHECK
constraints.

On Fri, Feb 14, 2025 at 8:41 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Thu, Feb 13, 2025 at 5:27 PM Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > On 2025-Feb-13, Ashutosh Bapat wrote:
> >
> > > > So considering that, I think a three-state system makes more sense.
> > > > Something like:
> > > >
> > > > 1) NOT ENFORCED -- no data is checked
> > > > 2) NOT VALID -- existing data is unchecked, new data is checked
> > > > 3) ENFORCED -- all data is checked
> > > >
> > > > Transitions:
> > > >
> > > > (1) - [ ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID ] -> (2)
> > >
> > > Per your notation, this means the the constraint is not enforced but
> > > new data is checked - that seems a contradiction, how would we check
> > > the data when the constraint is not being enforced. Or do you suggest
> > > that we convert a NOT ENFORCED constraint to ENFORCED as a result of
> > > converting it to NOT VALID?
> >
> > I agree this one is a little weird. For this I would have the command
> > be
> > ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED NOT VALID
> > this way it's explicit that what we want is flip the ENFORCED bit while
> > leaving NOT VALID as-is.
> >
> > > > (2) - [ ALTER TABLE ... VALIDATE CONSTRAINT ... ] -> (3)
> > >
> > > As a result of this a not enforced constraint would turn into an
> > > enforced constraint. The user might have intended to just validate the
> > > data but not enforce it to avoid paying price for the checks on new
> > > data.
> >
> > I'm not sure there's a use case for validating existing data without
> > starting to enforce the constraint. The data can become invalid
> > immediately after you've run the command, so why bother?
>
> Validating whole table at a time is cheaper than doing it for every
> row as it appears. So the ability to validate data in batches at
> regular intervals instead of validating every row has some
> attractiveness, esp in huge data/analytics cases. And we could
> implement it without much cost. But I don't have a concrete usecase.
>

Well, I’m not sure if it’s worth validating data in batches when we
don’t maintain their state, as this would lead to revalidating the
same data in the next validation along with newly inserted records.

Also, based on the current implementation, we can perform CHECK
constraint validation, but not FOREIGN KEY constraint validation,
since the necessary triggers for referential integrity checks haven’t
been created for NOT ENFORCED. While we can create those triggers,
it’s unclear whether we want to keep them around if they aren’t being
used for NOT ENFORCED constraints.

Regards,
Amul

On Tue, Feb 11, 2025 at 12:18 AM Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
[...]
>
> > • v13-0001-Add-AlterConstraintStmt-struct-for-ALTER-.-CONST.patch
>
> I think the new node name is wrong, because it makes the code looks as if we support ALTER CONSTRAINT as a statement for this, which is false. (This is a repeat of ReplicaIdentityStmt, which I think is a mistake). I would suggest a name like ATAlterConstraint instead. Perhaps we can use that in the patch for ALTER CONSTRAINT ... SET [NO] INHERIT as well, instead of (as I suggested Suraj) creating a separate subcommand number.

I have renamed AlterConstraintStmt to ATAlterConstraint as per your
suggestion in the attached version. Apart from this, there are no
other changes, as the final behavior is still unclear based on the
discussions so far.

If we don’t want to keep a constraint marked as valid when it is not
enforced, we should revert to the v12 version behavior, where the
constraint is marked invalid when changed to NOT ENFORCED and remains
so even after being changed to ENFORCED, until explicitly validated
using the existing ALTER ... VALIDATE CONSTRAINT command.

However, if we want to provide an option to validate the constraint
immediately when changing it to ENFORCED, we could introduce support
for a syntax like:

ALTER TABLE ... ALTER CONSTRAINT ... [ NOT ] ENFORCED [ (WITH |
WITHOUT) VALIDATION ]

Regards,
Amul

Hello,

On 2025-Feb-17, Amul Sul wrote:

> I have renamed AlterConstraintStmt to ATAlterConstraint as per your
> suggestion in the attached version. Apart from this, there are no
> other changes, as the final behavior is still unclear based on the
> discussions so far.

Okay, thanks. I've taken your alterDeferrability from your later patch
(renamed to just "deferrability"). Also, IMO the routine structure
needs a bit of a revamp. What do you think of the attached, which is
mostly your 0001? (Actually, now that I look, it seems I made more or
less the same changes you'd be doing in 0008, so this should be okay.)

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Tue, Feb 18, 2025 at 12:47 AM Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Hello,
>
> On 2025-Feb-17, Amul Sul wrote:
>
> > I have renamed AlterConstraintStmt to ATAlterConstraint as per your
> > suggestion in the attached version. Apart from this, there are no
> > other changes, as the final behavior is still unclear based on the
> > discussions so far.
>
> Okay, thanks. I've taken your alterDeferrability from your later patch
> (renamed to just "deferrability"). Also, IMO the routine structure
> needs a bit of a revamp. What do you think of the attached, which is
> mostly your 0001? (Actually, now that I look, it seems I made more or
> less the same changes you'd be doing in 0008, so this should be okay.)
>

The patch looks quite reasonable, but I’m concerned that renaming
ATExecAlterConstrRecurse() and ATExecAlterChildConstr() exclusively
for deferrability might require the enforceability patch to duplicate
these functions, even though some operations (e.g., pg_constraint
updates and recursion on child constraints) could have been reused.

Regards,
Amul

On 2025-Feb-18, Amul Sul wrote:

> The patch looks quite reasonable, but I’m concerned that renaming
> ATExecAlterConstrRecurse() and ATExecAlterChildConstr() exclusively
> for deferrability might require the enforceability patch to duplicate
> these functions, even though some operations (e.g., pg_constraint
> updates and recursion on child constraints) could have been reused.

True. I'll give another look to your 0008 and Suraj's patch for
inheritability change, to avoid repetitive boilerplate as much as
possible.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Tue, Feb 18, 2025 at 2:13 PM Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Feb-18, Amul Sul wrote:
>
> > The patch looks quite reasonable, but I’m concerned that renaming
> > ATExecAlterConstrRecurse() and ATExecAlterChildConstr() exclusively
> > for deferrability might require the enforceability patch to duplicate
> > these functions, even though some operations (e.g., pg_constraint
> > updates and recursion on child constraints) could have been reused.
>
> True. I'll give another look to your 0008 and Suraj's patch for
> inheritability change, to avoid repetitive boilerplate as much as
> possible.
>

Thanks, Álvaro, for committing the 0001 patch -- it really helps.

Attached is the rebased patch set against the latest master head,
which also includes a *new* refactoring patch (0001). In this patch,
I’ve re-added ATExecAlterChildConstr(), which is required for the main
feature patch (0008) to handle recursion from different places while
altering enforceability.

Regards,
Amul

On 2025-Feb-27, Amul Sul wrote:

> Attached is the rebased patch set against the latest master head,
> which also includes a *new* refactoring patch (0001). In this patch,
> I’ve re-added ATExecAlterChildConstr(), which is required for the main
> feature patch (0008) to handle recursion from different places while
> altering enforceability.

I think you refer to ATExecAlterConstrEnforceability, which claims
(falsely) that it is a subroutine to ATExecAlterConstrRecurse; in
reality it is called from ATExecAlterConstraintInternal or at least
that's what I see in your 0008. So I wonder if you haven't confused
yourself here. If nothing else, that comments needs fixed. I didn't
review these patches.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?" (Mafalda)

On Thu, Feb 27, 2025 at 4:48 PM Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Feb-27, Amul Sul wrote:
>
> > Attached is the rebased patch set against the latest master head,
> > which also includes a *new* refactoring patch (0001). In this patch,
> > I’ve re-added ATExecAlterChildConstr(), which is required for the main
> > feature patch (0008) to handle recursion from different places while
> > altering enforceability.
>
> I think you refer to ATExecAlterConstrEnforceability, which claims
> (falsely) that it is a subroutine to ATExecAlterConstrRecurse; in
> reality it is called from ATExecAlterConstraintInternal or at least
> that's what I see in your 0008. So I wonder if you haven't confused
> yourself here. If nothing else, that comments needs fixed. I didn't
> review these patches.
>

Yeah, that was intentional. I wanted to avoid recursion again by
hitting ATExecAlterChildConstr() at the end of
ATExecAlterConstraintInternal(). Also, I realized the value doesn’t
matter since recurse = false is explicitly set inside the
cmdcon->alterEnforceability condition. I wasn’t fully satisfied with
how we handled the recursion decision (code design), so I’ll give it
more thought. If I don’t find a better approach, I’ll add clearer
comments to explain the reasoning.

Regards,
Amul

Hi Amul,

On Thu, Feb 27, 2025 at 12:57 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:

> Attached is the rebased patch set against the latest master head,
> which also includes a *new* refactoring patch (0001). In this patch,
> I’ve re-added ATExecAlterChildConstr(), which is required for the main
> feature patch (0008) to handle recursion from different places while
> altering enforceability.

Thanks for the patches!

I reviewed and ran “make check” on each patch. I appreciate how the
patches are organized; separating the refactors from the
implementations made the review process very straightforward.
Overall, LGTM, and I have minor comments below:

0008
Since we are added "convalidated" in some of the constraints tests,
should we also add a "convalidated" field in the "table_constraints"
system view defined in src/backend/catalog/information_schema.sql? If
we do that, we'd also need to update the documentation for this view.

0009
Comment on top of the function ATExecAlterConstrEnforceability():
s/ATExecAlterConstrRecurse/ATExecAlterConstraintInternal/g

Typo in tablecmds.c: s/droping/dropping, s/ke/key
/* We should be droping trigger related to foreign ke constraint */

Thanks,
Alex

On Thu, Mar 6, 2025 at 9:37 PM Alexandra Wang
<alexandra(dot)wang(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Amul,
>
> On Thu, Feb 27, 2025 at 12:57 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>>
>> Attached is the rebased patch set against the latest master head,
>> which also includes a *new* refactoring patch (0001). In this patch,
>> I’ve re-added ATExecAlterChildConstr(), which is required for the main
>> feature patch (0008) to handle recursion from different places while
>> altering enforceability.
>
>
> Thanks for the patches!
>
> I reviewed and ran “make check” on each patch. I appreciate how the
> patches are organized; separating the refactors from the
> implementations made the review process very straightforward.

Thank you for the feedback and the review !

> Overall, LGTM, and I have minor comments below:
>
> 0008
> Since we are added "convalidated" in some of the constraints tests,
> should we also add a "convalidated" field in the "table_constraints"
> system view defined in src/backend/catalog/information_schema.sql? If
> we do that, we'd also need to update the documentation for this view.
>

I am not sure why we don't already have "convalidated" in the
table_constraints, but if we need it, we can add it separately.

> 0009
> Comment on top of the function ATExecAlterConstrEnforceability():
> s/ATExecAlterConstrRecurse/ATExecAlterConstraintInternal/g
>
> Typo in tablecmds.c: s/droping/dropping, s/ke/key
> /* We should be droping trigger related to foreign ke constraint */
>

Thanks, fixed in the attached version.

Regards,
Amul

On 2025-Feb-28, Amul Sul wrote:

> Yeah, that was intentional. I wanted to avoid recursion again by
> hitting ATExecAlterChildConstr() at the end of
> ATExecAlterConstraintInternal(). Also, I realized the value doesn’t
> matter since recurse = false is explicitly set inside the
> cmdcon->alterEnforceability condition. I wasn’t fully satisfied with
> how we handled the recursion decision (code design), so I’ll give it
> more thought. If I don’t find a better approach, I’ll add clearer
> comments to explain the reasoning.

So, did you have a chance to rethink the recursion model here? TBH I do
not like what you have one bit.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Para tener más hay que desear menos"

I have committed the first three refactoring patches (v16-0001,
v16-0002, v16-0003). (I guess Álvaro didn't like the first one, so I
suppose I'll revert that one, but it's a simple one, so you can proceed
either way.)

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

I have a few other review comments here in the meantime:

* patch v16-0007 "Ease the restriction that a NOT ENFORCED constraint
must be INVALID."

I don't understand the purpose of this one. And the commit message also
doesn't explain the reason, only what it does. I think we have settled
on three states (not enforced and not valid; enforced but not yet valid;
enforced and valid), so it seems sensible to keep valid as false if
enforced is also false. Did I miss something?

Specifically, this test case change

-ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
(b > 10) NOT ENFORCED; -- succeeds
+ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
(b > 10) NOT ENFORCED; -- fail
+ERROR: check constraint "b_greater_than_ten_not_enforced" of relation
"attmp3" is violated by some row
+ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
(b > 10) NOT VALID NOT ENFORCED; -- succeeds

seems very wrong to me.

* doc/src/sgml/advanced.sgml

Let's skip that. This material is too advanced for a tutorial.

* doc/src/sgml/ddl.sgml

Let's move the material about NOT ENFORCED into a separate section or
paragraph, not in the first paragraph that introduces foreign keys. I
suggest a separate sect2-level section at the end of the "Constraints"
section.

* src/backend/catalog/sql_features.txt

The SQL standard has NOT ENFORCED only for check and foreign-key
constraints, so you could flip this to "YES" here. (Hmm, do we need
to support not-null constraints, though (which are grouped under check
constraints in the standard)? Maybe turn the comment around and say
"except not-null constraints" or something like that.)

* src/backend/commands/tablecmds.c

I would omit this detail message:

errdetail("Enforceability can only be altered for foreign key constraints.")

We have generally tried to get rid of detail messages that say "cannot
do this on this object type, but you could do it on a different object
type", since that is not actually useful.

* src/test/regress/expected/foreign_key.out

This error message is confusing, since no insert or update is happening:

+ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
+ERROR: insert or update on table "fktable" violates foreign key
constraint "fktable_ftest1_fkey"

Could we give a differently worded error message in this case?

Here, you are relying on the automatic constraint naming, which seems
fragile and confusing:

+ALTER TABLE FKTABLE ADD FOREIGN KEY(ftest1, ftest2) REFERENCES PKTABLE
NOT VALID NOT ENFORCED;
+ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_ftest2_fkey
DEFERRABLE INITIALLY DEFERRED;

Better name the constraint explicitly in the first command.

On Tue, Mar 11, 2025 at 11:13 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> I have committed the first three refactoring patches (v16-0001,
> v16-0002, v16-0003). (I guess Álvaro didn't like the first one, so I
> suppose I'll revert that one, but it's a simple one, so you can proceed
> either way.)
>

Sure, thank you !

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

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

> I have a few other review comments here in the meantime:
>
> * patch v16-0007 "Ease the restriction that a NOT ENFORCED constraint
> must be INVALID."
>
> I don't understand the purpose of this one. And the commit message also
> doesn't explain the reason, only what it does. I think we have settled
> on three states (not enforced and not valid; enforced but not yet valid;
> enforced and valid), so it seems sensible to keep valid as false if
> enforced is also false. Did I miss something?
>

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

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

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

> Specifically, this test case change
>
> -ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
> (b > 10) NOT ENFORCED; -- succeeds
> +ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
> (b > 10) NOT ENFORCED; -- fail
> +ERROR: check constraint "b_greater_than_ten_not_enforced" of relation
> "attmp3" is violated by some row
> +ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK
> (b > 10) NOT VALID NOT ENFORCED; -- succeeds
>
> seems very wrong to me.
>

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

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

Done.

> * doc/src/sgml/ddl.sgml
>
> Let's move the material about NOT ENFORCED into a separate section or
> paragraph, not in the first paragraph that introduces foreign keys. I
> suggest a separate sect2-level section at the end of the "Constraints"
> section.
>

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

> * src/backend/catalog/sql_features.txt
>
> The SQL standard has NOT ENFORCED only for check and foreign-key
> constraints, so you could flip this to "YES" here. (Hmm, do we need
> to support not-null constraints, though (which are grouped under check
> constraints in the standard)? Maybe turn the comment around and say
> "except not-null constraints" or something like that.)
>

Done.

> * src/backend/commands/tablecmds.c
>
> I would omit this detail message:
>
> errdetail("Enforceability can only be altered for foreign key constraints.")
>
> We have generally tried to get rid of detail messages that say "cannot
> do this on this object type, but you could do it on a different object
> type", since that is not actually useful.
>
> * src/test/regress/expected/foreign_key.out
>

Done.

> This error message is confusing, since no insert or update is happening:
>
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
> +ERROR: insert or update on table "fktable" violates foreign key
> constraint "fktable_ftest1_fkey"
>
> Could we give a differently worded error message in this case?
>

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

> Here, you are relying on the automatic constraint naming, which seems
> fragile and confusing:
>
> +ALTER TABLE FKTABLE ADD FOREIGN KEY(ftest1, ftest2) REFERENCES PKTABLE
> NOT VALID NOT ENFORCED;
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_ftest2_fkey
> DEFERRABLE INITIALLY DEFERRED;
>
> Better name the constraint explicitly in the first command.
>

Fixed in the attached version.

Regards,
Amul.

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

On 2025-Mar-12, Amul Sul wrote:

> On Tue, Mar 11, 2025 at 11:13 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

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

Yeah, I gave this a look and I think this code layout is good. There
are more functions now, but the code flow is simpler.

Thanks!

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Wed, Mar 19, 2025 at 12:33 AM Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Mar-12, Amul Sul wrote:
>
> > On Tue, Mar 11, 2025 at 11:13 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> > > I think the next step here is that you work to fix Álvaro's concerns
> > > about the recursion structure.
> >
> > Yes, I worked on that in the attached version. I refactored
> > ATExecAlterConstraintInternal() and moved the code that updates the
> > pg_constraint entry into a separate function (see 0001), so it can be
> > called from the places where the entry needs to be updated, rather
> > than revisiting ATExecAlterConstraintInternal(). In 0002,
> > ATExecAlterConstraintInternal() is split into two functions:
> > ATExecAlterConstrDeferrability() and
> > ATExecAlterConstrInheritability(), which handle altering deferrability
> > and inheritability, respectively. These functions are expected to
> > recurse on themselves, rather than revisiting
> > ATExecAlterConstraintInternal() as before. This approach simplifies
> > things. Similarly can add ATExecAlterConstrEnforceability() which
> > recurses itself.
>
> Yeah, I gave this a look and I think this code layout is good. There
> are more functions now, but the code flow is simpler.
>

Thank you !

Attached is the updated version, where the commit messages for patch
0005 and 0006 have been slightly corrected. Additionally, a few code
comments have been updated to consistently use the ENFORCED/NOT
ENFORCED keywords. The rest of the patches and all the code are
unchanged.

Regards,
Amul

On 21.03.25 06:58, Amul Sul wrote:
>>>> I think the next step here is that you work to fix Álvaro's concerns
>>>> about the recursion structure.
>>>
>>> Yes, I worked on that in the attached version. I refactored
>>> ATExecAlterConstraintInternal() and moved the code that updates the
>>> pg_constraint entry into a separate function (see 0001), so it can be
>>> called from the places where the entry needs to be updated, rather
>>> than revisiting ATExecAlterConstraintInternal(). In 0002,
>>> ATExecAlterConstraintInternal() is split into two functions:
>>> ATExecAlterConstrDeferrability() and
>>> ATExecAlterConstrInheritability(), which handle altering deferrability
>>> and inheritability, respectively. These functions are expected to
>>> recurse on themselves, rather than revisiting
>>> ATExecAlterConstraintInternal() as before. This approach simplifies
>>> things. Similarly can add ATExecAlterConstrEnforceability() which
>>> recurses itself.
>>
>> Yeah, I gave this a look and I think this code layout is good. There
>> are more functions now, but the code flow is simpler.
>>
>
> Thank you !
>
> Attached is the updated version, where the commit messages for patch
> 0005 and 0006 have been slightly corrected. Additionally, a few code
> comments have been updated to consistently use the ENFORCED/NOT
> ENFORCED keywords. The rest of the patches and all the code are
> unchanged.

I have committed patches 0001 through 0003. I made some small changes:

In 0001, I renamed the function UpdateConstraintEntry() to
AlterConstrUpdateConstraintEntry() so the context is clearer.

In 0002, you had this change:

@@ -12113,75 +12154,89 @@ ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
* If the table at either end of the constraint is partitioned, we need to
* handle every constraint that is a child of this one.
*/
- if (recurse && changed &&
+ if (recurse &&
(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
- (OidIsValid(refrelid) &&
- get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)))
- ATExecAlterChildConstr(wqueue, cmdcon, conrel, tgrel, rel, contuple,
- recurse, otherrelids, lockmode);
+ get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE))
+ AlterConstrDeferrabilityRecurse(wqueue, cmdcon, conrel, tgrel, rel,
+ contuple, recurse, otherrelids,
+ lockmode);

AFAICT, dropping the "changed" from the conditional was not correct. Or at
least, it would do redundant work if nothing was "changed". So I put that
back. Let me know if that change was intentional or there is something else
going on.

I will work through the remaining patches. It looks good to me so far.

On Tue, Mar 25, 2025 at 10:18 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 21.03.25 06:58, Amul Sul wrote:
> >
> > [....]
> > Attached is the updated version, where the commit messages for patch
> > 0005 and 0006 have been slightly corrected. Additionally, a few code
> > comments have been updated to consistently use the ENFORCED/NOT
> > ENFORCED keywords. The rest of the patches and all the code are
> > unchanged.
>
> I have committed patches 0001 through 0003. I made some small changes:
>

Thank you very much !

> In 0001, I renamed the function UpdateConstraintEntry() to
> AlterConstrUpdateConstraintEntry() so the context is clearer.
>
> In 0002, you had this change:
>
> @@ -12113,75 +12154,89 @@ ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
> * If the table at either end of the constraint is partitioned, we need to
> * handle every constraint that is a child of this one.
> */
> - if (recurse && changed &&
> + if (recurse &&
> (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
> - (OidIsValid(refrelid) &&
> - get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)))
> - ATExecAlterChildConstr(wqueue, cmdcon, conrel, tgrel, rel, contuple,
> - recurse, otherrelids, lockmode);
> + get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE))
> + AlterConstrDeferrabilityRecurse(wqueue, cmdcon, conrel, tgrel, rel,
> + contuple, recurse, otherrelids,
> + lockmode);
>
> AFAICT, dropping the "changed" from the conditional was not correct. Or at
> least, it would do redundant work if nothing was "changed". So I put that
> back. Let me know if that change was intentional or there is something else
> going on.
>

Makes sense. This is intentional, but I must confess that this change
isn't part of the scope of this patch. I should have mentioned it when
posting, as it was something I intended to discuss with Álvaro, but it
slipped my mind.

The reason for the change is to revert to the behavior before commit
#80d7f990496b1c, where recursion occurred regardless of the
changed flags. This is also described in the header comment for
ATExecAlterConstrDeferrability() (earlier it was for
ATExecAlterConstraintInternal):

*
* Note that we must recurse even when the values are correct, in case
* indirect descendants have had their constraints altered locally.
* (This could be avoided if we forbade altering constraints in partitions
* but existing releases don't do that.)
*

Regards,
Amul

On 2025-Mar-26, Amul Sul wrote:

> The reason for the change is to revert to the behavior before commit
> #80d7f990496b1c, where recursion occurred regardless of the
> changed flags. This is also described in the header comment for
> ATExecAlterConstrDeferrability() (earlier it was for
> ATExecAlterConstraintInternal):
>
> * Note that we must recurse even when the values are correct, in case
> * indirect descendants have had their constraints altered locally.
> * (This could be avoided if we forbade altering constraints in partitions
> * but existing releases don't do that.)

Umm, why? Surely we should not allow a partition tree to become
inconsistent.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
#error "Operator lives in the wrong universe"
("Use of cookies in real-time system development", M. Gleixner, M. Mc Guire)