Horton

Thread

Commits

  1. Small fixes for Add ALTER TABLE ... ALTER CONSTRAINT ... SET [NO] INHERIT

  2. Add ALTER TABLE ... ALTER CONSTRAINT ... SET [NO] INHERIT

  3. Add pg_constraint rows for not-null constraints

  1. Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Suraj Kharage <suraj.kharage@enterprisedb.com> — 2024-11-14T05:01:56Z

    Hi,

Upstream commit 14e87ffa5c543b5f30ead7413084c25f7735039f
<https://github.com/postgres/postgres/commit/14e87ffa5c543b5f30ead7413084c25f7735039f>
added the support for named NOT NULL constraints which are INHERIT by
default.
We can declare those as NO INHERIT which means those constraints will not
be inherited to child tables and after this state, we don't have the
functionality to change the state back to INHERIT.

This patch adds this support where named NOT NULL constraint defined as NO
INHERIT can be changed to INHERIT.
For this, introduced the new syntax something like -

ALTER TABLE <tabname> ALTER CONSTRAINT <constrname> INHERIT;


Once the not null constraints are altered to INHERIT from NO INHERIT,
recurse to all children and propagate the constraint if it doesn't exist.


Alvaro stated that allowing a not null constraint state to be modified from
INHERIT to NO INHERIT is going to be quite problematic because of the
number of weird cases to avoid, so for now that support is not added.

Please share your thoughts on the same.

--

Thanks & Regards,
Suraj kharage,



enterprisedb.com <https://www.enterprisedb.com/>

  2. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Robert Haas <robertmhaas@gmail.com> — 2024-11-15T03:15:39Z

    On Thu, Nov 14, 2024 at 12:02 AM Suraj Kharage <
suraj.kharage@enterprisedb.com> wrote:

> Alvaro stated that allowing a not null constraint state to be modified
> from INHERIT to NO INHERIT is going to be quite problematic because of the
> number of weird cases to avoid, so for now that support is not added.
>

What's the reasoning behind that restriction? What are the weird cases?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  3. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    jian he <jian.universality@gmail.com> — 2024-11-19T13:22:43Z

    On Fri, Nov 15, 2024 at 11:15 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Nov 14, 2024 at 12:02 AM Suraj Kharage <suraj.kharage@enterprisedb.com> wrote:
>>
>> Alvaro stated that allowing a not null constraint state to be modified from INHERIT to NO INHERIT is going to be quite problematic because of the number of weird cases to avoid, so for now that support is not added.
>
> What's the reasoning behind that restriction? What are the weird cases?

current status:
drop table if exists idxpart,idxpart0,idxpart1 cascade;
create table idxpart (a int not null) partition by list (a);
create table idxpart0 (a int constraint foo not null no inherit);

alter table idxpart attach partition idxpart0 for values in (0,1);
ERROR:  constraint "foo" conflicts with non-inherited constraint on
child table "idxpart0"

to make it attach to the partition, we need to drop and recreate the
not-null constraint "foo".
that would be very expensive, since recreate, we need to revalidate
the previous row is not null or not.
related post:
https://www.postgresql.org/message-id/202410021219.bvjmxzdspif2%40alvherre.pgsql

with
alter table idxpart0 alter constraint foo inherit;

then we can

alter table idxpart attach partition idxpart0 for values in (0,1);

  4. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    jian he <jian.universality@gmail.com> — 2024-11-20T03:43:41Z

    On Thu, Nov 14, 2024 at 1:02 PM Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:
>
> Hi,
>
> Upstream commit 14e87ffa5c543b5f30ead7413084c25f7735039f added the support for named NOT NULL constraints which are INHERIT by default.
> We can declare those as NO INHERIT which means those constraints will not be inherited to child tables and after this state, we don't have the functionality to change the state back to INHERIT.
>
> This patch adds this support where named NOT NULL constraint defined as NO INHERIT can be changed to INHERIT.
> For this, introduced the new syntax something like -
>
> ALTER TABLE <tabname> ALTER CONSTRAINT <constrname> INHERIT;
>


            /* ALTER TABLE <name> ALTER CONSTRAINT INHERIT*/
            | ALTER CONSTRAINT name INHERIT
                {
                    AlterTableCmd *n = makeNode(AlterTableCmd);
                    Constraint *c = makeNode(Constraint);

                    n->subtype = AT_AlterConstraint;
                    n->def = (Node *) c;
                    c->contype = CONSTR_NOTNULL;

in gram.y, adding a comment saying this only supports not-null would be great.

comments " * Currently only works for Foreign Key constraints."
above ATExecAlterConstraint need change?



ATExecAlterConstraint
+ if (currcon->contype != CONSTRAINT_NOTNULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("constraint \"%s\" of relation \"%s\" is not a not null constraint",
+ cmdcon->conname, RelationGetRelationName(rel))));
the error message is not helpful?

