Feedback-ID: ia2694551:Fastmail
Date: Fri, 19 Apr 2024 13:59:31 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: pgsql-hackers@lists.postgresql.org,
	Justin Pryzby <pryzby@telsasoft.com>
Subject: Re: pgsql: Fix restore of not-null constraints with inheritance
Message-ID: <202404191159.cakeaydac4sq@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3a642a59-e3c7-4050-95dd-af3b5412e5f7@dunslane.net>
Archived-At: 
 <https://www.postgresql.org/message-id/202404191159.cakeaydac4sq%40alvherre.pgsql>
Precedence: bulk

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)