Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add support for NOT ENFORCED in foreign key constraints

  2. Expand test a bit

  3. refactor: Pass relation OID instead of Relation to createForeignKeyCheckTriggers()

  4. refactor: Split ATExecAlterConstraintInternal()

  5. refactor: Move some code that updates pg_constraint to a separate function

  6. Move RemoveInheritedConstraint() call slightly earlier

  7. refactor: Split tryAttachPartitionForeignKey()

  8. refactor: re-add ATExecAlterChildConstr()

  9. Add ATAlterConstraint struct for ALTER .. CONSTRAINT

  10. refactor: split ATExecAlterConstrRecurse()

  11. Add support for NOT ENFORCED in CHECK constraints

  1. NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-10-08T09:06:42Z

    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

  2. Re: NOT ENFORCED constraint feature

    Joel Jacobson <joel@compiler.org> — 2024-10-09T09:14:35Z

    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

  3. Re: NOT ENFORCED constraint feature

    Andrew Dunstan <andrew@dunslane.net> — 2024-10-09T13:15:12Z

    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

  4. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-10-15T04:30:00Z

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

Thanks for looking into it.

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

Ok, will fix it in the next version.

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

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

Regards,
Amul

  5. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-10-15T04:33:38Z

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

Agreed. Thanks for the inputs.

Regards,
Amul.

  6. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-11-04T12:17:27Z

    On Tue, Oct 15, 2024 at 10:00 AM Amul Sul <sulamul@gmail.com> wrote:
>
> On Wed, Oct 9, 2024 at 2:44 PM Joel Jacobson <joel@compiler.org> wrote:
> >
> > On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote:
> > > The attached patch proposes adding the ability to define CHECK and
> > > FOREIGN KEY constraints as NOT ENFORCED.
> >
> > Thanks for working on this!
> >
> > > Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch,
> >
> > I've looked at the 0001 patch and think it looks simple and straight forward.
> >
>
> Thanks for looking into it.
>
> > > but implementing it for FOREIGN KEY constraints requires more code due
> > > to triggers, see 0002 - 0005 patches.
> >
> > I can't say that much yet about the code changes in 0002 - 0005 yet,
> > but I've tested the patches and successfully experimented with the feature.
> >
> > Also think the documentation is good and sound. Only found a minor typo:
> > -    Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
> > +    Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
> >
>
> Ok, will fix it in the next version.

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

Regards,
Amul

  7. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2024-11-12T10:18:03Z

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

  8. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-11-14T13:40:03Z

    On Tue, Nov 12, 2024 at 3:48 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I started reviewing patch 0001 for check constraints.  I think it's a
> good idea how you structured it so that we can start with this
> relatively simple feature and get all the syntax parsing etc. right.
>
> I also looked over the remaining patches a bit.  The general structure
> looks right to me.  But I haven't done a detailed review yet.
>
Thank you for your feedback and suggestions.

> The 0001 patch needs a rebase over the recently re-committed patch for
> catalogued not-null constraints.  This might need a little work to
> verify that everything still makes sense.
>
> (I suppose technically we could support not-enforced not-null
> constraints.  But I would stay away from that for now.  That not-null
> constraints business is very complicated, don't get dragged into
> it. ;-) )
>
True. I had a quick conversation with Álvaro at PGConf.EU about my
current work and the pending ALTER CONSTRAINT support for CHECK
constraints. He mentioned that we might also need the same support for
NULL constraints. I'll look into that as well while working on ALTER
CONSTRAINT.

>
> Some more detailed comments on the code:
>
> * src/backend/access/common/tupdesc.c
>
> Try to keep the order of the fields consistent.  In tupdesc.h you have
> ccenforced before ccnoinherit, here you have it after.  Either way is
> fine, but let's keep it consistent.  (If you change it in tupdesc.h,
> also check relcache.c.)
>
Done.

>
> * src/backend/commands/tablecmds.c
>
>              cooked->skip_validation = false;
> +           cooked->is_enforced = true;
>              cooked->is_local = true;    /* not used for defaults */
>              cooked->inhcount = 0;   /* ditto */
>
> Add a comment like "not used for defaults" to the new line.
>
> Or maybe this should be rewritten slightly.  There might be more
> fields that are not used for defaults, like "skip_validation"?  Maybe
> they just shouldn't be set here, seems useless and confusing.
>
Yeah, setting here is confusing, I removed that we do not need.

> @@ -9481,6 +9484,9 @@ ATAddCheckConstraint(List **wqueue,
> AlteredTableInfo *tab, Relation rel,
>      {
>          CookedConstraint *ccon = (CookedConstraint *) lfirst(lcon);
>
> +       /* Only CHECK constraint can be not enforced */
> +       Assert(ccon->is_enforced || ccon->contype == CONSTRAINT_CHECK);
> +
>
> Is this assertion useful, since we are already in a function named
> ATAddCheckConstraint()?
>
Yes, Removed this.  Assertion in CreateConstraintEntry() is more than enough.


> @@ -11947,7 +11961,9 @@ ATExecValidateConstraint(List **wqueue, Relation
> rel, char *constrName,
>          }
>
>          /*
> -        * Now update the catalog, while we have the door open.
> +        * Now update the catalog regardless of enforcement; the validated
> +        * flag will not take effect until the constraint is marked as
> +        * enforced.
>           */
>
> Can you clarify what you mean here?  Is the enforced flag set later?
> I don't see that in the code.  What is the interaction between
> constraint validation and the enforced flag?
>
I revised the comment in the 0005 patch for clarity. My intent is
that, to trigger validation, the constraint must be enforced.
Additionally, when changing a constraint from non-enforced to
enforced, similar validation is triggered only if the constraint is
valid; otherwise, we simply update the constraint enforceability only.

>
> * src/backend/commands/typecmds.c
>
> You should also check and error if CONSTR_ATTR_ENFORCED is specified
> (even though it's effectively the default).  This matches SQL standard
> language: "For every <domain constraint> specified: ... If <constraint
> characteristics> is specified, then neither ENFORCED nor NOT ENFORCED
> shall be specified."
>
> The error code should be something like
> ERRCODE_INVALID_OBJECT_DEFINITION instead of
> ERRCODE_FEATURE_NOT_SUPPORTED.  The former is more for features that
> are impossible, the latter for features we haven't gotten to yet.
>
Understood. Fixed.

>
> * src/backend/parser/gram.y
>
> Same as above, in processCASbits(), you should add a similar check for
> CAS_ENFORCED, meaning that for example specifying UNIQUE ENFORCED is
> not allowed (even though it's the default).  This matches SQL standard
> language: "If <unique constraint definition> is specified, then
> <constraint characteristics> shall not specify a <constraint
> enforcement>."
>
Done. In processCASbits(), the error code will be
ERRCODE_FEATURE_NOT_SUPPORTED for consistency with the previous error.
Additionally, is_enforced = true is updated again in the CAS_ENFORCED
check block to align with the existing code style, which I believe is
reasonable.

>
> * src/backend/parser/parse_utilcmd.c
>
> @@ -1317,6 +1321,7 @@ expandTableLikeClause(RangeVar *heapRel,
> TableLikeClause *table_like_clause)
>              n->is_no_inherit = ccnoinherit;
>              n->raw_expr = NULL;
>              n->cooked_expr = nodeToString(ccbin_node);
> +           n->is_enforced = true;
>
> This has the effect that if you use the LIKE clause with INCLUDING
> CONSTRAINTS, the new constraint is always ENFORCED.  Is this what we
> want?  Did you have a reason?  I'm not sure what the ideal behavior
> might be.  But if we want it like this, maybe we should document this
> or at least put a comment here or something.
>
You are correct; this is a bug. It has been fixed in the attached
version, and tests have been added for it.

>
> * src/backend/utils/adt/ruleutils.c
>
> The syntax requires the NOT ENFORCED clause to be after DEFERRABLE
> etc., but this code does it the other way around.  You should move the
> new code after the switch statement and below the DEFERRABLE stuff.
>
> I wouldn't worry about restricting it based on constraint type.  The
> DEFERRABLE stuff doesn't do that either.  We can assume that the
> catalog contents are sane.
>
Done.

>
> * src/include/catalog/pg_constraint.h
>
> There needs to be an update in catalogs.sgml for the new catalog column.
>
Done.

>
> * src/test/regress/sql/constraints.sql
>
> Possible additional test cases:
> - trying [NOT] ENFORCED with a domain (CREATE and ALTER cases)
> - trying [NOT] ENFORCED with an unsupported constraint type (maybe UNIQUE)
>
Added. I thought about adding tests for all other constraints, but it
seemed excessive, so I decided not to.

>
> A note for the later patches: With patches 0001 through 0005 applied,
> I get compiler warnings:
>
> ../src/backend/commands/tablecmds.c:10918:17: error: 'deleteTriggerOid'
> may be used uninitialized [-Werror=maybe-uninitialized]
> ../src/backend/commands/tablecmds.c:10918:17: error: 'updateTriggerOid'
> may be used uninitialized [-Werror=maybe-uninitialized]
>
> (both with gcc and clang).
>
For some reason, my GCC 11.5 on the CentOS machine isn’t showing this
warning. However, I found the unassigned variable, fixed it, and tried
compiling on macOS, where it's now clean.

Updated version attached.

Regards,
Amul

  9. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-11-15T10:20:10Z

    On Thu, Nov 14, 2024 at 7:10 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Tue, Nov 12, 2024 at 3:48 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> Updated version attached.
>

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

Regards,
Amul

  10. Re: NOT ENFORCED constraint feature

    jian he <jian.universality@gmail.com> — 2024-11-18T12:42:56Z

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

hi.
i only played around with
v4-0001-Add-support-for-NOT-ENFORCED-in-CHECK-constraints.patch.

create table t(a int);
alter table t ADD CONSTRAINT the_constraint CHECK (a > 0) NOT ENFORCED;
insert into t select -1;
select  conname, contype,condeferrable,condeferred, convalidated,
conenforced,conkey,connoinherit
from    pg_constraint
where   conrelid = 't'::regclass;

pg_constraint->convalidated should be set to false for NOT ENFORCED constraint?
Am I missing something?


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


+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("misplaced ENFORCED clause"),
+ parser_errposition(cxt->pstate, con->location)));

+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("misplaced NOT ENFORCED clause"),
+ parser_errposition(cxt->pstate, con->location)));

