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 <alvhe...@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) >
v4-alter_not_null_constraint_to_inherit_no_inherit.patch
Description: Binary data
