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

Thanks for the review comments.

On Wed, Nov 20, 2024 at 9:13 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> On Thu, Nov 14, 2024 at 1:02 PM Suraj Kharage
> <suraj(dot)kharage(at)enterprisedb(dot)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.
>
Fixed.

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

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

Modified as per your suggestion.

>
> 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?
>
I have added that conditional to avoid tuple already updated by self error.
ATExecSetNotNull() is updating tuple and its coninhcount if a constraint
already exists.
Since we have a recursive call to all childrens
from ATExecAlterConstraint(), the recursive call to children doesn't go
through CommandCounterIncrement().

>
>
> 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);
>
Added.

>
>
> +-- 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;
>
Fixed.

Please find the attached v2 patch.