https://www.merriam-webster.com/dictionary/misplace
says:
"to put in a wrong or inappropriate place"

I found the "misplaced" error message is not helpful.
for example:
CREATE TABLE UNIQUE_EN_TBL(i int UNIQUE ENFORCED);
ERROR:  misplaced ENFORCED clause
the error message only tells us thatspecify ENFORCED is wrong.
but didn't say why it's wrong.

we can saying that
"ENFORCED clauses can only be used for CHECK constraints"
------------------------------------------------------------------
the following queries is a bug?

drop table t;
create table t(a int);
alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED;
insert into t select -1;
alter table t add constraint cc1 check (a > 1) not ENFORCED not ENFORCED;
ERROR:  check constraint "cc1" of relation "t" is violated by some row
alter table t add constraint cc1 check (a > 1) not ENFORCED;
ERROR:  check constraint "cc1" of relation "t" is violated by some row

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

seems not easy to make it fail with alter table multiple "not enforced".
I guess it should be fine.
since we disallow a mix of "not enforced" and "enforced".

alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED enforced;
------------------------------------------------------------------
typedef struct Constraint
{
    NodeTag        type;
    ConstrType    contype;        /* see above */
    char       *conname;        /* Constraint name, or NULL if unnamed */
    bool        deferrable;        /* DEFERRABLE? */
    bool        initdeferred;    /* INITIALLY DEFERRED? */
    bool        skip_validation;    /* skip validation of existing rows? */
    bool        initially_valid;    /* mark the new constraint as valid? */
    bool        is_enforced;        /* enforced constraint? */
}
makeNode(Constraint) will default is_enforced to false.
Which makes the default value not what we want.
That means we may need to pay more attention for the trip from
makeNode(Constraint) to finally insert the constraint to the catalog.

if we change it to is_not_enforced, makeNode will default to false.
is_not_enforced is false, means the constraint is enforced.
which is not that intuitive...
------------------------------------------------------------------
do we need to update for "enforced" in
https://www.postgresql.org/docs/current/sql-keywords-appendix.html
?
------------------------------------------------------------------
seems don't have
ALTER TABLE <name> VALIDATE CONSTRAINT
interacts with not forced sql tests.
for example:

drop table if exists t;
create table t(a int);
alter table t add constraint cc check (a <> 1) not enforced NOT VALID;
insert into t values(1); ---success.
alter table t validate constraint cc;

select  conname,convalidated, conenforced
from    pg_constraint
where   conrelid = 't'::regclass;

returns:
 conname | convalidated | conenforced
---------+--------------+-------------
 cc      | t            | f

Now we have a value in the table "t" that violates the check
constraint, while convalidated is true.
----------------------------------------------------------------------------
i add more tests where it should fail.
also add a test case for `create table like INCLUDING CONSTRAINTS`
please check attached.

  11. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2024-11-18T14:23:27Z

    On 18.11.24 13:42, jian he wrote:
> i only played around with
> v4-0001-Add-support-for-NOT-ENFORCED-in-CHECK-constraints.patch.
> 
> create table t(a int);
> alter table t ADD CONSTRAINT the_constraint CHECK (a > 0) NOT ENFORCED;
> insert into t select -1;
> select  conname, contype,condeferrable,condeferred, convalidated,
> conenforced,conkey,connoinherit
> from    pg_constraint
> where   conrelid = 't'::regclass;
> 
> pg_constraint->convalidated should be set to false for NOT ENFORCED constraint?
> Am I missing something?

The "validated" status is irrelevant when the constraint is set to not 
enforced.  But it's probably still a good idea to make sure the field is 
set consistently.  I'm also leaning toward setting it to false.  One 
advantage of that would be that if you set the constraint to enforced 
later, then it's automatically in the correct "not validated" state.

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

Maybe "the latter is the default" would be clearer.

> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("misplaced ENFORCED clause"),
> + parser_errposition(cxt->pstate, con->location)));
> 
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("misplaced NOT ENFORCED clause"),
> + parser_errposition(cxt->pstate, con->location)));
> 
> https://www.merriam-webster.com/dictionary/misplace
> says:
> "to put in a wrong or inappropriate place"
> 
> I found the "misplaced" error message is not helpful.
> for example:
> CREATE TABLE UNIQUE_EN_TBL(i int UNIQUE ENFORCED);
> ERROR:  misplaced ENFORCED clause
> the error message only tells us thatspecify ENFORCED is wrong.
> but didn't say why it's wrong.
> 
> we can saying that
> "ENFORCED clauses can only be used for CHECK constraints"

This handling is similar to other error messages in 
transformConstraintAttrs().  It could be slightly improved, but it's not 
essential for this patch.

