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

On 2025-Feb-10, Suraj Kharage wrote:

> Thanks, Alvaro, for the review.
> 
> I have addressed your comments per the above suggestions in the attached v4
> patch.

Okay, thanks.  It looks good to me, but I realized a few days ago that
this patch affects the same code as the patch from Amul Sul to change
enforcedness of constraints[1], and it would be good to structure all
these in a sensible manner to avoid creating a mess of routines that
work against each other.

So I have pushed patch 0001 from Amul, which restructures the way ALTER
TABLE .. ALTER CONSTRAINT works.  This should make it possible to use
the same infrastructure for his NOT ENFORCED constraint change as well
as NO INHERIT.  The way I see this working for your patch is that you'd
remove the new AT_AlterConstraintInherit code I had suggested
previously, and instead add a new flag to the ATAlterConstraint struct
to carry the information of the state change we want; then the state
change would actually be implemented inside ATExecAlterConstraintInternal.
I think you'll also need a new member in ATAlterConstraint to carry the
column name that's being modified.

One detail about that is that the recursion model would have to be
different, I think.  In the existing code for DEFERRED we simply walk
down the hierarchy using the 'conparentid' field to find children.  That
won't work for INHERIT / NO INHERIT -- for this we need to use normal
find_inheritance_children-based recursion.

One thing to keep in mind is what happens with
 ALTER TABLE ONLY parent ALTER CONSTRAINT zzz;
ensuring that it operates without recursing for legacy inheritance, and
throwing an error for partitioned tables.

Thanks!

[1] https://postgr.es/m/CAAJ_b96gEVfK5MVe5YRVwBuobMFr_CKGvz683zFLNeF8gAN5_Q@mail.gmail.com

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Digital and video cameras have this adjustment and film cameras don't for the
same reason dogs and cats lick themselves: because they can."   (Ken Rockwell)

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)

On 2025-Mar-03, Suraj Kharage wrote:

> 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, I have pushed this.  I made some changes to the tests, first by
renaming the tables to avoid too generic names, and second to try and
exercise everything about once.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 05.03.25 13:56, Alvaro Herrera wrote:
> On 2025-Mar-03, Suraj Kharage wrote:
> 
>> 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, I have pushed this.  I made some changes to the tests, first by
> renaming the tables to avoid too generic names, and second to try and
> exercise everything about once.

A patch in the NOT ENFORCED constraints patch series proposes to 
refactor some of the code added by this patch series ([0] patch 
v18-0001).  I noticed that the code paths from this patch series do not 
call InvokeObjectPostAlterHook() or CacheInvalidateRelcache() when a 
constraint is altered.  Was this intentional?  If not, I can fix it as 
part of that other patch, just wanted to check here.


[0]: 
https://www.postgresql.org/message-id/CAAJ_b97aHsJgWhAuRQi1JdWsjzd_ygWEjqQVq_Ddo8dyCnnwkw@mail.gmail.com

Hello

On 2025-Mar-25, Peter Eisentraut wrote:

> A patch in the NOT ENFORCED constraints patch series proposes to refactor
> some of the code added by this patch series ([0] patch v18-0001).  I noticed
> that the code paths from this patch series do not call
> InvokeObjectPostAlterHook() or CacheInvalidateRelcache() when a constraint
> is altered.  Was this intentional?  If not, I can fix it as part of that
> other patch, just wanted to check here.

It was not, I'm good with you fixing it, with thanks.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The ability of users to misuse tools is, of course, legendary" (David Steele)
https://postgr.es/m/11b38a96-6ded-4668-b772-40f992132797@pgmasters.net

On 25.03.25 12:52, Alvaro Herrera wrote:
> Hello
> 
> On 2025-Mar-25, Peter Eisentraut wrote:
> 
>> A patch in the NOT ENFORCED constraints patch series proposes to refactor
>> some of the code added by this patch series ([0] patch v18-0001).  I noticed
>> that the code paths from this patch series do not call
>> InvokeObjectPostAlterHook() or CacheInvalidateRelcache() when a constraint
>> is altered.  Was this intentional?  If not, I can fix it as part of that
>> other patch, just wanted to check here.
> 
> It was not, I'm good with you fixing it, with thanks.

done