Re: cataloguing NOT NULL constraints

On 2024-May-07, Kyotaro Horiguchi wrote:

> Hello,
>
> At Wed, 1 May 2024 19:49:35 +0200, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote in
> > Here are two patches that I intend to push soon (hopefully tomorrow).
>
> This commit added and edited two error messages, resulting in using
> slightly different wordings "in" and "on" for relation constraints.
>
> + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"",
> ===
> + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",

Thank you, I hadn't noticed the inconsistency -- I fix this in the
attached series.

While trying to convince myself that I could mark the remaining open
item for this work closed, I discovered that pg_dump fails to produce
working output for some combinations. Notably, if I create Andrew
Bille's example in 16:

create table test_0 (id serial primary key);
create table test_1 (id integer primary key) inherits (test_0);

then current master's pg_dump produces output that the current server
fails to restore, failing the PK creation in test_0:

ALTER TABLE ONLY public.test_0
ADD CONSTRAINT test_0_pkey PRIMARY KEY (id);
ERROR: cannot change NO INHERIT status of NOT NULL constraint "pgdump_throwaway_notnull_0" in relation "test_1"

because we have already created the NOT NULL NO INHERIT constraint in
test_1 when we created it, and because of d45597f72fe5, we refuse to
change it into a regular inheritable constraint, which the PK in its
parent table needs.

I spent a long time trying to think how to fix this, and I had despaired
wanting to write that I would need to revert the whole NOT NULL business
for pg17 -- but that was until I realized that we don't actually need
this NOT NULL NO INHERIT business except during pg_upgrade, and that
simplifies things enough to give me confidence that the whole feature
can be kept.

Because, remember: the idea of those NO INHERIT "throwaway" constraints
is that we can skip reading the data when we create the PRIMARY KEY
during binary upgrade. We don't actually need the NO INHERIT
constraints for anything during regular pg_dump. So what we can do, is
restrict the usage of NOT NULL NO INHERIT so that they occur only during
pg_upgrade. I think this will make Justin P. happier, because we no
longer have these unsightly NOT NULL NO INHERIT nonstandard syntax in
dumps.

The attached patch series does that. Actually, it does a little more,
but it's not really much:

0001: fix the typos pointed out by Kyotaro.

0002: A mechanical code movement that takes some ugly ballast out of
getTableAttrs into its own routine. I realized that this new code was
far too ugly and messy to be in the middle of filling the tbinfo struct
of attributes. If you use "git show --color-moved
--color-moved-ws=ignore-all-space" with this commit you can see that
nothing happens apart from the code move.

0003: pgindent, fixes the comments just moved to account for different
indentation depth.

0004: moves again the moved PQfnumber() calls back to getTableAttrs(),
for efficiency (we don't want to search the result for those resnums for
every single attribute of all tables being dumped).

0005: This is the actual code change I describe above. We restrict
use_throwaway_nulls so that it's only set during binary upgrade mode.
This changes pg_dump output; in the normal case, we no longer have NOT
NULL NO INHERIT. I added one test stanza to verify that pg_upgrade
retains these clauses, where they are critical.

0006: Tighten up what d45597f72fe5 did, in that outside of binary
upgrade mode, we no longer accept changes to NOT NULL NO INHERIT
constraints so that they become INHERIT. Previously we accepted that
during recursion, but this isn't really very principled. (I had
accepted this because pg_dump required it for some other cases). This
changes some test output, and I also simplify some test cases that were
testing stuff that's no longer interesting.

(To push, I'll squash 0002+0003+0004 as a single one, and perhaps 0005
with them; I produced them like this only to make them easy to see
what's changing.)

I also have a pending patch for 16 that adds tables like the problematic
ones so that they remain for future pg_upgrade testing. With the
changes in this series, the whole thing finally works AFAICT.

I did notice one more small bit of weirdness, which is that at the end
of the process you may end up with constraints that retain the throwaway
name. This doesn't seem at all critical, considering that you can't
drop them anyway and such names do not survive a further dump (because
they are marked as inherited constraint without a "local" definition, so
they're not dumped separately). I would still like to fix it, but it
seems to require unduly contortions so I may end up not doing anything
about it.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/