we should instead saying
ALTER TABLE <tabname> ALTER CONSTRAINT <constrname> INHERIT;
only support not-null constraint.

in ATExecSetNotNull we already have:
if (recurse)
{
        List       *children;
        children = find_inheritance_children(RelationGetRelid(rel),
                                             lockmode);
        foreach_oid(childoid, children)
        {
            Relation    childrel = table_open(childoid, NoLock);
            CommandCounterIncrement();
            ATExecSetNotNull(wqueue, childrel, conName, colName,
                             recurse, true, lockmode);
            table_close(childrel, NoLock);
        }
    }
so we don't need another CommandCounterIncrement()
in the `foreach_oid(childoid, children)` loop?


maybe we need a CommandCounterIncrement() for
+ /* Update the constraint tuple and mark connoinherit as false. */
+ currcon->connoinherit = false;
+
+ CatalogTupleUpdate(conrel, &contuple->t_self, contuple);
+ ObjectAddressSet(address, ConstraintRelationId, currcon->oid);


+-- error out when provided not null constarint does not exists.
+create table part1(f1 int not null no inherit);
+alter table foo alter constraint part1_id_not_nul inherit;
+ERROR:  constraint "part1_id_not_nul" of relation "foo" does not exist
+drop table part1;
i think you mean:
+alter table part1 alter constraint foo inherit;

  5. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Suraj Kharage <suraj.kharage@enterprisedb.com> — 2024-11-25T03:12:53Z

    --

Thanks & Regards,
Suraj kharage,



enterprisedb.com <https://www.enterprisedb.com/>


On Tue, Nov 19, 2024 at 6:52 PM jian he <jian.universality@gmail.com> wrote:

> On Fri, Nov 15, 2024 at 11:15 AM Robert Haas <robertmhaas@gmail.com>
> wrote:
> >
> > On Thu, Nov 14, 2024 at 12:02 AM Suraj Kharage <
> suraj.kharage@enterprisedb.com> wrote:
> >>
> >> Alvaro stated that allowing a not null constraint state to be modified
> from INHERIT to NO INHERIT is going to be quite problematic because of the
> number of weird cases to avoid, so for now that support is not added.
> >
> > What's the reasoning behind that restriction? What are the weird cases?
>
> current status:
> drop table if exists idxpart,idxpart0,idxpart1 cascade;
> create table idxpart (a int not null) partition by list (a);
> create table idxpart0 (a int constraint foo not null no inherit);
>
> alter table idxpart attach partition idxpart0 for values in (0,1);
> ERROR:  constraint "foo" conflicts with non-inherited constraint on
> child table "idxpart0"
>
> to make it attach to the partition, we need to drop and recreate the
> not-null constraint "foo".
> that would be very expensive, since recreate, we need to revalidate
> the previous row is not null or not.
> related post:
>
> https://www.postgresql.org/message-id/202410021219.bvjmxzdspif2%40alvherre.pgsql


Right.
Another case which needs conclusion is -
When changing from INHERIT to NO INHERIT, we need to walk all children and
decrement coninhcount for the corresponding constraint. If a constraint in
one child reaches zero, should we drop it?  not sure. If we do, make sure
to reset the corresponding attnotnull bit too. We could decide not to drop
the constraint, in which case you don’t need to reset attnotnull.


>
>
> with
> alter table idxpart0 alter constraint foo inherit;
>
> then we can
>
> alter table idxpart attach partition idxpart0 for values in (0,1);
>

  6. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Suraj Kharage <suraj.kharage@enterprisedb.com> — 2024-11-25T03:20:42Z

    Thanks for the review comments.

On Wed, Nov 20, 2024 at 9:13 AM jian he <jian.universality@gmail.com> wrote:

