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

On Wed, Nov 27, 2024 at 12:53 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> 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.

Ok, I see. I didn't think about the pg_dump / restore aspect of this.

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

Yeah, that makes sense.

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

+1

--
Thanks, Amit Langote