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

On Wed, Nov 27, 2024 at 12:53 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2024-Nov-20, Amit Langote wrote:
> > On Tue, Nov 19, 2024 at 6:41 PM Alvaro Herrera <alvherre@alvh.no-ip.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