> On Thu, Nov 14, 2024 at 1:02 PM Suraj Kharage
> <suraj.kharage@enterprisedb.com> wrote:
> >
> > Hi,
> >
> > Upstream commit 14e87ffa5c543b5f30ead7413084c25f7735039f added the
> support for named NOT NULL constraints which are INHERIT by default.
> > We can declare those as NO INHERIT which means those constraints will
> not be inherited to child tables and after this state, we don't have the
> functionality to change the state back to INHERIT.
> >
> > This patch adds this support where named NOT NULL constraint defined as
> NO INHERIT can be changed to INHERIT.
> > For this, introduced the new syntax something like -
> >
> > ALTER TABLE <tabname> ALTER CONSTRAINT <constrname> INHERIT;
> >
>
>
>             /* ALTER TABLE <name> ALTER CONSTRAINT INHERIT*/
>             | ALTER CONSTRAINT name INHERIT
>                 {
>                     AlterTableCmd *n = makeNode(AlterTableCmd);
>                     Constraint *c = makeNode(Constraint);
>
>                     n->subtype = AT_AlterConstraint;
>                     n->def = (Node *) c;
>                     c->contype = CONSTR_NOTNULL;
>
> in gram.y, adding a comment saying this only supports not-null would be
> great.
>
Fixed.


>
> comments " * Currently only works for Foreign Key constraints."
> above ATExecAlterConstraint need change?
>
Modified the comment.


>
> ATExecAlterConstraint
> + if (currcon->contype != CONSTRAINT_NOTNULL)
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("constraint \"%s\" of relation \"%s\" is not a not null
> constraint",
> + cmdcon->conname, RelationGetRelationName(rel))));
> the error message is not helpful?
>
> we should instead saying
> ALTER TABLE <tabname> ALTER CONSTRAINT <constrname> INHERIT;
> only support not-null constraint.
>

Modified as per your suggestion.


>
> in ATExecSetNotNull we already have:
> if (recurse)
> {
>         List       *children;
>         children = find_inheritance_children(RelationGetRelid(rel),
>                                              lockmode);
>         foreach_oid(childoid, children)
>         {
>             Relation    childrel = table_open(childoid, NoLock);
>             CommandCounterIncrement();
>             ATExecSetNotNull(wqueue, childrel, conName, colName,
>                              recurse, true, lockmode);
>             table_close(childrel, NoLock);
>         }
>     }
> so we don't need another CommandCounterIncrement()
> in the `foreach_oid(childoid, children)` loop?
>
I have added that conditional to avoid tuple already updated by self error.
ATExecSetNotNull() is updating tuple and its coninhcount if a constraint
already exists.
Since we have a recursive call to all childrens
from ATExecAlterConstraint(), the recursive call to children doesn't go
through CommandCounterIncrement().


>
>
> maybe we need a CommandCounterIncrement() for
> + /* Update the constraint tuple and mark connoinherit as false. */
> + currcon->connoinherit = false;
> +
> + CatalogTupleUpdate(conrel, &contuple->t_self, contuple);
> + ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
>
Added.


>
>
> +-- error out when provided not null constarint does not exists.
> +create table part1(f1 int not null no inherit);
> +alter table foo alter constraint part1_id_not_nul inherit;
> +ERROR:  constraint "part1_id_not_nul" of relation "foo" does not exist
> +drop table part1;
> i think you mean:
> +alter table part1 alter constraint foo inherit;
>
Fixed.

Please find the attached v2 patch.

  7. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Robert Haas <robertmhaas@gmail.com> — 2024-11-25T20:41:28Z

    On Tue, Nov 19, 2024 at 8:22 AM jian he <jian.universality@gmail.com> wrote:
> current status:
> drop table if exists idxpart,idxpart0,idxpart1 cascade;
> create table idxpart (a int not null) partition by list (a);
> create table idxpart0 (a int constraint foo not null no inherit);
>
> alter table idxpart attach partition idxpart0 for values in (0,1);
> ERROR:  constraint "foo" conflicts with non-inherited constraint on
> child table "idxpart0"
>
> to make it attach to the partition, we need to drop and recreate the
> not-null constraint "foo".
> that would be very expensive, since recreate, we need to revalidate
> the previous row is not null or not.

In a simple implementation of ALTER TABLE this would be true, but I
don't see why it should need to be true in ours. It should be possible
to notice that there's an existing NOT NULL constraint and use that as
evidence that the new one can be added without needing to revalidate
the table contents. ALTER TABLE does similar things already. For
instance, TryReuseIndex() can attempt to attach an existing index file
to a new index definition without rebuilding it; TryReuseForeignKey
can attempt to re-add a foreign key constraint without needing to
revalidate it. But even more to the point, ATAddCheckNNConstraint and
MergeWithExistingConstraint know about merging a newly-added
constraint with a preexisting one without needing to revalidate the
table.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  8. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-08T09:13:22Z

    On 2024-Nov-25, Suraj Kharage wrote:

> Another case which needs conclusion is -
> When changing from INHERIT to NO INHERIT, we need to walk all children and
> decrement coninhcount for the corresponding constraint. If a constraint in
> one child reaches zero, should we drop it?  not sure. If we do, make sure
> to reset the corresponding attnotnull bit too. We could decide not to drop
> the constraint, in which case you don’t need to reset attnotnull.

I think it's more useful if we keep such a constraint (but of course
change its conislocal to true, if it isn't that already).

