Thread

Commits

Skip invalid database pg_upgrade test on obsolete servers
- 5a0ac6855a21 16.3 landed
- 56455ebd354e 17.0 landed
Fix restore of not-null constraints with inheritance
- d9f686a72ee9 17.0 cited

pgsql: Fix restore of not-null constraints with inheritance

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-18T13:37:43Z

Fix restore of not-null constraints with inheritance

In tables with primary keys, pg_dump creates tables with primary keys by
initially dumping them with throw-away not-null constraints (marked "no
inherit" so that they don't create problems elsewhere), to later drop
them once the primary key is restored.  Because of a unrelated
consideration, on tables with children we add not-null constraints to
all columns of the primary key when it is created.

If both a table and its child have primary keys, and pg_dump happens to
emit the child table first (and its throw-away not-null) and later its
parent table, the creation of the parent's PK will fail because the
throw-away not-null constraint collides with the permanent not-null
constraint that the PK wants to add, so the dump fails to restore.

We can work around this problem by letting the primary key "take over"
the child's not-null.  This requires no changes to pg_dump, just two
changes to ALTER TABLE: first, the ability to convert a no-inherit
not-null constraint into a regular inheritable one (including recursing
down to children, if there are any); second, the ability to "drop" a
constraint that is defined both directly in the table and inherited from
a parent (which simply means to mark it as no longer having a local
definition).

Secondarily, change ATPrepAddPrimaryKey() to acquire locks all the way
down the inheritance hierarchy, in case we need to recurse when
propagating constraints.

These two changes allow pg_dump to reproduce more cases involving
inheritance from versions 16 and older.

Lastly, make two changes to pg_dump: 1) do not try to drop a not-null
constraint that's marked as inherited; this allows a dump to restore
with no errors if a table with a PK inherits from another which also has
a PK; 2) avoid giving inherited constraints throwaway names, for the
rare cases where such a constraint survives after the restore.

Reported-by: Andrew Bille <andrewbille@gmail.com>
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/CAJnzarwkfRu76_yi3dqVF_WL-MpvT54zMwAxFwJceXdHB76bOA@mail.gmail.com
Discussion: https://postgr.es/m/Zh0aAH7tbZb-9HbC@pryzbyj2023

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/d9f686a72ee91f6773e5d2bc52994db8d7157a8e

Modified Files
--------------
src/backend/catalog/heap.c                | 36 +++++++++++++++--
src/backend/catalog/pg_constraint.c       | 43 +++++++++++++-------
src/backend/commands/tablecmds.c          | 65 +++++++++++++++++++++++++++----
src/bin/pg_dump/pg_dump.c                 | 26 +++++++++++--
src/include/catalog/pg_constraint.h       |  2 +-
src/test/regress/expected/constraints.out | 56 ++++++++++++++++++++++++++
src/test/regress/sql/constraints.sql      | 22 +++++++++++
7 files changed, 221 insertions(+), 29 deletions(-)

Re: pgsql: Fix restore of not-null constraints with inheritance

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-18T14:08:58Z

On 2024-Apr-18, Alvaro Herrera wrote:

> Lastly, make two changes to pg_dump: 1) do not try to drop a not-null
> constraint that's marked as inherited; this allows a dump to restore
> with no errors if a table with a PK inherits from another which also has
> a PK; 2) avoid giving inherited constraints throwaway names, for the
> rare cases where such a constraint survives after the restore.

Hmm, this last bit broke pg_upgrade on crake.  I'll revert this part,
meanwhile I'll be installing 9.2 to see if it can be fixed in a better way.

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

Re: pgsql: Fix restore of not-null constraints with inheritance

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-18T15:39:31Z

On 2024-Apr-18, Alvaro Herrera wrote:

> On 2024-Apr-18, Alvaro Herrera wrote:
> 
> > Lastly, make two changes to pg_dump: 1) do not try to drop a not-null
> > constraint that's marked as inherited; this allows a dump to restore
> > with no errors if a table with a PK inherits from another which also has
> > a PK; 2) avoid giving inherited constraints throwaway names, for the
> > rare cases where such a constraint survives after the restore.
> 
> Hmm, this last bit broke pg_upgrade on crake.  I'll revert this part,
> meanwhile I'll be installing 9.2 to see if it can be fixed in a better way.

Eh, so:

1. running "make check" on pg_upgrade using an oldinstall pointing to
9.2 fails, because PostgreSQL::Test::Cluster doesn't support that
version -- it only goes back to 9.2.  How difficult was it to port it
back to all these old versions?

2. running "make check" with an oldinstall pointing to 10 fails, because
the "invalid database" checks fail:

not ok 7 - invalid database causes failure status (got 0 vs expected 1)

#   Failed test 'invalid database causes failure status (got 0 vs expected 1)'
#   at t/002_pg_upgrade.pl line 407.
not ok 8 - invalid database causes failure stdout /(?^:invalid)/

3. Lastly, even if I put back the code that causes the failures on crake
and restore from 10 (and ignore those two problems), I cannot reproduce
the issues it reported.  Is crake running some funky code that's not
what "make check" on pg_upgrade does, perchance?

I think we should SKIP the tests with invalid databases when running
with an oldinstall 10 and older, because that commit only patches back
to 11:

Author: Andres Freund <andres@anarazel.de>
Branch: master [c66a7d75e] 2023-07-13 13:03:28 -0700
Branch: REL_16_STABLE Release: REL_16_0 [a4b4cc1d6] 2023-07-13 13:03:30 -0700
Branch: REL_15_STABLE Release: REL_15_4 [f66403749] 2023-07-13 13:04:45 -0700
Branch: REL_14_STABLE Release: REL_14_9 [d11efe830] 2023-07-13 13:03:33 -0700
Branch: REL_13_STABLE Release: REL_13_12 [81ce00006] 2023-07-13 13:03:34 -0700
Branch: REL_12_STABLE Release: REL_12_16 [034a9fcd2] 2023-07-13 13:03:36 -0700
Branch: REL_11_STABLE Release: REL_11_21 [1c38e7ae1] 2023-07-13 13:03:37 -0700

    Handle DROP DATABASE getting interrupted

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat)

Re: pgsql: Fix restore of not-null constraints with inheritance

Andrew Dunstan <andrew@dunslane.net> — 2024-04-18T19:18:32Z

On 2024-04-18 Th 11:39, Alvaro Herrera wrote:
> On 2024-Apr-18, Alvaro Herrera wrote:
>
>> On 2024-Apr-18, Alvaro Herrera wrote:
>>
>>> Lastly, make two changes to pg_dump: 1) do not try to drop a not-null
>>> constraint that's marked as inherited; this allows a dump to restore
>>> with no errors if a table with a PK inherits from another which also has
>>> a PK; 2) avoid giving inherited constraints throwaway names, for the
>>> rare cases where such a constraint survives after the restore.
>> Hmm, this last bit broke pg_upgrade on crake.  I'll revert this part,
>> meanwhile I'll be installing 9.2 to see if it can be fixed in a better way.
> Eh, so:
>
> 1. running "make check" on pg_upgrade using an oldinstall pointing to
> 9.2 fails, because PostgreSQL::Test::Cluster doesn't support that
> version -- it only goes back to 9.2.  How difficult was it to port it
> back to all these old versions?

It's not that hard to make it go back to 9.2. Here's a version that's a 
couple of years old, but it supports versions all the way back to 7.2 :-)

If there's interest I'll work on supporting our official "old" versions 
(i.e. 9.2 and up)

>
> 2. running "make check" with an oldinstall pointing to 10 fails, because
> the "invalid database" checks fail:
>
> not ok 7 - invalid database causes failure status (got 0 vs expected 1)
>
> #   Failed test 'invalid database causes failure status (got 0 vs expected 1)'
> #   at t/002_pg_upgrade.pl line 407.
> not ok 8 - invalid database causes failure stdout /(?^:invalid)/

>
>
> 3. Lastly, even if I put back the code that causes the failures on crake
> and restore from 10 (and ignore those two problems), I cannot reproduce
> the issues it reported.  Is crake running some funky code that's not
> what "make check" on pg_upgrade does, perchance?

It's running the buildfarm cross version upgrade module. See 
<https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/TestUpgradeXversion.pm>

It's choking on the change in constraint names between the dump of the 
pre-upgrade database and the dump of the post-upgrade database, e.g.

  CREATE TABLE public.rule_and_refint_t2 (
-    id2a integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
-    id2c integer CONSTRAINT pgdump_throwaway_notnull_1 NOT NULL NO INHERIT
+    id2a integer CONSTRAINT rule_and_refint_t2_id2a_not_null NOT NULL NO INHERIT,
+    id2c integer CONSTRAINT rule_and_refint_t2_id2c_not_null NOT NULL NO INHERIT
  );

look at the dumpdiff-REL9_2_STABLE file for the full list.

I assume that means pg_dump is generating names that pg_upgrade throws 
away? That seems ... unfortunate.

There is a perl module at 
src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm. This is used to adjust 
the dump files before we diff them. Maybe you can remedy the problem by 
adding some code in there.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: pgsql: Fix restore of not-null constraints with inheritance

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-19T11:59:31Z

