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 <alvhe...@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) >
v6-0001-alter-not-null-constraint-from-inherit-to-no-inhe.patch
Description: Binary data
