Re: doc fail about ALTER TABLE ATTACH re. NO INHERIT - Mailing list pgsql-hackers

On Tue, Nov 19, 2024 at 6:41 PM Alvaro Herrera <alvherre@alvh.no-ip.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@alvh.no-ip.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