> ------------------------------------------------------------------
> the following queries is a bug?
> 
> drop table t;
> create table t(a int);
> alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED;
> insert into t select -1;
> alter table t add constraint cc1 check (a > 1) not ENFORCED not ENFORCED;
> ERROR:  check constraint "cc1" of relation "t" is violated by some row
> alter table t add constraint cc1 check (a > 1) not ENFORCED;
> ERROR:  check constraint "cc1" of relation "t" is violated by some row
> 
> ------------------------------------------------------------------
> drop table t;
> create table t(a int);
> alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED not enforced;
> 
> seems not easy to make it fail with alter table multiple "not enforced".
> I guess it should be fine.
> since we disallow a mix of "not enforced" and "enforced".
> 
> alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED enforced;
> ------------------------------------------------------------------

Hmm, these duplicate clauses should have been caught by 
transformConstraintAttrs().

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

Yes, it could be safer to make the field so that the default is false. 
I guess the skip_validation field is like that for a similar reason, but 
I'm not sure.

> ------------------------------------------------------------------
> do we need to update for "enforced" in
> https://www.postgresql.org/docs/current/sql-keywords-appendix.html
> ?
> ------------------------------------------------------------------

That is generated automatically.

> seems don't have
> ALTER TABLE <name> VALIDATE CONSTRAINT
> interacts with not forced sql tests.
> for example:
> 
> drop table if exists t;
> create table t(a int);
> alter table t add constraint cc check (a <> 1) not enforced NOT VALID;
> insert into t values(1); ---success.
> alter table t validate constraint cc;
> 
> select  conname,convalidated, conenforced
> from    pg_constraint
> where   conrelid = 't'::regclass;
> 
> returns:
>   conname | convalidated | conenforced
> ---------+--------------+-------------
>   cc      | t            | f
> 
> Now we have a value in the table "t" that violates the check
> constraint, while convalidated is true.
> ----------------------------------------------------------------------------

I think we should prevent running VALIDATE for not enforced constraints. 
  I don't know what that would otherwise mean.

It's also questionable whether NOT VALID makes sense to specify.

  12. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-12-03T12:00:20Z

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

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

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

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

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

Done.

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

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

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

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

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

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

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

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

Regards,
Amul

  13. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-03T12:59:02Z

    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?

  14. Re: NOT ENFORCED constraint feature

    jian he <jian.universality@gmail.com> — 2024-12-04T08:10:20Z

    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.

  15. Re: NOT ENFORCED constraint feature

    jian he <jian.universality@gmail.com> — 2024-12-04T10:43:32Z

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

  16. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-12-04T11:04:23Z

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

Your are correct, will move this to 0005 patch.

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

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

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

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

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

Ok.

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

I think ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE would be much suitable.

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

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

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

Regards,
Amul

  17. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-12-04T13:02:33Z

    On Tue, Dec 3, 2024 at 6:29 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 03.12.24 13:00, Amul Sul wrote:
> [....]
> >
> > Ok. Initially, I was doing it the same way, but to maintain consistency
> > with the pg_constraint column and avoid negation in multiple places, I
> > chose that approach. However, I agree that having the default to false
> > would be safer. I’ve renamed the flag to is_not_enforced. Other names
> > I considered were not_enforced or is_unenforced, but since we already
> > have existing flags with two underscores, is_not_enforced shouldn't be
> > a problem.
>
> I was initially thinking about this as well, but after seeing it now, I
> don't think this is a good change.  Because now we have both "enforced"
> and "not_enforced" sprinkled around the code.  If we were to do this
> consistently everywhere, then it might make sense, but this way it's
> just confusing.  The Constraint struct is only initialized in a few
> places, so I think we can be careful there.  Also note that the field
> initially_valid is equally usually true.
>

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

> I could of other notes on patch 0001:
>
> Update information_schema table_constraint.enforced (see
> src/backend/catalog/information_schema.sql and
> doc/src/sgml/information_schema.sgml).
>

Done.

> The handling of merging check constraints seems incomplete.  What should
> be the behavior of this:
>
> => create table p1 (a int check (a > 0) not enforced);
> CREATE TABLE
> => create table c1 (a int check (a > 0) enforced) inherits (p1);
> CREATE TABLE
>
> Or this?
>
> => create table p2 (a int check (a > 0) enforced);
> CREATE TABLE
> => create table c2 () inherits (p1, p2);
> CREATE TABLE
>
> Should we catch these and error?
>

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

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

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

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

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

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

Regards,
Amul

  18. Re: NOT ENFORCED constraint feature

    jian he <jian.universality@gmail.com> — 2024-12-05T05:32:20Z

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

  19. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-12-05T11:10:18Z

    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/

  20. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-12-06T09:13:51Z

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

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

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

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

Regards,
Amul

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

  21. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-12-09T10:11:48Z

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

  22. Re: NOT ENFORCED constraint feature

    jian he <jian.universality@gmail.com> — 2024-12-09T16:10:15Z

    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;

  23. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-12-09T17:29:02Z

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

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

Regards,
Amul

  24. Re: NOT ENFORCED constraint feature

    jian he <jian.universality@gmail.com> — 2024-12-10T07:50:57Z

    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?

  25. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-12-10T11:47:25Z

    On Tue, Dec 10, 2024 at 1:21 PM jian he <jian.universality@gmail.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

  26. Re: NOT ENFORCED constraint feature

    jian he <jian.universality@gmail.com> — 2024-12-11T12:42:26Z

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

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


I am playing around with the remaining patch.

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

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

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

The above arguments are pretty intuitive.

Constraint *cmdcon
Relation conrel
Relation tgrel
HeapTuple contuple

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

  27. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2024-12-11T12:58:13Z

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

Thank you.

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

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

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

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

Regards,
Amul

  28. Re: NOT ENFORCED constraint feature

    Triveni N <triveni.n@enterprisedb.com> — 2025-01-08T04:41:07Z

    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.ahuja@enterprisedb.com>
wrote:

> FYI
>
> ---------- Forwarded message ---------
> From: Amul Sul <sulamul@gmail.com>
> Date: Tue, Dec 10, 2024 at 5:18 PM
> Subject: Re: NOT ENFORCED constraint feature
> To: jian he <jian.universality@gmail.com>
> Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Peter Eisentraut <
> peter@eisentraut.org>, pgsql-hackers <pgsql-hackers@postgresql.org>, Joel
> Jacobson <joel@compiler.org>
>
>
> On Tue, Dec 10, 2024 at 1:21 PM jian he <jian.universality@gmail.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

  29. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-11T17:13:35Z

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

  30. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-01-11T17:26:32Z

    On Saturday, 11 January 2025, Peter Eisentraut <peter@eisentraut.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

  31. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-16T12:37:56Z

    On 11.01.25 18:26, Amul Sul wrote:
> On Saturday, 11 January 2025, Peter Eisentraut <peter@eisentraut.org 
> <mailto:peter@eisentraut.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.

  32. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-01-16T13:25:43Z

    On Thu, Jan 16, 2025 at 6:07 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 11.01.25 18:26, Amul Sul wrote:
> > On Saturday, 11 January 2025, Peter Eisentraut <peter@eisentraut.org
> > <mailto:peter@eisentraut.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

  33. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-01-17T12:50:15Z

    On Thu, Jan 16, 2025 at 6:55 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Thu, Jan 16, 2025 at 6:07 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > On 11.01.25 18:26, Amul Sul wrote:
> > > On Saturday, 11 January 2025, Peter Eisentraut <peter@eisentraut.org
> > > <mailto:peter@eisentraut.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

  34. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-01-20T16:53:14Z

    On Fri, Jan 17, 2025 at 6:20 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Thu, Jan 16, 2025 at 6:55 PM Amul Sul <sulamul@gmail.com> wrote:
> >
> > On Thu, Jan 16, 2025 at 6:07 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> > >
> > > On 11.01.25 18:26, Amul Sul wrote:
> > > > On Saturday, 11 January 2025, Peter Eisentraut <peter@eisentraut.org
> > > > <mailto:peter@eisentraut.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

  35. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-28T10:31:48Z

    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.

  36. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-01-28T10:58:00Z

    On Tue, Jan 28, 2025 at 4:01 PM Peter Eisentraut <peter@eisentraut.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,

  37. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-28T16:17:07Z

    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.

  38. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-01-29T12:47:09Z

    On Tue, Jan 28, 2025 at 9:47 PM Peter Eisentraut <peter@eisentraut.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

  39. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-01-31T12:59:11Z

    On Wed, Jan 29, 2025 at 6:18 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Tue, Jan 28, 2025 at 9:47 PM Peter Eisentraut <peter@eisentraut.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

  40. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-31T13:40:50Z

    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/

  41. Re: NOT ENFORCED constraint feature

    jian he <jian.universality@gmail.com> — 2025-02-01T15:01:22Z

    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?

  42. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-03T04:09:25Z

    On Sat, Feb 1, 2025 at 8:31 PM jian he <jian.universality@gmail.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

  43. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-03T04:27:18Z

    On Fri, Jan 31, 2025 at 7:10 PM Alvaro Herrera <alvherre@alvh.no-ip.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

  44. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-02-03T05:19:37Z

    On Mon, Feb 3, 2025 at 9:57 AM Amul Sul <sulamul@gmail.com> wrote:
>
> On Fri, Jan 31, 2025 at 7:10 PM Alvaro Herrera <alvherre@alvh.no-ip.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

  45. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-03T05:30:00Z

    On Mon, Feb 3, 2025 at 10:49 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Mon, Feb 3, 2025 at 9:57 AM Amul Sul <sulamul@gmail.com> wrote:
> >
> > On Fri, Jan 31, 2025 at 7:10 PM Alvaro Herrera <alvherre@alvh.no-ip.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

  46. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-03T07:50:41Z

    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"

  47. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-02-03T11:46:09Z

    On Mon, Feb 3, 2025 at 1:20 PM Alvaro Herrera <alvherre@alvh.no-ip.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

  48. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-03T12:19:30Z

    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/

  49. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-02-04T04:39:29Z

    On Mon, Feb 3, 2025 at 5:55 PM Alvaro Herrera <alvherre@alvh.no-ip.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

  50. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-04T10:34:57Z

    On Mon, Feb 3, 2025 at 9:39 AM Amul Sul <sulamul@gmail.com> wrote:
>
> On Sat, Feb 1, 2025 at 8:31 PM jian he <jian.universality@gmail.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

  51. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-04T13:52:35Z

    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.

  52. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-04T13:54:28Z

    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.

  53. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-04T13:56:52Z

    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.

  54. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-04T14:22:47Z

    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

  55. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-10T06:03:06Z

    On Tue, Feb 4, 2025 at 7:22 PM Peter Eisentraut <peter@eisentraut.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

  56. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-10T18:48:08Z

    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.

  57. Re: NOT ENFORCED constraint feature

    Isaac Morland <isaac.morland@gmail.com> — 2025-02-10T19:30:52Z

    On Mon, 10 Feb 2025 at 13:48, Álvaro Herrera <alvherre@alvh.no-ip.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.

  58. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-11T13:36:05Z

    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/

  59. Re: NOT ENFORCED constraint feature

    Isaac Morland <isaac.morland@gmail.com> — 2025-02-11T15:39:35Z

    On Tue, 11 Feb 2025 at 08:36, Álvaro Herrera <alvherre@alvh.no-ip.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).

  60. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-02-12T04:47:07Z

    On Tue, Feb 11, 2025 at 9:09 PM Isaac Morland <isaac.morland@gmail.com> wrote:
>
> On Tue, 11 Feb 2025 at 08:36, Álvaro Herrera <alvherre@alvh.no-ip.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

  61. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-12T11:13:52Z

    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)

  62. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-12T12:41:22Z

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

  63. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-12T14:45:36Z

    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)

  64. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-02-13T05:34:10Z

    On Wed, Feb 12, 2025 at 8:15 PM Peter Eisentraut <peter@eisentraut.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

  65. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-13T11:57:03Z

    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)

  66. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-02-14T15:11:14Z

    On Thu, Feb 13, 2025 at 5:27 PM Álvaro Herrera <alvherre@alvh.no-ip.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

  67. Re: NOT ENFORCED constraint feature

    Isaac Morland <isaac.morland@gmail.com> — 2025-02-14T15:15:07Z

    On Fri, 14 Feb 2025 at 10:11, Ashutosh Bapat <ashutosh.bapat.oss@gmail.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.

  68. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-17T04:05:56Z

    On Fri, Feb 14, 2025 at 8:41 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Feb 13, 2025 at 5:27 PM Álvaro Herrera <alvherre@alvh.no-ip.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

  69. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-17T10:11:20Z

    On Tue, Feb 11, 2025 at 12:18 AM Álvaro Herrera <alvherre@alvh.no-ip.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

  70. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-17T19:17:23Z

    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/

  71. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-18T05:02:26Z

    On Tue, Feb 18, 2025 at 12:47 AM Álvaro Herrera <alvherre@alvh.no-ip.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

  72. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-18T08:43:35Z

    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/

  73. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-27T06:56:33Z

    On Tue, Feb 18, 2025 at 2:13 PM Álvaro Herrera <alvherre@alvh.no-ip.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

  74. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-27T11:18:51Z

    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)

  75. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-02-28T04:57:58Z

    On Thu, Feb 27, 2025 at 4:48 PM Álvaro Herrera <alvherre@alvh.no-ip.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

  76. Re: NOT ENFORCED constraint feature

    Alexandra Wang <alexandra.wang.oss@gmail.com> — 2025-03-06T16:06:50Z

    Hi Amul,

On Thu, Feb 27, 2025 at 12:57 AM Amul Sul <sulamul@gmail.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

  77. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-10T09:42:33Z

    On Thu, Mar 6, 2025 at 9:37 PM Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:
>
> Hi Amul,
>
> On Thu, Feb 27, 2025 at 12:57 AM Amul Sul <sulamul@gmail.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

  78. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-11T09:07:52Z

    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"

  79. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-11T17:43:48Z

    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.

  80. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-12T10:02:07Z

    On Tue, Mar 11, 2025 at 11:13 PM Peter Eisentraut <peter@eisentraut.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

  81. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-18T19:03:30Z

    On 2025-Mar-12, Amul Sul wrote:

> On Tue, Mar 11, 2025 at 11:13 PM Peter Eisentraut <peter@eisentraut.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/

  82. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-21T05:58:44Z

    On Wed, Mar 19, 2025 at 12:33 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-12, Amul Sul wrote:
>
> > On Tue, Mar 11, 2025 at 11:13 PM Peter Eisentraut <peter@eisentraut.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

  83. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-25T16:48:46Z

    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.

  84. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-26T04:02:33Z

    On Tue, Mar 25, 2025 at 10:18 PM Peter Eisentraut <peter@eisentraut.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

  85. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-26T06:59:31Z

    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)

  86. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-27T04:48:46Z

    On Wed, Mar 26, 2025 at 12:29 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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.
>

I just checked, and we are not allowed to alter a constraint on the
child table alone, nor can we merge it when attaching to the parent
constraint if the deferrability is different. Therefore, I think we
should remove this comment as it seems outdated now.

Regards,
Amul

  87. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-03-27T05:35:30Z

    On Wed, Mar 26, 2025 at 9:33 AM Amul Sul <sulamul@gmail.com> wrote:

