Re: cataloguing NOT NULL constraints

Hi Alvaro,

On Sat, Sep 10, 2022 at 2:58 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> There were a lot more problems in that submission than I at first
> realized, and I had to rewrite a lot of code in order to fix them. I
> have fixed all the user-visible problems I found in this version, and
> reviewed the tests results more carefully so I am now more confident
> that behaviourally it's doing the right thing; but
>
> 1. the pg_upgrade test problem is still unaddressed,
> 2. I haven't verified that catalog contents is correct, especially
> regarding dependencies,
> 3. there are way too many XXX and FIXME comments sprinkled everywhere.
>
> I'm sure a couple of these XXX comments can be left for later work, and
> there's a few that should be dealt with by merely removing them; but the
> others (and all FIXMEs) represent pending work.
>
> Also, I'm not at all happy about having this new ConstraintNotNull
> artificial node there; perhaps this can be solved by using a regular
> Constraint with some new flag, or maybe it will even work without any
> extra flags by the fact that the node appears where it appears. Anyway,
> requires investigation. Also, the AT_SetAttNotNull continues to irk me.
>
> test_ddl_deparse is also unhappy. This is probably an easy fix;
> apparently, ATExecDropConstraint has been doing things wrong forever.
>
> Anyway, here's version 2 of this, with apologies for those who spent
> time reviewing version 1 with all its brokenness.

I have been testing this with the intention of understanding how you
made this work with inheritance. While doing so with the previous
version, I ran into an existing issue (bug?) that I reported at [1].

I ran into another while testing version 2 that I think has to do with
this patch. So this happens:

-- regular inheritance
create table foo (a int not null);
create table foo1 (a int not null);
alter table foo1 inherit foo;
alter table foo alter a drop not null ;
ERROR: constraint "foo_a_not_null" of relation "foo1" does not exist

-- partitioning
create table parted (a int not null) partition by list (a);
create table part1 (a int not null);
alter table parted attach partition part1 default;
alter table parted alter a drop not null;
ERROR: constraint "parted_a_not_null" of relation "part1" does not exist

In both of these cases, MergeConstraintsIntoExisting(), called by
CreateInheritance() when attaching the child to the parent, marks the
child's NOT NULL check constraint as the child constraint of the
corresponding constraint in parent, which seems fine and necessary.

However, ATExecDropConstraint_internal(), the new function called by
ATExecDropNotNull(), doesn't seem to recognize when recursing to the
child tables that a child's copy NOT NULL check constraint attached to
the parent's may have a different name, so scanning pg_constraint with
the parent's name is what gives the above error. I wonder if it
wouldn't be better for ATExecDropNotNull() to handle its own recursion
rather than delegating it to the DropConstraint()?

The same error does not occur when the NOT NULL constraint is added to
parent after-the-fact and thus recursively to the children:

-- regular inheritance
create table foo (a int);
create table foo1 (a int not null) inherits (foo);
alter table foo alter a set not null;
alter table foo alter a drop not null ;
ALTER TABLE

-- partitioning
create table parted (a int) partition by list (a);
create table part1 partition of parted (a not null) default;
alter table parted alter a set not null;
alter table parted alter a drop not null;
ALTER TABLE

And the reason for that seems a bit accidental, because
MergeWithExistingConstraint(), called by AddRelationNewConstraints()
when recursively adding the NOT NULL check constraint to a child, does
not have the code to find the child's already existing constraint that
matches with it. So, in this case, we get a copy of the parent's
constraint with the same name in the child. There is a line in the
header comments of both MergeWithExistingConstraint() and
MergeConstraintsIntoExisting() asking to keep their code in sync, so
maybe the patch missed adding the new NOT NULL check constraint logic
to the former?

Also, it seems that the inheritance recursion for SET NOT NULL is now
occurring both in the prep phase and exec phase due to the following
new code added to ATExecSetNotNull():

@@ -7485,6 +7653,50 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
InvokeObjectPostAlterHook(RelationRelationId,
RelationGetRelid(rel), attnum);
...
+ /* See if there's one already, and skip this if so. */
+ constr = findNotNullConstraintAttnum(rel, attnum, NULL);
+ if (constr && direct)
+ heap_freetuple(constr); /* nothing to do */
+ else
+ {
+ Constraint *newconstr;
+ ObjectAddress addr;
+ List *children;
+ List *already_done_rels;
+
+ newconstr = makeCheckNotNullConstraint(rel->rd_rel->relnamespace,
+ constrname,
+
NameStr(rel->rd_rel->relname),
+ colName,
+ false, /* XXX is_row */
+ InvalidOid);
+
+ addr = ATAddCheckConstraint_internal(wqueue, tab, rel, newconstr,
+ false, false, false, lockmode);
+ already_done_rels = list_make1_oid(RelationGetRelid(rel));
+
+ /* and recurse into children, if there are any */
+ children =
find_inheritance_children(RelationGetRelid(rel), lockmode);
+ ATAddCheckConstraint_recurse(wqueue, children, newconstr,

It seems harmless because ATExecSetNotNull() set up to run on the
children by the prep phase becomes a no-op due to the work done by the
above code, but maybe we should keep one or the other.

Regarding the following bit:

- /* If rel is partition, shouldn't drop NOT NULL if parent has the same */
+ /*
+ * If rel is partition, shouldn't drop NOT NULL if parent has the same.
+ * XXX is this consideration still valid? Can we get rid of this by
+ * changing the type of dependency between the two constraints instead?
+ */
if (rel->rd_rel->relispartition)
{
Oid parentId =
get_partition_parent(RelationGetRelid(rel), false);

Yes, it seems we can now prevent dropping a partition's NOT NULL
constraint by seeing that it is inherited, so no need for this block
which was written when the NOT NULL constraints didn't have the
inherited marking.

BTW, have you thought about making DROP NOT NULL command emit a
different error message than what one gets now:

create table foo (a int);
create table foo1 (a int) inherits (foo);
alter table foo alter a set not null;
alter table foo1 alter a drop not null ;
ERROR: cannot drop inherited constraint "foo_a_not_null" of relation "foo1"

Like, say:

ERROR: cannot drop an inherited NOT NULL constraint

Maybe you did and thought that it's OK for it to spell out the
internally generated constraint name, because we already require users
to know that they exist, say to drop it using DROP CONSTRAINT.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

[1] https://www.postgresql.org/message-id/CA%2BHiwqFggpjAvsVqNV06HUF6CUrU0Vo3pLgGWCViGbPkzTiofg%40mail.gmail.com