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

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(at)alvh(dot)no-ip(dot)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)
>