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

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)