There are arguments for doing both things (drop it or leave it); but if
you drop it, there's no way to put it back without scanning the table
again.  If you keep it, it's easy to drop it afterwards.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
 PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)

  9. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-08T09:47:48Z

    On 2024-Nov-25, Robert Haas wrote:

> In a simple implementation of ALTER TABLE this would be true, but I
> don't see why it should need to be true in ours. It should be possible
> to notice that there's an existing NOT NULL constraint and use that as
> evidence that the new one can be added without needing to revalidate
> the table contents. ALTER TABLE does similar things already. For
> instance, TryReuseIndex() can attempt to attach an existing index file
> to a new index definition without rebuilding it; TryReuseForeignKey
> can attempt to re-add a foreign key constraint without needing to
> revalidate it. But even more to the point, ATAddCheckNNConstraint and
> MergeWithExistingConstraint know about merging a newly-added
> constraint with a preexisting one without needing to revalidate the
> table.

I think you're explaining why we need this patch, which seems a bit
useless in the thread where this patch was posted.

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

  10. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Suraj Kharage <suraj.kharage@enterprisedb.com> — 2025-01-13T02:56:03Z

    On Wed, Jan 8, 2025 at 2:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> On 2024-Nov-25, Suraj Kharage wrote:
>
> > Another case which needs conclusion is -
> > When changing from INHERIT to NO INHERIT, we need to walk all children
> and
> > decrement coninhcount for the corresponding constraint. If a constraint
> in
> > one child reaches zero, should we drop it?  not sure. If we do, make sure
> > to reset the corresponding attnotnull bit too. We could decide not to
> drop
> > the constraint, in which case you don’t need to reset attnotnull.
>
> I think it's more useful if we keep such a constraint (but of course
> change its conislocal to true, if it isn't that already).
>
> There are arguments for doing both things (drop it or leave it); but if
> you drop it, there's no way to put it back without scanning the table
> again.  If you keep it, it's easy to drop it afterwards.
>

Thanks Alvaro.
Please find attached revised version of patch which added the INHERIT to NO
INHERIT state change for not null constraint.
Keep the constraint (instead of dropping) when we mark NO INHERIT.
As Alvaro mentioned above, we can take others opinion on this behavior.
Also, changed the syntax to ALTER TABLE <tabname> ALTER CONSTRAINT
<constrname> SET INHERIT/NO INHERIT;
just to avoid grammer conflicts but we can decide that as well.

Thanks,
Suraj

  11. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-04T18:41:12Z

    On 2025-Jan-13, Suraj Kharage wrote:

> Please find attached revised version of patch which added the INHERIT to NO
> INHERIT state change for not null constraint.

Thanks!

