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

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)

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

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)

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