Thread: Re: NOT ENFORCED constraint feature

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

On 2025-Feb-18, Amul Sul wrote:

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

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

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

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

On 2025-Feb-27, Amul Sul wrote:

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

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

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

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

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

On 2025-Feb-28, Amul Sul wrote:

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

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

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

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

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

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

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

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

Specifically, this test case change

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

seems very wrong to me.

* doc/src/sgml/advanced.sgml

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

* doc/src/sgml/ddl.sgml

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

* src/backend/catalog/sql_features.txt

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

* src/backend/commands/tablecmds.c

I would omit this detail message:

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

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

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

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

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

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

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

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

Better name the constraint explicitly in the first command.

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

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/

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

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

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

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

In 0002, you had this change:

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

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

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

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

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)

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

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.
0004applies 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
testfrequently drops and recreates same partitioned and non-partitioned table inducing different states. So I have a
feelingthat we are just leaving one state combination behind and not all. It's an existing problem but probably with
enforcedand 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
alsoreports, 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
validatedbecause there's no data or it actually scanned the table and found no rows violating the constraint. Let's
leave/insertone 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
constraintand 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
droppedanother 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
asinherited. 
>

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

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.

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

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

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

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

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

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.

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