> On Tue, Mar 25, 2025 at 10:18 PM Peter Eisentraut <peter@eisentraut.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 !
>
> I wanted to run my dump and restore test on this patch set but it doesn't
apply cleanly. I tried applying 0004-0006. 0004 applies but not 0005.

I reviewed the tests to see if they leave objects in enough varied states
for 002_pg_upgrade to test it well. The test frequently drops and recreates
same partitioned and non-partitioned table inducing different states. So I
have a feeling that we are just leaving one state combination behind and
not all. It's an existing problem but probably with enforced and valid
states it becomes more serious. I ended up reviewing the tests. Here are
some comments.

+-- Reverting it back to ENFORCED will result in failure because constraint
validation will be triggered,
+-- as it was previously in a valid state.
+ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
+ERROR:  insert or update on table "fktable" violates foreign key
constraint "fktable_ftest1_fkey"
+DETAIL:  Key (ftest1)=(1) is not present in table "pktable".

The text of error is misleading. There's no INSERT or UPDATE happening it's
ALTER. That's what VALIDATE CONSTRAINT also reports, so not fault of this
patch. But thought of writing it here since I noticed this now.

+-- Remove any existing rows that violate the constraint, then attempt to
change
+-- it.
+TRUNCATE FKTABLE;

I think, it will be good if we don't remove all the data; we wouldn't know
whether constraint was considered validated because there's no data or it
actually scanned the table and found no rows violating the constraint.
Let's leave/insert one row which obeys the constraint.

+-- Modifying other attributes of a constraint should not affect its
enforceability, and vice versa
+ALTER TABLE FKTABLE ADD CONSTRAINT fk_con FOREIGN KEY(ftest1, ftest2)
REFERENCES PKTABLE NOT VALID NOT ENFORCED;
+ALTER TABLE FKTABLE ALTER CONSTRAINT fk_con DEFERRABLE INITIALLY DEFERRED;
+SELECT condeferrable, condeferred, conenforced, convalidated
+FROM pg_constraint WHERE conname = 'fk_con';
+ condeferrable | condeferred | conenforced | convalidated
+---------------+-------------+-------------+--------------
+ t             | t           | f           | f
+(1 row)
+
+ALTER TABLE FKTABLE ALTER CONSTRAINT fk_con NOT ENFORCED;
+SELECT condeferrable, condeferred, conenforced, convalidated
+FROM pg_constraint WHERE conname = 'fk_con';
+ condeferrable | condeferred | conenforced | convalidated
+---------------+-------------+-------------+--------------
+ t             | t           | f           | f
+(1 row)

The statement is marking a NOT ENFORCED constraint as NOT ENFORCED. What is
it trying to test?

+
+ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT ENFORCED;
+BEGIN;
+-- doesn't match FK, no error.

A "but" before no error would make the comment clearer.

+UPDATE pktable SET id = 10 WHERE id = 5;
+-- doesn't match PK, no error.

A "but" before no error would make the comment clearer.

 CREATE TABLE fk_partitioned_fk_1 (fdrop1 int, fdrop2 int, a int, fdrop3
int, b int);
 ALTER TABLE fk_partitioned_fk_1 DROP COLUMN fdrop1, DROP COLUMN fdrop2,
DROP COLUMN fdrop3;
 ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_1 FOR
VALUES FROM (0,0) TO (1000,1000);
-ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES
fk_notpartitioned_pk;
+ALTER TABLE fk_partitioned_fk ADD CONSTRAINT fk_partitioned_fk_a_b_fkey
FOREIGN KEY (a, b)
+ REFERENCES fk_notpartitioned_pk NOT ENFORCED;
 CREATE TABLE fk_partitioned_fk_2 (b int, fdrop1 int, fdrop2 int, a int);
 ALTER TABLE fk_partitioned_fk_2 DROP COLUMN fdrop1, DROP COLUMN fdrop2;
+ALTER TABLE fk_partitioned_fk_2 ADD FOREIGN KEY (a, b) REFERENCES
fk_notpartitioned_pk NOT ENFORCED;
 ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR
VALUES FROM (1000,1000) TO (2000,2000);
+ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey
ENFORCED;

I don't understand what value this change is bringing? Maybe a comment
explaining it. Why did we change name of one constraint and not the other?

 ERROR:  insert or update on table "fk_partitioned_fk_1" violates foreign
key constraint "fk_partitioned_fk_a_b_fkey"
 DETAIL:  Key (a, b)=(500, 501) is not present in table
"fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk (a,b) VALUES (1500, 1501);
-ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign
key constraint "fk_partitioned_fk_a_b_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign
key constraint "fk_partitioned_fk_2_a_b_fkey"
 DETAIL:  Key (a, b)=(1500, 1501) is not present in table
"fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk_2 (a,b) VALUES (1500, 1501);
-ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign
key constraint "fk_partitioned_fk_a_b_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign
key constraint "fk_partitioned_fk_2_a_b_fkey"
 DETAIL:  Key (a, b)=(1500, 1501) is not present in table
"fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk (a,b) VALUES (2500, 2502);
 ERROR:  insert or update on table "fk_partitioned_fk_3_1" violates foreign
key constraint "fk_partitioned_fk_a_b_fkey"

These diffs would go away if we didn't rename the constraint.

@@ -1667,6 +1753,37 @@ Indexes:
 Referenced by:
     TABLE "fk_partitioned_fk" CONSTRAINT "fk_partitioned_fk_a_b_fkey"
FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk(a, b)

+-- Check the exsting FK trigger
+CREATE TEMP TABLE tmp_trg_info AS SELECT tgtype, tgrelid::regclass,
tgconstraint
+FROM pg_trigger
+WHERE tgrelid IN (SELECT relid FROM
pg_partition_tree('fk_partitioned_fk'::regclass)
+ UNION ALL SELECT 'fk_notpartitioned_pk'::regclass);
+SELECT count(1) FROM tmp_trg_info;
+ count
+-------
+    14
+(1 row)
+
+ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey
NOT ENFORCED;
+-- No triggers
+SELECT count(1) FROM pg_trigger
+WHERE tgrelid IN (SELECT relid FROM
pg_partition_tree('fk_partitioned_fk'::regclass)
+ UNION ALL SELECT 'fk_notpartitioned_pk'::regclass);
+ count
+-------
+     0
+(1 row)
+
+-- Changing it back to ENFORCED will recreate the necessary triggers.
+ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey
ENFORCED;
+-- Should be exactly the same number of triggers found as before
+SELECT COUNT(1) FROM pg_trigger pgt JOIN tmp_trg_info tt
+ON (pgt.tgtype = tt.tgtype AND pgt.tgrelid = tt.tgrelid AND
pgt.tgconstraint = tt.tgconstraint);
+ count
+-------
+    14
+(1 row)

Why don't we just print the relevant triggers. Just matching counts could
be misleading - we may have added one and dropped another keeping the count
same.

+BEGIN;
+ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey
NOT ENFORCED;
+ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR
VALUES IN (1500,1502);
+-- should have only one constraint
+\d fk_partitioned_fk_2
+        Table "public.fk_partitioned_fk_2"
+ Column |  Type   | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ b      | integer |           |          |
+ a      | integer |           |          |
+Partition of: fk_partitioned_fk FOR VALUES IN (1500, 1502)
+Foreign-key constraints:
+    TABLE "fk_partitioned_fk" CONSTRAINT "fk_partitioned_fk_a_b_fkey"
FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk(a, b) ON UPDATE CASCADE
ON DELETE CASCADE NOT ENFORCED

