Re: doc fail about ALTER TABLE ATTACH re. NO INHERIT

On Tue, Nov 19, 2024 at 6:41 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> On 2024-Nov-14, Amit Langote wrote:
>
> > Sorry, here's the full example. Note I'd changed both
> > AddRelationNotNullConstraints() and AdjustNotNullInheritance() to not
> > throw an error *if* the table is a leaf partition when the NO INHERIT
> > of an existing constraint doesn't match that of the new constraint.
> >
> > create table p (a int not null) partition by list (a);
> > create table p1 partition of p for values in (1);
> > alter table p1 add constraint a_nn_ni not null a no inherit;
>
> Yeah, I think this behavior is bogus, because the user wants to have
> something (a constraint that will not inherit) but they cannot have it,
> because there is already a constraint that will inherit. The current
> behavior of throwing an error seems correct to me. With your patch,
> what this does is mark the constraint as "local" in addition to
> inherited, but that'd be wrong, because the constraint the user wanted
> is not of the same state.

Yeah, just not throwing the error, as my patch did, is not enough.
The patch didn't do anything to ensure that a separate constraint with
the properties that the user entered will exist alongside the
inherited one, but that's not possible given that the design only
allows one NOT NULL constraint for a column as you've written below.

> > On Wed, Nov 13, 2024 at 10:49 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> > > I think:
> > > * if a leaf partition already has an inherited not-null constraint
> > > from its parent and we want to add another one, we should:
> > > - if the one being added is NO INHERIT, then throw an error, because
> > > we cannot merge them
> >
> > Hmm, we'll be doing something else for CHECK constraints if we apply my patch:
> >
> > create table p (a int not null, constraint check_a check (a > 0)) partition by list (a);
> > create table p1 partition of p (constraint check_a check (a > 0) no inherit) for values in (1);
> > NOTICE: merging constraint "check_a" with inherited definition
> >
> > \d p1
> > Table "public.p1"
> > Column | Type | Collation | Nullable | Default
> > --------+---------+-----------+----------+---------
> > a | integer | | not null |
> > Partition of: p FOR VALUES IN (1)
> > Check constraints:
> > "check_a" CHECK (a > 0) NO INHERIT
> >
> > I thought we were fine with allowing merging of such child
> > constraints, because leaf partitions can't have children to pass them
> > onto, so the NO INHERIT clause is essentially pointless.
>
> I'm beginning to have second thoughts about this whole thing TBH, as it
> feels inconsistent -- and unnecessary, if we get the patch to flip the
> INHERIT/NO INHERIT flag of constraints.

Ah, ok, I haven't looked at that patch, but I am happy to leave this alone.

> > > - if the one being added is not NO INHERIT, then both constraints
> > > would have the same state and so we silently do nothing.
> >
> > Maybe we should emit some kind of NOTICE message in such cases?
>
> Hmm, I'm not sure. It's not terribly useful, is it? I mean, if the
> user wants to have a constraint, then whether the constraint is there or
> not, the end result is the same and we needn't say anything about it.
>
> It's only if the constraint is not what they want (because of
> mismatching INHERIT flag) that we throw some message.
>
> (I wonder if we'd throw an error if the proposed constraint has a
> different _name_ from the existing constraint. If a DDL script is
> expecting that the constraint will be named a certain way, then by
> failing to throw an error we might be giving confusing expectations.)

Here's an example that I think matches the above description, which,
ISTM, leads to a state similar to what one would encounter with my
patch as described earlier:

create table parent (a int not null);
create table child (a int, constraint a_not_null_child not null a)
inherits (parent);
NOTICE: merging column "a" with inherited definition

\d+ child
Table "public.child"
Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
a | integer | | not null | | plain |
| |
Not-null constraints:
"a_not_null_child" NOT NULL "a" (local, inherited)
Inherits: parent
Access method: heap

I think the inherited constraint should be named parent_a_not_null,
but the constraint that gets stored is the user-specified constraint
in `create table child`. Actually, even the automatically generated
constraint name using the child table's name won't match the name of
the inherited constraint:

create table child (a int not null) inherits (parent);
NOTICE: merging column "a" with inherited definition
\d+ child
Table "public.child"
Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
a | integer | | not null | | plain |
| |
Not-null constraints:
"child_a_not_null" NOT NULL "a" (local, inherited)
Inherits: parent
Access method: heap

Not sure, but perhaps the following piece of code of
AddRelationNotNullConstraints() should also check that the names
match?

/*
* Search in the list of inherited constraints for any entries on the
* same column; determine an inheritance count from that. Also, if at
* least one parent has a constraint for this column, then we must not
* accept a user specification for a NO INHERIT one. Any constraint
* from parents that we process here is deleted from the list: we no
* longer need to process it in the loop below.
*/
foreach_ptr(CookedConstraint, old, old_notnulls)
{
if (old->attnum == attnum)
{
/*
* If we get a constraint from the parent, having a local NO
* INHERIT one doesn't work.
*/
if (constr->is_no_inherit)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("cannot define not-null constraint
on column \"%s\" with NO INHERIT",
strVal(linitial(constr->keys))),
errdetail("The column has an inherited
not-null constraint.")));

inhcount++;
old_notnulls = foreach_delete_current(old_notnulls, old);
}
}

Unless the inherited NOT NULL constraints are not required to have the
same name.

> > > * if a leaf partition has a locally defined not-null marked NO INHERIT
> > > - if we add another NO INHERIT, silently do nothing
> > > - if we add an INHERIT one, throw an error because cannot merge.
> >
> > So IIUC, there cannot be multiple *named* NOT NULL constraints for the
> > same column?
>
> That's correct. What I mean is that if you have a constraint, and you
> try to add another, then the reaction is to compare the desired
> constraint with the existing one; if the comparison yields okay, then we
> silently do nothing; if the comparison says both things are
> incompatible, we throw an error. In no case would we add a second
> constraint.
>
> > > If you want, you could leave the not-null constraint case alone and I
> > > can have a look later. It wasn't my intention to burden you with that.
> >
> > No worries. I want to ensure that the behaviors for NOT NULL and
> > CHECK constraints are as consistent as possible.
>
> Sounds good.
>
> > Anyway, for now, I've fixed my patch to only consider CHECK
> > constraints -- leaf partitions can have inherited CHECK constraints
> > that are marked NO INHERIT.
>
> I agree that both types of constraints should behave as similarly as
> possible in as many ways as possible. Behavioral differences are
> unlikely to be cherished by users.

To be clear, I suppose we agree on continuing to throw an error when
trying to define a NO INHERIT CHECK constraint on a leaf partition.
That is to match the behavior we currently (as of 14e87ffa5) have for
NOT NULL constraints.

--
Thanks, Amit Langote