Feedback-ID: ia2694551:Fastmail
Date: Tue, 19 Nov 2024 10:41:38 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: doc fail about ALTER TABLE ATTACH re. NO INHERIT
Message-ID: <202411190941.bqiakaqi7tdz@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqHBeN9f0qhe46=evtZxSiJ7i-AFjwexegGwYLYBDpdaew@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202411190941.bqiakaqi7tdz%40alvherre.pgsql>
Precedence: bulk

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.

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

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

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

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/