Did it just rename an existing constraint on fk_partitioned_fk_2? Is that
ok? I thought we just mark the constraint as inherited.

-- 
Best Wishes,
Ashutosh Bapat

  88. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-27T12:54:58Z

    On Thu, Mar 27, 2025 at 11:05 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
>
>
> On Wed, Mar 26, 2025 at 9:33 AM Amul Sul <sulamul@gmail.com> wrote:
>>
>> On Tue, Mar 25, 2025 at 10:18 PM Peter Eisentraut <peter@eisentraut.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 !
>>
> I wanted to run my dump and restore test on this patch set but it doesn't apply cleanly. I tried applying 0004-0006. 0004 applies but not 0005.
>

Yes, I failed to notice that due to the previous patch commit with
Peter's improvements, this has started failing to apply. I've attached
the rebased version.

> I reviewed the tests to see if they leave objects in enough varied states for 002_pg_upgrade to test it well. The test frequently drops and recreates same partitioned and non-partitioned table inducing different states. So I have a feeling that we are just leaving one state combination behind and not all. It's an existing problem but probably with enforced and valid states it becomes more serious. I ended up reviewing the tests. Here are some comments.
>

Thanks for the review.

> +-- Reverting it back to ENFORCED will result in failure because constraint validation will be triggered,
> +-- as it was previously in a valid state.
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fktable_ftest1_fkey ENFORCED;
> +ERROR:  insert or update on table "fktable" violates foreign key constraint "fktable_ftest1_fkey"
> +DETAIL:  Key (ftest1)=(1) is not present in table "pktable".
>
> The text of error is misleading. There's no INSERT or UPDATE happening it's ALTER. That's what VALIDATE CONSTRAINT also reports, so not fault of this patch. But thought of writing it here since I noticed this now.
>

Yes, this is an existing issue, and Peter too mentioned the same in his
previous review[1].

> +-- Remove any existing rows that violate the constraint, then attempt to change
> +-- it.
> +TRUNCATE FKTABLE;
>
> I think, it will be good if we don't remove all the data; we wouldn't know whether constraint was considered validated because there's no data or it actually scanned the table and found no rows violating the constraint. Let's leave/insert one row which obeys the constraint.
>

Makes sense, did it that way.

> +-- Modifying other attributes of a constraint should not affect its enforceability, and vice versa
> +ALTER TABLE FKTABLE ADD CONSTRAINT fk_con FOREIGN KEY(ftest1, ftest2) REFERENCES PKTABLE NOT VALID NOT ENFORCED;
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fk_con DEFERRABLE INITIALLY DEFERRED;
> +SELECT condeferrable, condeferred, conenforced, convalidated
> +FROM pg_constraint WHERE conname = 'fk_con';
> + condeferrable | condeferred | conenforced | convalidated
> +---------------+-------------+-------------+--------------
> + t             | t           | f           | f
> +(1 row)
> +
> +ALTER TABLE FKTABLE ALTER CONSTRAINT fk_con NOT ENFORCED;
> +SELECT condeferrable, condeferred, conenforced, convalidated
> +FROM pg_constraint WHERE conname = 'fk_con';
> + condeferrable | condeferred | conenforced | convalidated
> +---------------+-------------+-------------+--------------
> + t             | t           | f           | f
> +(1 row)
>
> The statement is marking a NOT ENFORCED constraint as NOT ENFORCED. What is it trying to test?
>

There was previously a bug that caused changes to other attributes.
Here, I wanted to verify that nothing changes if the constraint is
already in the desired state.

> +
> +ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT ENFORCED;
> +BEGIN;
> +-- doesn't match FK, no error.
>
> A "but" before no error would make the comment clearer.
>

Done.

> +UPDATE pktable SET id = 10 WHERE id = 5;
> +-- doesn't match PK, no error.
>
> A "but" before no error would make the comment clearer.
>

Done.

>  CREATE TABLE fk_partitioned_fk_1 (fdrop1 int, fdrop2 int, a int, fdrop3 int, b int);
>  ALTER TABLE fk_partitioned_fk_1 DROP COLUMN fdrop1, DROP COLUMN fdrop2, DROP COLUMN fdrop3;
>  ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_1 FOR VALUES FROM (0,0) TO (1000,1000);
> -ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk;
> +ALTER TABLE fk_partitioned_fk ADD CONSTRAINT fk_partitioned_fk_a_b_fkey FOREIGN KEY (a, b)
> + REFERENCES fk_notpartitioned_pk NOT ENFORCED;
>  CREATE TABLE fk_partitioned_fk_2 (b int, fdrop1 int, fdrop2 int, a int);
>  ALTER TABLE fk_partitioned_fk_2 DROP COLUMN fdrop1, DROP COLUMN fdrop2;
> +ALTER TABLE fk_partitioned_fk_2 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT ENFORCED;
>  ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000);
> +ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey ENFORCED;
>
> I don't understand what value this change is bringing? Maybe a comment explaining it. Why did we change name of one constraint and not the other?
>

Earlier, I used the system-generated constraint name in the ALTER
TABLE command, but I was advised[1] to use an explicit name instead.
As a result, I started using an explicit name when creating the
constraint on the fk_partitioned_fk table, which I plan to modify
later. However, I didn't take into account the other table that wasn't
being modified, such as the constraint for fk_partitioned_fk_2.

>  ERROR:  insert or update on table "fk_partitioned_fk_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
>  DETAIL:  Key (a, b)=(500, 501) is not present in table "fk_notpartitioned_pk".
>  INSERT INTO fk_partitioned_fk (a,b) VALUES (1500, 1501);
> -ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
> +ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_b_fkey"
>  DETAIL:  Key (a, b)=(1500, 1501) is not present in table "fk_notpartitioned_pk".
>  INSERT INTO fk_partitioned_fk_2 (a,b) VALUES (1500, 1501);
> -ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
> +ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_b_fkey"
>  DETAIL:  Key (a, b)=(1500, 1501) is not present in table "fk_notpartitioned_pk".
>  INSERT INTO fk_partitioned_fk (a,b) VALUES (2500, 2502);
>  ERROR:  insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
>
> These diffs would go away if we didn't rename the constraint.
>

The next patch, 0006, will revert this diff where it removes the ALTER
command that adds the constraint to fk_partitioned_fk_2. Previously,
the fk_partitioned_fk_2 table inherited its constraint from the parent
via the ATTACH operation, resulting in the same name as the parent
constraint. However, this patch explicitly created the constraint,
which led to a different name than when it was inherited.
Nevertheless, I fix this by explicitly specifying the constraint
name to match the parent constraint in the ALTER command to minimize
the diff in the 0005 patch.

> @@ -1667,6 +1753,37 @@ Indexes:
>  Referenced by:
>      TABLE "fk_partitioned_fk" CONSTRAINT "fk_partitioned_fk_a_b_fkey" FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk(a, b)
>
> +-- Check the exsting FK trigger
> +CREATE TEMP TABLE tmp_trg_info AS SELECT tgtype, tgrelid::regclass, tgconstraint
> +FROM pg_trigger
> +WHERE tgrelid IN (SELECT relid FROM pg_partition_tree('fk_partitioned_fk'::regclass)
> + UNION ALL SELECT 'fk_notpartitioned_pk'::regclass);
> +SELECT count(1) FROM tmp_trg_info;
> + count
> +-------
> +    14
> +(1 row)
> +
> +ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey NOT ENFORCED;
> +-- No triggers
> +SELECT count(1) FROM pg_trigger
> +WHERE tgrelid IN (SELECT relid FROM pg_partition_tree('fk_partitioned_fk'::regclass)
> + UNION ALL SELECT 'fk_notpartitioned_pk'::regclass);
> + count
> +-------
> +     0
> +(1 row)
> +
> +-- Changing it back to ENFORCED will recreate the necessary triggers.
> +ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey ENFORCED;
> +-- Should be exactly the same number of triggers found as before
> +SELECT COUNT(1) FROM pg_trigger pgt JOIN tmp_trg_info tt
> +ON (pgt.tgtype = tt.tgtype AND pgt.tgrelid = tt.tgrelid AND pgt.tgconstraint = tt.tgconstraint);
> + count
> +-------
> +    14
> +(1 row)
>
> Why don't we just print the relevant triggers. Just matching counts could be misleading - we may have added one and dropped another keeping the count same.
>

