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

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)