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

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)
>