I am not sure how to make such tests stable enough since the trigger
name involves OIDs. In count check, I tried adjusting the join
condition to ensure that I get the exact same type of constraint
w.r.t. trigger relation and the constraint.

> +BEGIN;
> +ALTER TABLE fk_partitioned_fk ALTER CONSTRAINT fk_partitioned_fk_a_b_fkey NOT ENFORCED;
> +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES IN (1500,1502);
> +-- should have only one constraint
> +\d fk_partitioned_fk_2
> +        Table "public.fk_partitioned_fk_2"
> + Column |  Type   | Collation | Nullable | Default
> +--------+---------+-----------+----------+---------
> + b      | integer |           |          |
> + a      | integer |           |          |
> +Partition of: fk_partitioned_fk FOR VALUES IN (1500, 1502)
> +Foreign-key constraints:
> +    TABLE "fk_partitioned_fk" CONSTRAINT "fk_partitioned_fk_a_b_fkey" FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk(a, b) ON UPDATE CASCADE ON DELETE CASCADE NOT ENFORCED
>
> Did it just rename an existing constraint on fk_partitioned_fk_2? Is that ok? I thought we just mark the constraint as inherited.
>

This is the existing behavior of the psql meta command, which hides
the child constraint name when it inherits the parent constraint.


Regards,
Amul

1] http://postgr.es/m/CAAJ_b94PTcHzp2BqOxQZ7S-Zxp2hGV192a=4V8dTifjypDPpEw@mail.gmail.com

  89. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-27T12:58:26Z

    On 25.03.25 17:48, Peter Eisentraut wrote:
> I have committed patches 0001 through 0003.  I made some small changes:

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

For the time being, here are the remaining patches rebased.

I think you could squash these together at this point.  This is 
especially useful since 0003 overwrites part of the changes in 0002, so 
it's better to see them all together.

Some details:

In CloneFkReferenced() and also in DetachPartitionFinalize(), you have 
this change:

-       fkconstraint->initially_valid = true;
+       fkconstraint->initially_valid = constrForm->convalidated;

I'm having a hard time understanding this.  Is this an pre-existing 
problem?  What does this change do?

Most of the other stuff is mechanical and fits into existing structures, 
so it seems ok.

Small cosmetic suggestion: write count(*) instead of count(1).  This 
fits better with existing practices.

The merge rules for inheritance and partitioning are still confusing.  I 
don't understand the behavior inh_check_constraint10 in the inherit.sql 
test:

+-- the invalid state of the child constraint will be ignored here.
+alter table p1 add constraint inh_check_constraint10 check (f1 < 10) 
not enforced;
+alter table p1_c1 add constraint inh_check_constraint10 check (f1 < 10) 
not valid enforced;

Why is that correct?  At least we should explain it here, or somewhere.

I'm also confused about the changes of the constraint names in the 
foreign_key.sql test:

-ERROR:  insert or update on table "fk_partitioned_fk_2" violates 
foreign key constraint "fk_partitioned_fk_a_b_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_2" violates 
foreign key constraint "fk_partitioned_fk_2_a_b_fkey"