I find the doc changes a little odd.  First, you seem to have added a
[INHERIT/NO INHERIT] flag in the wrong place (line 112); that stuff
already has the NO INHERIT flag next to the constraint types that allow
it, so that change should be removed from the patch.  I think the
addition in line 62 are sufficient.  Second, adding the explanation for
what this does to the existing varlistentry for ALTER CONSTRAINT looks
out of place.  I would add a separate one, something like this perhaps:

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index f9576da435e..10614bcdbd6 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -59,6 +59,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
     ADD <replaceable class="parameter">table_constraint</replaceable> [ NOT VALID ]
     ADD <replaceable class="parameter">table_constraint_using_index</replaceable>
     ALTER CONSTRAINT <replaceable class="parameter">constraint_name</replaceable> [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
+    ALTER CONSTRAINT <replaceable class="parameter">constraint_name</replaceable> SET [ NO ] INHERIT
     VALIDATE CONSTRAINT <replaceable class="parameter">constraint_name</replaceable>
     DROP CONSTRAINT [ IF EXISTS ]  <replaceable class="parameter">constraint_name</replaceable> [ RESTRICT | CASCADE ]
     DISABLE TRIGGER [ <replaceable class="parameter">trigger_name</replaceable> | ALL | USER ]
@@ -551,7 +552,27 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     <listitem>
      <para>
       This form alters the attributes of a constraint that was previously
-      created. Currently only foreign key constraints may be altered.
+      created. Currently only foreign key constraints may be altered in
+      this fashion, but see below.
+     </para>
+    </listitem>
+   </varlistentry>
+
+   <varlistentry id="sql-altertable-desc-alter-constraint-inherit">
+    <term><literal>ALTER CONSTRAINT ... SET INHERIT</literal></term>
+    <term><literal>ALTER CONSTRAINT ... SET NO INHERIT</literal></term>
+    <listitem>
+     <para>
+      This form modifies a inheritable constraint so that it becomes not
+      inheritable, or vice-versa. Only not-null constraints may be altered
+      in this fashion at present.
+      In addition to changing the inheritability status of the constraint,
+      in the case where a non-inheritable constraint is being marked
+      inheritable, if the table has children, an equivalent constraint
+      is added to them. If marking an inheritable constraint as
+      non-inheritable on a table with children, then the corresponding
+      constraint on children will be marked as no longer inherited,
+      but not removed.
      </para>
     </listitem>
    </varlistentry>


I don't think reusing AT_AlterConstraint for this is a good idea.  I
would rather add a new AT_AlterConstraintInherit /
AT_AlterConstraintNoInherit, which takes only a constraint name in
n->name rather than a Constraint in n->def.  So gram.y would look like

			/*
			 * ALTER TABLE <name> ALTER CONSTRAINT SET [NO] INHERIT
			 */
			| ALTER CONSTRAINT name SET INHERIT
				{
					AlterTableCmd *n = makeNode(AlterTableCmd);

					n->subtype = AT_AlterConstraintInherit;
					n->name = $3;

					$$ = (Node *) n;
				}
			| ALTER CONSTRAINT name SET NO INHERIT
				{
					AlterTableCmd *n = makeNode(AlterTableCmd);

					n->subtype = AT_AlterConstraintNoInherit;
					n->name = $3;

					$$ = (Node *) n;
				}

This avoids hardcoding in the grammar that we only support this for
not-null constraints -- I'm sure we'll want to implement this for CHECK
constraints later, and at the grammar level there just wouldn't be any
way to implement that the way you have it.


It's a pity that bison doesn't like having unadorned NO INHERIT here.
That would align better with the other use of INHERIT / NO INHERIT we
have in alter table -- requiring a SET there looks ugly.  I tried to
change it and the shift/reduce conflict is annoying.  I don't have any
bright ideas on fixing that.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor.  Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

  12. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Suraj Kharage <suraj.kharage@enterprisedb.com> — 2025-02-10T05:13:18Z

    Thanks, Alvaro, for the review.

I have addressed your comments per the above suggestions in the attached v4
patch.

--

Thanks & Regards,
Suraj kharage,



enterprisedb.com <https://www.enterprisedb.com/>


On Wed, Feb 5, 2025 at 12:11 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> On 2025-Jan-13, Suraj Kharage wrote:
>
> > Please find attached revised version of patch which added the INHERIT to
> NO
> > INHERIT state change for not null constraint.
>
> Thanks!
>
> I find the doc changes a little odd.  First, you seem to have added a
> [INHERIT/NO INHERIT] flag in the wrong place (line 112); that stuff
> already has the NO INHERIT flag next to the constraint types that allow
> it, so that change should be removed from the patch.  I think the
> addition in line 62 are sufficient.  Second, adding the explanation for
> what this does to the existing varlistentry for ALTER CONSTRAINT looks
> out of place.  I would add a separate one, something like this perhaps:
>
> diff --git a/doc/src/sgml/ref/alter_table.sgml
> b/doc/src/sgml/ref/alter_table.sgml
> index f9576da435e..10614bcdbd6 100644
> --- a/doc/src/sgml/ref/alter_table.sgml
> +++ b/doc/src/sgml/ref/alter_table.sgml
> @@ -59,6 +59,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable
> class="parameter">name</replaceable>
>      ADD <replaceable class="parameter">table_constraint</replaceable> [
> NOT VALID ]
>      ADD <replaceable
> class="parameter">table_constraint_using_index</replaceable>
>      ALTER CONSTRAINT <replaceable
> class="parameter">constraint_name</replaceable> [ DEFERRABLE | NOT
> DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
> +    ALTER CONSTRAINT <replaceable
> class="parameter">constraint_name</replaceable> SET [ NO ] INHERIT
>      VALIDATE CONSTRAINT <replaceable
> class="parameter">constraint_name</replaceable>
>      DROP CONSTRAINT [ IF EXISTS ]  <replaceable
> class="parameter">constraint_name</replaceable> [ RESTRICT | CASCADE ]
>      DISABLE TRIGGER [ <replaceable
> class="parameter">trigger_name</replaceable> | ALL | USER ]
> @@ -551,7 +552,27 @@ WITH ( MODULUS <replaceable
> class="parameter">numeric_literal</replaceable>, REM
>      <listitem>
>       <para>
>        This form alters the attributes of a constraint that was previously
> -      created. Currently only foreign key constraints may be altered.
> +      created. Currently only foreign key constraints may be altered in
> +      this fashion, but see below.
> +     </para>
> +    </listitem>
> +   </varlistentry>
> +
> +   <varlistentry id="sql-altertable-desc-alter-constraint-inherit">
> +    <term><literal>ALTER CONSTRAINT ... SET INHERIT</literal></term>
> +    <term><literal>ALTER CONSTRAINT ... SET NO INHERIT</literal></term>
> +    <listitem>
> +     <para>
> +      This form modifies a inheritable constraint so that it becomes not
> +      inheritable, or vice-versa. Only not-null constraints may be altered
> +      in this fashion at present.
> +      In addition to changing the inheritability status of the constraint,
> +      in the case where a non-inheritable constraint is being marked
> +      inheritable, if the table has children, an equivalent constraint
> +      is added to them. If marking an inheritable constraint as
> +      non-inheritable on a table with children, then the corresponding
> +      constraint on children will be marked as no longer inherited,
> +      but not removed.
>       </para>
>      </listitem>
>     </varlistentry>
>
>
> I don't think reusing AT_AlterConstraint for this is a good idea.  I
> would rather add a new AT_AlterConstraintInherit /
> AT_AlterConstraintNoInherit, which takes only a constraint name in
> n->name rather than a Constraint in n->def.  So gram.y would look like
>
>                         /*
>                          * ALTER TABLE <name> ALTER CONSTRAINT SET [NO]
> INHERIT
>                          */
>                         | ALTER CONSTRAINT name SET INHERIT
>                                 {
>                                         AlterTableCmd *n =
> makeNode(AlterTableCmd);
>
>                                         n->subtype =
> AT_AlterConstraintInherit;
>                                         n->name = $3;
>
>                                         $$ = (Node *) n;
>                                 }
>                         | ALTER CONSTRAINT name SET NO INHERIT
>                                 {
>                                         AlterTableCmd *n =
> makeNode(AlterTableCmd);
>
>                                         n->subtype =
> AT_AlterConstraintNoInherit;
>                                         n->name = $3;
>
>                                         $$ = (Node *) n;
>                                 }
>
> This avoids hardcoding in the grammar that we only support this for
> not-null constraints -- I'm sure we'll want to implement this for CHECK
> constraints later, and at the grammar level there just wouldn't be any
> way to implement that the way you have it.
>
>
> It's a pity that bison doesn't like having unadorned NO INHERIT here.
> That would align better with the other use of INHERIT / NO INHERIT we
> have in alter table -- requiring a SET there looks ugly.  I tried to
> change it and the shift/reduce conflict is annoying.  I don't have any
> bright ideas on fixing that.
>
> --
> Álvaro Herrera               48°01'N 7°57'E  —
> https://www.EnterpriseDB.com/
> "Nunca confiaré en un traidor.  Ni siquiera si el traidor lo he creado yo"
> (Barón Vladimir Harkonnen)
>

  13. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-19T15:46:55Z

    On 2025-Feb-10, Suraj Kharage wrote:

> Thanks, Alvaro, for the review.
> 
> I have addressed your comments per the above suggestions in the attached v4
> patch.

Okay, thanks.  It looks good to me, but I realized a few days ago that
this patch affects the same code as the patch from Amul Sul to change
enforcedness of constraints[1], and it would be good to structure all
these in a sensible manner to avoid creating a mess of routines that
work against each other.

So I have pushed patch 0001 from Amul, which restructures the way ALTER
TABLE .. ALTER CONSTRAINT works.  This should make it possible to use
the same infrastructure for his NOT ENFORCED constraint change as well
as NO INHERIT.  The way I see this working for your patch is that you'd
remove the new AT_AlterConstraintInherit code I had suggested
previously, and instead add a new flag to the ATAlterConstraint struct
to carry the information of the state change we want; then the state
change would actually be implemented inside ATExecAlterConstraintInternal.
I think you'll also need a new member in ATAlterConstraint to carry the
column name that's being modified.

One detail about that is that the recursion model would have to be
different, I think.  In the existing code for DEFERRED we simply walk
down the hierarchy using the 'conparentid' field to find children.  That
won't work for INHERIT / NO INHERIT -- for this we need to use normal
find_inheritance_children-based recursion.

One thing to keep in mind is what happens with
 ALTER TABLE ONLY parent ALTER CONSTRAINT zzz;
ensuring that it operates without recursing for legacy inheritance, and
throwing an error for partitioned tables.

Thanks!

[1] https://postgr.es/m/CAAJ_b96gEVfK5MVe5YRVwBuobMFr_CKGvz683zFLNeF8gAN5_Q@mail.gmail.com

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Digital and video cameras have this adjustment and film cameras don't for the
same reason dogs and cats lick themselves: because they can."   (Ken Rockwell)

  14. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Suraj Kharage <suraj.kharage@enterprisedb.com> — 2025-02-21T14:10:44Z

    Thanks, Alvaro.

I have revised the patch as per your last update.
Please find attached v5 for further review.

--

Thanks & Regards,
Suraj kharage,



enterprisedb.com <https://www.enterprisedb.com/>


On Wed, Feb 19, 2025 at 9:16 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> On 2025-Feb-10, Suraj Kharage wrote:
>
> > Thanks, Alvaro, for the review.
> >
> > I have addressed your comments per the above suggestions in the attached
> v4
> > patch.
>
> Okay, thanks.  It looks good to me, but I realized a few days ago that
> this patch affects the same code as the patch from Amul Sul to change
> enforcedness of constraints[1], and it would be good to structure all
> these in a sensible manner to avoid creating a mess of routines that
> work against each other.
>
> So I have pushed patch 0001 from Amul, which restructures the way ALTER
> TABLE .. ALTER CONSTRAINT works.  This should make it possible to use
> the same infrastructure for his NOT ENFORCED constraint change as well
> as NO INHERIT.  The way I see this working for your patch is that you'd
> remove the new AT_AlterConstraintInherit code I had suggested
> previously, and instead add a new flag to the ATAlterConstraint struct
> to carry the information of the state change we want; then the state
> change would actually be implemented inside ATExecAlterConstraintInternal.
> I think you'll also need a new member in ATAlterConstraint to carry the
> column name that's being modified.
>
> One detail about that is that the recursion model would have to be
> different, I think.  In the existing code for DEFERRED we simply walk
> down the hierarchy using the 'conparentid' field to find children.  That
> won't work for INHERIT / NO INHERIT -- for this we need to use normal
> find_inheritance_children-based recursion.
>
> One thing to keep in mind is what happens with
>  ALTER TABLE ONLY parent ALTER CONSTRAINT zzz;
> ensuring that it operates without recursing for legacy inheritance, and
> throwing an error for partitioned tables.
>
> Thanks!
>
> [1]
> https://postgr.es/m/CAAJ_b96gEVfK5MVe5YRVwBuobMFr_CKGvz683zFLNeF8gAN5_Q@mail.gmail.com
>
> --
> Álvaro Herrera         PostgreSQL Developer  —
> https://www.EnterpriseDB.com/
> "Digital and video cameras have this adjustment and film cameras don't for
> the
> same reason dogs and cats lick themselves: because they can."   (Ken
> Rockwell)
>

  15. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-28T22:45:10Z

    On 2025-Feb-21, Suraj Kharage wrote:

> Thanks, Alvaro.
> 
> I have revised the patch as per your last update.
> Please find attached v5 for further review.

Hello

I noticed two issues.  One is that we are OK to modify a constraint
that's defined in our parent, which breaks everything.  We can only
allow a top-level constraint to be modified.  I added a check in
ATExecAlterConstraint() for this.  Please add a test case for this.

The other one is that when we set a constraint to NO INHERIT on a table
with children and grandchildren, we must only modify the directly
inheriting constraints as not having a parent -- we must not recurse to
also do that in the grandchildren!  Otherwise they become disconnected,
which makes no sense.  So we just want to locate the constraint for each
child, modify by subtracting 1 from coninhcount and set islocal, and
we're done.  The whole ATExecSetNotNullNoInherit() function is based on
the wrong premise that this requires to recurse.  I chose to remove it
to keep things simple.

Stylistically, in tablecmds.c we always have the 'List **wqueue'
argument as the first one in functions that take it.  So when adding it
to a function that doesn't have it, don't put it last.

This error message:
-                errmsg("ALTER TABLE \"%s\" ALTER CONSTRAINT \"%s\" SET %s only supports not null constraint",
-                       RelationGetRelationName(rel), cmdcon->conname,
-                       cmdcon->noinherit ? "NO INHERIT" : "INHERIT")));
seems a bit excessive.  Looking at other examples, it doesn't look like
we need to cite the complete message in so much detail (especially if,
say, the user specified a schema-qualified table name in the command
which won't show up in the error message, this would look just weird).
I simplified it.

Please verify that the tests are still working correctly and resubmit.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)

  16. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Suraj Kharage <suraj.kharage@enterprisedb.com> — 2025-03-03T09:13:40Z

    Thanks Alvaro for the review and fixup patch.

