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

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.

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;