And then patch 0003 changes it again.  This seems pretty random.  We 
should make sure that tests don't contain unrelated changes like that. 
(Or at least it's not clear why they are related.)

There is also in 0002

+-- should be having two constraints

and then in 0003:

--- should be having two constraints
+-- should only have one constraint

So another reason for squashing these together, so we don't have 
confusing intermediate states.

That said, is there a simpler way?  Patch 0003 appears to add a lot of 
complexity.  Could we make this simpler by saying, if you have otherwise 
matching constraints with different enforceability, make this an error. 
Then users can themselves adjust the enforceability how they want to 
make it match.

  90. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-27T13:36:52Z

    On Thu, Mar 27, 2025 at 6:28 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 25.03.25 17:48, Peter Eisentraut wrote:
> > I have committed patches 0001 through 0003.  I made some small changes:
>
> > I will work through the remaining patches.  It looks good to me so far.
>
> For the time being, here are the remaining patches rebased.
>
> I think you could squash these together at this point.  This is
> especially useful since 0003 overwrites part of the changes in 0002, so
> it's better to see them all together.
>
> Some details:
>
> In CloneFkReferenced() and also in DetachPartitionFinalize(), you have
> this change:
>
> -       fkconstraint->initially_valid = true;
> +       fkconstraint->initially_valid = constrForm->convalidated;
>
> I'm having a hard time understanding this.  Is this an pre-existing
> problem?  What does this change do?
>

No issue for the master head since constraints are always valid for
newly created tables. However, I wanted to ensure that the validation
status aligns with enforceability. When constraints are not enforced,
the convalidated flag must be false, so I didn't want to mark it as
true blindly, so fetching its value.


> Most of the other stuff is mechanical and fits into existing structures,
> so it seems ok.
>
> Small cosmetic suggestion: write count(*) instead of count(1).  This
> fits better with existing practices.
>

Ok.

> The merge rules for inheritance and partitioning are still confusing.  I
> don't understand the behavior inh_check_constraint10 in the inherit.sql
> test:
>
> +-- the invalid state of the child constraint will be ignored here.
> +alter table p1 add constraint inh_check_constraint10 check (f1 < 10)
> not enforced;
> +alter table p1_c1 add constraint inh_check_constraint10 check (f1 < 10)
> not valid enforced;
>
> Why is that correct?  At least we should explain it here, or somewhere.
>

A NOT ENFORCED constraint will be considered NOT VALID, so the next
constraint, even if specified with a NOT VALID or VALID clause, will
be ignored. I'll improve the comment a bit.

> I'm also confused about the changes of the constraint names in the
> foreign_key.sql test:
>
> -ERROR:  insert or update on table "fk_partitioned_fk_2" violates
> foreign key constraint "fk_partitioned_fk_a_b_fkey"
> +ERROR:  insert or update on table "fk_partitioned_fk_2" violates
> foreign key constraint "fk_partitioned_fk_2_a_b_fkey"
>
> And then patch 0003 changes it again.  This seems pretty random.  We
> should make sure that tests don't contain unrelated changes like that.
> (Or at least it's not clear why they are related.)
>

I have fixed it in the v19 version, which I just posted a few moments ago.

> There is also in 0002
>
> +-- should be having two constraints
>
> and then in 0003:
>
> --- should be having two constraints
> +-- should only have one constraint
>
> So another reason for squashing these together, so we don't have
> confusing intermediate states.
>

Sure.

> That said, is there a simpler way?  Patch 0003 appears to add a lot of
> complexity.  Could we make this simpler by saying, if you have otherwise
> matching constraints with different enforceability, make this an error.
> Then users can themselves adjust the enforceability how they want to
> make it match.

We can simply discard this patch, as it still reflects the correct
behavior. It creates a new constraint without affecting the existing
constraint with differing enforceability on the child. I noticed
similar behavior with deferrability -- when it differs, the
constraints are not merged, and a new constraint is created on the
child. Let me know your thoughts so I can avoid squashing patch 0006.

Regards,
Amul

  91. Re: NOT ENFORCED constraint feature

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-27T14:15:25Z

    On 2025-Mar-27, Amul Sul wrote:

> On Thu, Mar 27, 2025 at 6:28 PM Peter Eisentraut <peter@eisentraut.org> wrote:

> > That said, is there a simpler way?  Patch 0003 appears to add a lot of
> > complexity.  Could we make this simpler by saying, if you have otherwise
> > matching constraints with different enforceability, make this an error.
> > Then users can themselves adjust the enforceability how they want to
> > make it match.
> 
> We can simply discard this patch, as it still reflects the correct
> behavior. It creates a new constraint without affecting the existing
> constraint with differing enforceability on the child. I noticed
> similar behavior with deferrability -- when it differs, the
> constraints are not merged, and a new constraint is created on the
> child. Let me know your thoughts so I can avoid squashing patch 0006.

I didn't read that patch and I don't know what level of complexity we're
talking about, but the idea of creating a second constraint beside an
existing one itches me.  I'm pretty certain most users would rather not
end up with redundant constraints that only differ in enforceability or
whatever other properties.  I failed to realize that this was happening
when adding FKs on partitioned tables, and I now think it was a mistake.
(As I said in some previous thread, I'd rather have this kind of
situation raise an error so that the user can do something about it,
rather than silently moving ahead with a worse solution like creating a
redundant constraint.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  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

  92. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-28T09:00:39Z

    On Thu, Mar 27, 2025 at 7:45 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-27, Amul Sul wrote:
>
> > On Thu, Mar 27, 2025 at 6:28 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> > > That said, is there a simpler way?  Patch 0003 appears to add a lot of
> > > complexity.  Could we make this simpler by saying, if you have otherwise
> > > matching constraints with different enforceability, make this an error.
> > > Then users can themselves adjust the enforceability how they want to
> > > make it match.
> >
> > We can simply discard this patch, as it still reflects the correct
> > behavior. It creates a new constraint without affecting the existing
> > constraint with differing enforceability on the child. I noticed
> > similar behavior with deferrability -- when it differs, the
> > constraints are not merged, and a new constraint is created on the
> > child. Let me know your thoughts so I can avoid squashing patch 0006.
>
> I didn't read that patch and I don't know what level of complexity we're
> talking about, but the idea of creating a second constraint beside an
> existing one itches me.  I'm pretty certain most users would rather not
> end up with redundant constraints that only differ in enforceability or
> whatever other properties.  I failed to realize that this was happening
> when adding FKs on partitioned tables, and I now think it was a mistake.
> (As I said in some previous thread, I'd rather have this kind of
> situation raise an error so that the user can do something about it,
> rather than silently moving ahead with a worse solution like creating a
> redundant constraint.)
>

Okay, in the attached version, I’ve added an error in
tryAttachPartitionForeignKey() if the enforceability is different.
Please have a look at the 0005 patch and let me know if it looks good.
The rest of the patches remain unchanged.

I haven’t squashed the patches because, if we decide to keep the error
and avoid further complexity, we can simply discard the 0006 patch.

Regards,
Amul

  93. Re: NOT ENFORCED constraint feature

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-03-28T10:03:51Z

    On Thu, Mar 27, 2025 at 6:25 PM Amul Sul <sulamul@gmail.com> wrote:

>
> I am not sure how to make such tests stable enough since the trigger
> name involves OIDs. In count check, I tried adjusting the join
> condition to ensure that I get the exact same type of constraint
> w.r.t. trigger relation and the constraint.

There are tests which mask variable parts of EXPLAIN output. Can we
use similar trick to mask OIDs from the trigger names?

-- 
Best Wishes,
Ashutosh Bapat

  94. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-03-28T13:27:51Z

    On Fri, Mar 28, 2025 at 3:34 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Mar 27, 2025 at 6:25 PM Amul Sul <sulamul@gmail.com> wrote:
>
> >
> > I am not sure how to make such tests stable enough since the trigger
> > name involves OIDs. In count check, I tried adjusting the join
> > condition to ensure that I get the exact same type of constraint
> > w.r.t. trigger relation and the constraint.
>
> There are tests which mask variable parts of EXPLAIN output. Can we
> use similar trick to mask OIDs from the trigger names?
>

Okay, tried it in the attached version. Please check if it looks good.

Regards,
Amul

  95. Re: NOT ENFORCED constraint feature

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-02T12:32:31Z

    On 28.03.25 14:27, Amul Sul wrote:
> On Fri, Mar 28, 2025 at 3:34 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
>>
>> On Thu, Mar 27, 2025 at 6:25 PM Amul Sul <sulamul@gmail.com> wrote:
>>
>>>
>>> I am not sure how to make such tests stable enough since the trigger
>>> name involves OIDs. In count check, I tried adjusting the join
>>> condition to ensure that I get the exact same type of constraint
>>> w.r.t. trigger relation and the constraint.
>>
>> There are tests which mask variable parts of EXPLAIN output. Can we
>> use similar trick to mask OIDs from the trigger names?
> 
> Okay, tried it in the attached version. Please check if it looks good.

I have committed version 21 of the patches (without 0006).

The patch you posted failed the regression test foreign_key because in 
the output of the queries that list the triggers, the conname output did 
not match the expected output.  I committed it so that the test output 
matches the code behavior.  But please double-check that that's what you 
intended.

Also, something we hadn't looked at before, I think, I made 
get_relation_foreign_keys() in src/backend/optimizer/util/plancat.c 
ignore not-enforced foreign keys.  That means, not-enforced foreign keys 
will not be used for cost estimation.  This is, I think, what we want, 
as we discussed earlier.  If we ever want an alternative mode where 
not-enforced constraints are considered for cost-estimation, then we 
could quite easily tweak this.

  96. Re: NOT ENFORCED constraint feature

    amul sul <sulamul@gmail.com> — 2025-04-03T05:07:16Z

    On Wed, Apr 2, 2025 at 6:02 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 28.03.25 14:27, Amul Sul wrote:
> > On Fri, Mar 28, 2025 at 3:34 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> >>
> >> On Thu, Mar 27, 2025 at 6:25 PM Amul Sul <sulamul@gmail.com> wrote:
> >>
> >>>
> >>> I am not sure how to make such tests stable enough since the trigger
> >>> name involves OIDs. In count check, I tried adjusting the join
> >>> condition to ensure that I get the exact same type of constraint
> >>> w.r.t. trigger relation and the constraint.
> >>
> >> There are tests which mask variable parts of EXPLAIN output. Can we
> >> use similar trick to mask OIDs from the trigger names?
> >
> > Okay, tried it in the attached version. Please check if it looks good.
>
> I have committed version 21 of the patches (without 0006).
>
> The patch you posted failed the regression test foreign_key because in
> the output of the queries that list the triggers, the conname output did
> not match the expected output.  I committed it so that the test output
> matches the code behavior.  But please double-check that that's what you
> intended.
>
Interestingly, it's not failing for me, and what's even stranger is
that the version with the committed test works fine on my system as
well. :)

> Also, something we hadn't looked at before, I think, I made
> get_relation_foreign_keys() in src/backend/optimizer/util/plancat.c
> ignore not-enforced foreign keys.  That means, not-enforced foreign keys
> will not be used for cost estimation.  This is, I think, what we want,
> as we discussed earlier.  If we ever want an alternative mode where
> not-enforced constraints are considered for cost-estimation, then we
> could quite easily tweak this.
>

Yeah, it makes sense.

Thanks so much for the review, committing the patch, and all your guidance.

Regards,
Amul