On 2024-Apr-18, Andrew Dunstan wrote:

> On 2024-04-18 Th 11:39, Alvaro Herrera wrote:

> It's not that hard to make it go back to 9.2. Here's a version that's a
> couple of years old, but it supports versions all the way back to 7.2 :-)

Hmm, so I tried grabbing the old-version module definitions from here
and pasting them into the new Cluster.pm, but that doesn't work, as it
seems we've never handled some of those problems.

> If there's interest I'll work on supporting our official "old" versions
> (i.e. 9.2 and up)

I'm not sure it's really worth the trouble, depending on how much effort
it is, or how much uglier would Cluster.pm get.  Maybe supporting back
to 10 is enough, assuming I can reproduce crake's failure with 10, which
I think should be possible.

> It's running the buildfarm cross version upgrade module. See
> <https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/TestUpgradeXversion.pm>

Thanks, I'll have a look and see if I can get this to run on my side.

> It's choking on the change in constraint names between the dump of the
> pre-upgrade database and the dump of the post-upgrade database, e.g.
> 
>  CREATE TABLE public.rule_and_refint_t2 (
> -    id2a integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
> -    id2c integer CONSTRAINT pgdump_throwaway_notnull_1 NOT NULL NO INHERIT
> +    id2a integer CONSTRAINT rule_and_refint_t2_id2a_not_null NOT NULL NO INHERIT,
> +    id2c integer CONSTRAINT rule_and_refint_t2_id2c_not_null NOT NULL NO INHERIT
>  );

Yeah, I saw this, and it was pretty obvious that the change I reverted
in d72d32f52d26 was the culprit.  It's all good now.

> I assume that means pg_dump is generating names that pg_upgrade throws away?
> That seems ... unfortunate.

Well, I don't know if you're aware, but now pg_dump will include
throwaway not-null constraints for all columns of primary keys that
don't have an explicit not-null constraint.  Later in the same dump, the
creation of the primary key removes that constraint.  pg_upgrade doesn't
really play any role here, except that apparently the throwaway
constraint name is being preserved, or something.

Anyway, it's moot [for] now.

BTW because of a concern from Justin that the NO INHERIT stuff will
cause errors in old server versions, I started to wonder if it wouldn't
be better to add these constraints in a separate line for compatibility,
so for example in the table above it'd be

CREATE TABLE public.rule_and_refint_t2 (
    id2a integer,
    id2c integer
);
ALTER TABLE public.rule_and_refint_t2 ADD CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL id2a NO INHERIT;
ALTER TABLE public.rule_and_refint_t2 ADD CONSTRAINT pgdump_throwaway_notnull_1 NOT NULL id2c NO INHERIT;

which might be less problematic in terms of compatibility: you still end
up having the table, it's only the ALTER TABLE that would error out.

> There is a perl module at src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm.
> This is used to adjust the dump files before we diff them. Maybe you can
> remedy the problem by adding some code in there.

Hopefully nothing is needed there.  (I think it would be difficult to
make the names match anyway.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"If you have nothing to say, maybe you need just the right tool to help you
not say it."                   (New York Times, about Microsoft PowerPoint)

Re: pgsql: Fix restore of not-null constraints with inheritance

Justin Pryzby <pryzby@telsasoft.com> — 2024-04-21T01:25:48Z

On Fri, Apr 19, 2024 at 01:59:31PM +0200, Alvaro Herrera wrote:
> BTW because of a concern from Justin that the NO INHERIT stuff will
> cause errors in old server versions, I started to wonder if it wouldn't
> be better to add these constraints in a separate line for compatibility,
> so for example in the table above it'd be
> 
> CREATE TABLE public.rule_and_refint_t2 (
>     id2a integer,
>     id2c integer
> );
> ALTER TABLE public.rule_and_refint_t2 ADD CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL id2a NO INHERIT;
> ALTER TABLE public.rule_and_refint_t2 ADD CONSTRAINT pgdump_throwaway_notnull_1 NOT NULL id2c NO INHERIT;
> 
> which might be less problematic in terms of compatibility: you still end
> up having the table, it's only the ALTER TABLE that would error out.

Under pg_restore -d, those would all be run in a single transactional
command, so it would *still* fail to create the table...

It seems like the workaround to restore into an old server version would
be to run:
| pg_restore -f- |sed 's/ NO INHERIT//' |psql

Putting them on separate lines makes that a tiny bit better, since you
could do:
| pg_restore -f- |sed '/^ALTER TABLE .* ADD CONSTRAINT .* NOT NULL/{ s/ NO INHERIT// }' |psql

But I'm not sure whether that's enough of an improvement to warrant the
effort.

-- 
Justin