Feedback-ID: ia2694551:Fastmail
Date: Mon, 13 May 2024 15:44: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: <202405131344.stk24vg66gxm@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+TgmoaOsp+eh64A8OYF=Y49dy6nE1V3pmM9Vd_JDhk5N3yx=w@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202405131344.stk24vg66gxm%40alvherre.pgsql>
Precedence: bulk

On 2024-May-13, Robert Haas wrote:

> On Sat, May 11, 2024 at 5:40 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > Specifically, the problem is that I mentioned that we could restrict the
> > NOT NULL NO INHERIT addition in pg_dump for primary keys to occur only
> > in pg_upgrade; but it turns this is not correct.  In normal
> > dump/restore, there's an additional table scan to check for nulls when
> > the constraints is not there, so the PK creation would become measurably
> > slower.  (In a table with a million single-int rows, PK creation goes
> > from 2000ms to 2300ms due to the second scan to check for nulls).
> 
> I have a feeling that any theory of the form "X only needs to happen
> during pg_upgrade" is likely to be wrong. pg_upgrade isn't really
> doing anything especially unusual: just creating some objects and
> loading data. Those things can also be done at other times, so
> whatever is needed during pg_upgrade is also likely to be needed at
> other times. Maybe that's not sound reasoning for some reason or
> other, but that's my intuition.

True.  It may be that by setting up the upgrade SQL script differently,
we don't need to make the distinction at all.  I hope to be able to do
that.

> I'm sorry that I haven't been following this thread closely, but I'm
> confused about how we ended up here. What exactly are the user-visible
> behavior changes wrought by this patch, and how do they give rise to
> these issues?

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.

However, at some point we realized that we needed to add NOT NULL
constraints in child tables for the columns in which the parent had a
primary key.  Then things become messy because we had the throwaway
constraints on one hand and the not-nulls that descend from the PK on
the other hand, where one was NO INHERIT and the other wasn't; worse if
the child also has a primary key.

It turned out that we didn't have any mechanism to transform a NO
INHERIT constraint into a regular one that would be inherited.  I added
one, didn't like the way it worked, tried to restrict it but that caused
other problems; this is the mess that led to the revert (pg_dump in
normal mode would emit scripts that fail for some legitimate cases).

One possible way forward might be to make pg_dump smarter by adding one
more query to know the relationship between constraints that must be
dropped and those that don't.  Another might be to allow multiple
not-null constraints on the same column (one inherits, the other
doesn't, and you can drop them independently).  There may be others.

> The other possibility that occurs to me is that I think the motivation
> for cataloging NOT NULL constraints was that we wanted to be able to
> track dependencies on them, or something like that, which seems like
> it might be able to create issues of the type that you're facing, but
> the details aren't clear to me.

NOT VALID constraints would be extremely useful, for one thing (because
then you don't need to exclusively-lock the table during a long scan in
order to add a constraint), and it's just one step away from having
these constraints be catalogued.  It was also fixing some inconsistent
handling of inheritance cases.

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