Re: Support for NO INHERIT to INHERIT state change with named NOT NULL constraints - Mailing list pgsql-hackers

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 ] [
INITIALLYDEFERRED | 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)