I agree with your changes and merged that into the main patch along with a
couple of other changes.

Please find attached v6 for further review.

--

Thanks & Regards,
Suraj kharage,



enterprisedb.com <https://www.enterprisedb.com/>


On Sat, Mar 1, 2025 at 4:15 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> On 2025-Feb-21, Suraj Kharage wrote:
>
> > Thanks, Alvaro.
> >
> > I have revised the patch as per your last update.
> > Please find attached v5 for further review.
>
> Hello
>
> I noticed two issues.  One is that we are OK to modify a constraint
> that's defined in our parent, which breaks everything.  We can only
> allow a top-level constraint to be modified.  I added a check in
> ATExecAlterConstraint() for this.  Please add a test case for this.
>
> The other one is that when we set a constraint to NO INHERIT on a table
> with children and grandchildren, we must only modify the directly
> inheriting constraints as not having a parent -- we must not recurse to
> also do that in the grandchildren!  Otherwise they become disconnected,
> which makes no sense.  So we just want to locate the constraint for each
> child, modify by subtracting 1 from coninhcount and set islocal, and
> we're done.  The whole ATExecSetNotNullNoInherit() function is based on
> the wrong premise that this requires to recurse.  I chose to remove it
> to keep things simple.
>
> Stylistically, in tablecmds.c we always have the 'List **wqueue'
> argument as the first one in functions that take it.  So when adding it
> to a function that doesn't have it, don't put it last.
>
> This error message:
> -                errmsg("ALTER TABLE \"%s\" ALTER CONSTRAINT \"%s\" SET %s
> only supports not null constraint",
> -                       RelationGetRelationName(rel), cmdcon->conname,
> -                       cmdcon->noinherit ? "NO INHERIT" : "INHERIT")));
> seems a bit excessive.  Looking at other examples, it doesn't look like
> we need to cite the complete message in so much detail (especially if,
> say, the user specified a schema-qualified table name in the command
> which won't show up in the error message, this would look just weird).
> I simplified it.
>
> Please verify that the tests are still working correctly and resubmit.
>
> --
> Álvaro Herrera         PostgreSQL Developer  —
> https://www.EnterpriseDB.com/
> "E pur si muove" (Galileo Galilei)
>

  17. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

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

    On 2025-Mar-03, Suraj Kharage wrote:

> Thanks Alvaro for the review and fixup patch.
> 
> I agree with your changes and merged that into the main patch along with a
> couple of other changes.
> 
> Please find attached v6 for further review.

Thanks, I have pushed this.  I made some changes to the tests, first by
renaming the tables to avoid too generic names, and second to try and
exercise everything about once.

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

  18. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-25T10:22:45Z

    On 05.03.25 13:56, Alvaro Herrera wrote:
> On 2025-Mar-03, Suraj Kharage wrote:
> 
>> Thanks Alvaro for the review and fixup patch.
>>
>> I agree with your changes and merged that into the main patch along with a
>> couple of other changes.
>>
>> Please find attached v6 for further review.
> 
> Thanks, I have pushed this.  I made some changes to the tests, first by
> renaming the tables to avoid too generic names, and second to try and
> exercise everything about once.

A patch in the NOT ENFORCED constraints patch series proposes to 
refactor some of the code added by this patch series ([0] patch 
v18-0001).  I noticed that the code paths from this patch series do not 
call InvokeObjectPostAlterHook() or CacheInvalidateRelcache() when a 
constraint is altered.  Was this intentional?  If not, I can fix it as 
part of that other patch, just wanted to check here.


[0]: 
https://www.postgresql.org/message-id/CAAJ_b97aHsJgWhAuRQi1JdWsjzd_ygWEjqQVq_Ddo8dyCnnwkw@mail.gmail.com

  19. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-25T11:52:08Z

    Hello

On 2025-Mar-25, Peter Eisentraut wrote:

> A patch in the NOT ENFORCED constraints patch series proposes to refactor
> some of the code added by this patch series ([0] patch v18-0001).  I noticed
> that the code paths from this patch series do not call
> InvokeObjectPostAlterHook() or CacheInvalidateRelcache() when a constraint
> is altered.  Was this intentional?  If not, I can fix it as part of that
> other patch, just wanted to check here.

It was not, I'm good with you fixing it, with thanks.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The ability of users to misuse tools is, of course, legendary" (David Steele)
https://postgr.es/m/11b38a96-6ded-4668-b772-40f992132797@pgmasters.net

  20. Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-25T13:39:17Z

    On 25.03.25 12:52, Alvaro Herrera wrote:
> Hello
> 
> On 2025-Mar-25, Peter Eisentraut wrote:
> 
>> A patch in the NOT ENFORCED constraints patch series proposes to refactor
>> some of the code added by this patch series ([0] patch v18-0001).  I noticed
>> that the code paths from this patch series do not call
>> InvokeObjectPostAlterHook() or CacheInvalidateRelcache() when a constraint
>> is altered.  Was this intentional?  If not, I can fix it as part of that
>> other patch, just wanted to check here.
> 
> It was not, I'm good with you fixing it, with thanks.

done