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

[jian he]

On Thu, Nov 14, 2024 at 1:02 PM Suraj Kharage
<suraj.khar...@enterprisedb.com> wrote:
>
> Hi,
>
> Upstream commit 14e87ffa5c543b5f30ead7413084c25f7735039f added the support 
> for named NOT NULL constraints which are INHERIT by default.
> We can declare those as NO INHERIT which means those constraints will not be 
> inherited to child tables and after this state, we don't have the 
> functionality to change the state back to INHERIT.
>
> This patch adds this support where named NOT NULL constraint defined as NO 
> INHERIT can be changed to INHERIT.
> For this, introduced the new syntax something like -
>
> ALTER TABLE <tabname> ALTER CONSTRAINT <constrname> INHERIT;
>


            /* ALTER TABLE <name> ALTER CONSTRAINT INHERIT*/
            | ALTER CONSTRAINT name INHERIT
                {
                    AlterTableCmd *n = makeNode(AlterTableCmd);
                    Constraint *c = makeNode(Constraint);

                    n->subtype = AT_AlterConstraint;
                    n->def = (Node *) c;
                    c->contype = CONSTR_NOTNULL;

in gram.y, adding a comment saying this only supports not-null would be great.

comments " * Currently only works for Foreign Key constraints."
above ATExecAlterConstraint need change?



ATExecAlterConstraint
+ if (currcon->contype != CONSTRAINT_NOTNULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("constraint \"%s\" of relation \"%s\" is not a not null constraint",
+ cmdcon->conname, RelationGetRelationName(rel))));
the error message is not helpful?

we should instead saying
ALTER TABLE <tabname> ALTER CONSTRAINT <constrname> INHERIT;
only support not-null constraint.

in ATExecSetNotNull we already have:
if (recurse)
{
        List       *children;
        children = find_inheritance_children(RelationGetRelid(rel),
                                             lockmode);
        foreach_oid(childoid, children)
        {
            Relation    childrel = table_open(childoid, NoLock);
            CommandCounterIncrement();
            ATExecSetNotNull(wqueue, childrel, conName, colName,
                             recurse, true, lockmode);
            table_close(childrel, NoLock);
        }
    }
so we don't need another CommandCounterIncrement()
in the `foreach_oid(childoid, children)` loop?


maybe we need a CommandCounterIncrement() for
+ /* Update the constraint tuple and mark connoinherit as false. */
+ currcon->connoinherit = false;
+
+ CatalogTupleUpdate(conrel, &contuple->t_self, contuple);
+ ObjectAddressSet(address, ConstraintRelationId, currcon->oid);


+-- error out when provided not null constarint does not exists.
+create table part1(f1 int not null no inherit);
+alter table foo alter constraint part1_id_not_nul inherit;
+ERROR:  constraint "part1_id_not_nul" of relation "foo" does not exist
+drop table part1;
i think you mean:
+alter table part1 alter constraint foo inherit;