Feedback-ID: ia2694551:Fastmail
Date: Wed, 13 Nov 2024 14:49:34 +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: <202411131349.fg47nc7ssdog@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqGk2Y54FAE7fTKCmJNusT49RgFUOMu2yPzEWqRMG99=6g@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202411131349.fg47nc7ssdog%40alvherre.pgsql>
Precedence: bulk

On 2024-Nov-13, Amit Langote wrote:

> I rebased my patch over 14e87ffa5c54 and noticed that NOT NULL
> constraints can also (or not) be marked NO INHERIT.  I think we should
> allow NO INHERIT NOT NULL constraints on leaf partitions just like
> CHECK constraints, so changed AddRelationNotNullConstraints() that way
> and added some tests.

OK, looks good.

> However, I noticed that there is no clean way to do that for the following case:
> 
> ALTER TABLE leaf_partition ADD CONSTRAINT col_nn_ni NOT NULL col NO INHERIT;

Sorry, I didn't understand what's the initial state.  Does the
constraint already exist here or not?

> If I change the new function AdjustNotNullInheritance() added by your
> commit to not throw an error for leaf partitions, the above constraint
> (col_nn_ni) is not stored at all, because the function returns true in
> that case, which means the caller does nothing with the constraint.  I
> am not sure what the right thing to do would be.  If we return false,
> the caller will store the constraint the first time around, but
> repeating the command will again do nothing, not even prevent the
> addition of a duplicate constraint.

Do you mean if we return false, it allows two not-null constraints for
the same column?  That would absolutely be a bug.

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
  - if the one being added is not NO INHERIT, then both constraints
    would have the same state and so we silently do nothing.
* 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.


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.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.