Feedback-ID: ia2694551:Fastmail
Date: Mon, 13 May 2024 18:45:40 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, exclusion@gmail.com,
	dean.a.rasheed@gmail.com, andrewbille@gmail.com, peter@eisentraut.org,
	pgsql-hackers@lists.postgresql.org, Justin Pryzby <pryzby@telsasoft.com>
Subject: Re: cataloguing NOT NULL constraints
Message-ID: <202405131645.7pvv4b22npim@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+TgmoZg9_h=CQUJfKuf3XCiTRY+p4abJCeW4vMgTtbo-UtkcA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202405131645.7pvv4b22npim%40alvherre.pgsql>
Precedence: bulk

On 2024-May-13, Robert Haas wrote:

> On Mon, May 13, 2024 at 9:44 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > The problematic point is the need to add NOT NULL constraints during
> > table creation that don't exist in the table being dumped, for
> > performance of primary key creation -- I called this a throwaway
> > constraint.  We needed to be able to drop those constraints after the PK
> > was created.  These were marked NO INHERIT to allow them to be dropped,
> > which is easier if the children don't have them.  This all worked fine.
> 
> This seems really weird to me. Why is it necessary? I mean, in
> existing releases, if you declare a column as PRIMARY KEY, the columns
> included in the key are forced to be NOT NULL, and you can't change
> that for so long as they are included in the PRIMARY KEY.

The point is that a column can be in a primary key and not have an
explicit not-null constraint.  This is different from having a column be
NOT NULL and having a primary key on top.  In both cases the attnotnull
flag is set; the difference between these two scenarios is what happens
if you drop the primary key.  If you do not have an explicit not-null
constraint, then the attnotnull flag is lost as soon as you drop the
primary key.  You don't have to do DROP NOT NULL for that to happen.

This means that if you have a column that's in the primary key but does
not have an explicit not-null constraint, then we shouldn't make one up.
(Which we would, if we were to keep an unadorned NOT NULL that we can't
drop at the end of the dump.)

> So I would have thought that after this patch, you'd end up with the
> same thing.

At least as I interpret the standard, you wouldn't.

> One way of doing that would be to make the PRIMARY KEY depend on the
> now-catalogued NOT NULL constraints, and the other way would be to
> keep it as an ad-hoc prohibition, same as now.

That would be against what [I think] the standard says.

> But I don't see why I need to end up with what the patch generates,
> which seems to be something like CONSTRAINT pgdump_throwaway_notnull_0
> NOT NULL NO INHERIT. That kind of thing suggests that we're changing
> around the order of operations in pg_dump, probably by adding the NOT
> NULL constraints at a later stage than currently, and I think the
> proper solution is most likely to be to avoid doing that in the first
> place.

The point of the throwaway constraints is that they don't remain after
the dump has restored completely.  They are there only so that we don't
have to scan the data looking for possible nulls when we create the
primary key.  We have a DROP CONSTRAINT for the throwaway not-nulls as
soon as the PK is created.

We're not changing any order of operations as such.

> That's not to say that we shouldn't try to make improvements, just
> that it may be hard to get right.

Sure, that's why this patch has now been reverted twice :-) and has been
in the works for ... how many years now?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/