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

On 2024-Nov-20, Amit Langote wrote:

> On Tue, Nov 19, 2024 at 6:41 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:

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

Yeah, the user-specified name taking precedence over using a name from
inheritance was an explicit decision. I think this is working as
intended. An important point is that pg_dump should preserve any
constraint name; that's for example why we disallow ALTER TABLE DROP
CONSTRAINT of an inherited constraint: previously I made that command
reset the 'islocal' flag of the constraint and otherwise do nothing; but
the problem is that if we allow that, then the constraint gets the wrong
name after dump/restore.

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

I was of two minds about checking that the constraint names match, but
in the end I decided it wasn't useful and limiting, because you cannot
have a particular name in the children if you want to.

One thing that distinguishes not-null constraints from check ones is
that when walking down inheritance trees we match by the column that the
apply to, rather than by name as check constraints do. So the fact that
the names don't match doesn't harm. That would not fly for check
constraints.

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

Yep.

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

Yeah. I think we should only worry to the extent that these things
trouble users over what they can and cannot do with tables. Adding a NO
INHERIT constraint to a partition, just so that they can continue to
have the constraint after they detach and that the constraint doesn't
affect any tables that are added as children ... doesn't seem a very
important use case. _But_, for anybody out there that does care about
such a thing, we might have an ALTER TABLE command to change a
constraint from NO INHERIT to INHERIT, and perhaps vice-versa.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"No es bueno caminar con un hombre muerto"