Subject: Re: optimize file transfer in pg_upgrade

From: Bruce Momjian <[email-masked]>
Date: Mon, 17 Mar 2025 15:27:11 -0400

On Wed, Mar  5, 2025 at 03:40:52PM -0500, Greg Sabino Mullane wrote:
> On Wed, Mar 5, 2025 at 2:43 PM Nathan Bossart <[email-masked]> wrote:
> 
>     One other design point I wanted to bring up is whether we should bother
>     generating a rollback script for the new "swap" mode.  In short, I'm
>     wondering if it would be unreasonable to say that, just for this mode, once
>     pg_upgrade enters the file transfer step, reverting to the old cluster
>     requires restoring a backup.
> 
> 
> I think that's a fair requirement. And like Robert, revert scripts make me
> nervous.
> 
> 
>     * Anecdotally, I'm not sure I've ever actually seen pg_upgrade fail
>     during or after file transfer, and I'm hoping to get some real data about
>     that in the near future.  Has anyone else dealt with such a failure?
> 
> 
> I've seen various failures, but they always get caught quite early. Certainly
> early enough to easily abort, fix perms/mounts/etc., then retry. I think your
> instinct is correct that this reversion is more trouble than its worth. I don't
> think the pg_upgrade docs mention taking a backup, but that's always step 0 in
> my playbook, and that's the rollback plan in the unlikely event of failure.

I avoided many optimizations in pg_upgrade in the fear they would lead
to hard-to-detect bugs, or breakage from major release changes. 
pg_upgrade is probably old enough now (15 years) that we can risk these
optimizations.

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 17 Mar 2025 14:34:34 -0500

On Wed, Mar 05, 2025 at 08:34:37PM -0600, Nathan Bossart wrote:
> Thank you, Greg and Robert, for sharing your thoughts.  With that, here's
> what I'm considering to be a reasonably complete patch set for this
> feature.  This leaves about a month for rigorous testing and editing, so
> I'm hopeful it'll be ready v18.

Here are my notes after a round of self-review.

0001:
* The documentation does not adequately describe the interaction between
  --no-sync-data-files and --sync-method=syncfs.
* I really don't like the exclude_dir hack for skipping the main tablespace
  directory, but I haven't thought of anything that seems better.
* I should verify that there's no path separator issues on Windows for the
  exclude_dir hack.  From some quick code analysis, I think it should work
  fine, but I probably ought to test it out to be sure.
* The documentation needs to mention that the tablespace directories
  themselves are not synchronized.

0002:
* The documentation changes are subject to update based on ongoing stats
  import/export work.
* Does --statistics-only --sequence-data make any sense?  It seems like it
  ought to function as expected, but it's hard to see a use-case.

0003:
* Once committed, I should update one of my buildfarm animals to use
  PG_TEST_PG_UPGRADE_MODE=--swap.
* For check_hard_link() and disable_old_cluster(), move the Assert() to an
  "else" block with a pg_fatal() call for sturdiness.
* I need to do a thorough pass-through on all comments.  Many are not
  sufficiently detailed.
* The "." and ".." checks in the catalog swap code are redundant and can be
  removed.
* The directory for "moved-aside" stuff should be placed within the old
  cluster's corresponding tablespace directory so that no changes need to
  be made to delete_old_cluster.{sh,bat}.
* Manual testing with non-default tablespaces!

Updated patches based on these notes are attached.

-- 
nathan

---

From: Robert Haas <[email-masked]>
Date: Mon, 17 Mar 2025 16:04:45 -0400

>   PG_TEST_PG_UPGRADE_MODE=--swap.

It would be better if we didn't need a separate buildfarm animal to
test this, because that means you won't get notified by local testing
OR by CI if you break this. Can we instead have one test that checks
this which is part of the normal test run?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 17 Mar 2025 15:30:31 -0500

> this which is part of the normal test run?

That's what I set out to do before I discovered PG_TEST_PG_UPGRADE_MODE.
The commit message for b059a24 seemed to indicate that we don't want to
automatically test all supported modes, but I agree that it would be nice
to have some basic coverage for --swap on CI/buildfarm regardless of
PG_TEST_PG_UPGRADE_MODE.  How about we add a simple TAP test (attached),
and I still plan on switching a buildfarm animal to --swap for more
in-depth testing?

-- 
nathan

---

From: Robert Haas <[email-masked]>
Date: Tue, 18 Mar 2025 09:40:52 -0400

> and I still plan on switching a buildfarm animal to --swap for more

The background here is that I'm kind of on the warpath against weird
configurations that we only test on certain buildfarm animals at the
moment, because the result of that is that CI is clean and then the
buildfarm turns red when you commit. That's an unenjoyable experience
for the committer and for everyone who looks at the buildfarm results.
The way to fix it is to stop relying on "rerun all the tests with this
weird mode flag" and rely more on tests that are designed to test that
specific flag and, ideally, that get run by in local testing or at
least by CI.

I'm not quite sure what the best thing is to do is for the pg_upgrade
tests in particular, and it may well be best to do as you propose for
now and figure that out later. But I question whether just rerunning
all of those tests with several different mode flags is the right
thing to do. Why for example does 005_char_signedness.pl need to be
checked under both --link and --clone? I would guess that there are
one or maybe two tests in src/bin/pg_upgrade/t that needs to test
--link and --clone and they should grow internal loops to do that
(when supported by the local platform) and PG_UPGRADE_TEST_MODE should
go in the garbage.

---

From: Tom Lane <[email-masked]>
Date: Tue, 18 Mar 2025 10:04:41 -0400

> (when supported by the local platform) and PG_UPGRADE_TEST_MODE should

+1

I'd be particularly allergic to running 002_pg_upgrade.pl multiple
times, as that's one of our most expensive tests, and I flat out
don't believe that expending that many cycles could be justified.
Surely we can test these modes sufficiently in some much cheaper and
more targeted way.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Tue, 18 Mar 2025 10:12:51 -0400

Hi,

+1

It's useful to have coverage of as many object types as possible in pg_upgrade
- hence 002_pg_upgrade.pl. It helps us to find problems in new code that
didn't think about pg_upgrade.

But that doesn't mean that it's a good idea to run all other pg_upgrade tests
the same way, to the contrary - the cost is too high.

Even leaving runtime aside, I have a hard time believing that --link, --clone,
--swap benefit from running the same way as 002_pg_upgrade.pl - the
implementation of those flags is on a lower level and works the same across
e.g. different index AMs.

I'd go so far as to say that 002_pg_upgrade.pl style testing actually makes it
*harder* to diagnose problems related to things like --link, because there are
no targeted tests, but just a huge set of things that maybe allow to infer
some bug if you spend a lot of time.

Greetings,

Andres Freund

---

From: Álvaro Herrera <[email-masked]>
Date: Tue, 18 Mar 2025 16:51:43 +0100

> specific flag and, ideally, that get run by in local testing or at

FWIW this is exactly the rationale that got me writing an email on the
Ashutosh's thread for a new pg_dump/restore test under
002_pg_upgrade.pl, whereby I was saying that we should not hide it
behind PG_TEST_EXTRA which almost nobody would remember to use.  But I
discarded that draft, because that had actually been Ashutosh's idea at
some point in the thread and had been discarded because of the runtime
increase it'd cause.  But, somehow, I still don't believe the theory
that it's such a bad idea to add a few seconds so that we have such a
comprehensive pg_dump test, with much less programmer overhead than
pg_dump's own weird enormous test script.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat)

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 18 Mar 2025 12:29:02 -0500

>> Surely we can test these modes sufficiently in some much cheaper and

Here is a first sketch at a test that cycles through all the transfer modes
and makes sure they succeed or fail with an error along the lines of "not
supported on this platform."  Each test verifies that some very simple
objects make it to the new version, which we could of course expand on.
Would something like this suffice?

-- 
nathan

---

From: Andres Freund <[email-masked]>
Date: Tue, 18 Mar 2025 13:37:02 -0400

Hi,

> Would something like this suffice?

I'd add a few more complications:

- Create and test a relation that was rewritten, to ensure we test the
  relfilenode != oid case and one that isn't rewritten.

- Perhaps create a tablespace?

- Do we need a new old cluster for each of the modes? That seems like wasted
  time?  I guess it's required for --link...

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 18 Mar 2025 12:47:01 -0500

+1

> - Perhaps create a tablespace?

+1, I don't think we have much, if any, coverage of pg_upgrade with
non-default tablespaces.

>   time?  I guess it's required for --link...

It'll also be needed for --swap.  We could optionally save the old cluster
for a couple of modes if we really wanted to.  *shrug*

I'll work on the first two...

-- 
nathan

---

From: Andres Freund <[email-masked]>
Date: Tue, 18 Mar 2025 13:50:10 -0400

> for a couple of modes if we really wanted to.  *shrug*

Don't worry about it, I think the template initdb stuff should make it cheap enough...

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 18 Mar 2025 14:08:42 -0500

> Don't worry about it, I think the template initdb stuff should make it cheap enough...

Cool.  I realize now why there's poor coverage for pg_upgrade with
tablespaces: you can't upgrade between the same version with tablespaces
(presumably due to the version-specific subdirectory conflict).  I don't
know if the regression tests leave around any tablespaces for the
cross-version pg_upgrade tests, but that's probably the best we can do at
the moment.

For now, here's a new version of the test with a rewritten table.  I also
tried to fix the expected error regex to handle some of the other error
messages for unsupported modes (as revealed by cfbot).

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 18 Mar 2025 21:14:22 -0500

> messages for unsupported modes (as revealed by cfbot).

And here is a new version of the full patch set.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 19 Mar 2025 10:31:22 -0500

> And here is a new version of the full patch set.

I'm currently planning to commit this sometime early-ish next week.  One
notable loose end is the lack of a pg_upgrade test with a non-default
tablespace, but that is an existing problem that IMHO is best handled
separately (since we can only test it in cross-version upgrades).

-- 
nathan

---

From: Andres Freund <[email-masked]>
Date: Wed, 19 Mar 2025 11:41:59 -0400

Hi,

On 2025-03-18 21:14:22 -0500, Nathan Bossart wrote:
> From 8b6a5e0148c2f7a663f5003f12ae9461d2b06a5c Mon Sep 17 00:00:00 2001
> From: Nathan Bossart <[email-masked]>
> Date: Tue, 18 Mar 2025 20:58:07 -0500
> Subject: [PATCH v7 1/4] Add test for pg_upgrade file transfer modes.
> 
> This new test checks all of pg_upgrade's file transfer modes.  For
> each mode, we verify that pg_upgrade either succeeds (and some test
> objects successfully reach the new version) or fails with an error
> that indicates the mode is not supported on the current platform.

LGTM.  I'm sure we could do more than the test does today, but I think it's a
good improvement.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 12:28:33 -0400

> separately (since we can only test it in cross-version upgrades).

Agreed that that shouldn't block this, but we need some kind of
plan for testing it better.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 19 Mar 2025 12:44:38 -0400

Hi,

> plan for testing it better.

Yea, this is really suboptimal.

Shouldn't allow_in_place_tablespaces be sufficient to deal with that scenario?
Or at least it should make it reasonably easy to cope if it doesn't already
suffice?

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 19 Mar 2025 14:32:01 -0500

> Or at least it should make it reasonably easy to cope if it doesn't already

Unfortunately, pg_upgrade can't yet handle in-place tablespaces.  One
reason is that pg_tablespace_location() returns a relative path for those
(e.g., "pg_tblspc/123456").  We'd also need to adjust init_tablespaces() to
not fail if all the tablespaces are in-place.  There may be other reasons,
too.  I'm confident we could get it working, but I'm not too excited about
trying to sneak this into v18.

In addition to testing with in-place tablespaces, we might also want to
teach the transfer modes test to do cross-version testing when possible.
In that case, we can test normal (non-in-place) tablespaces.  However, that
would be limited to the buildfarm.

Does this seem like a reasonable plan for v19?

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 19 Mar 2025 16:28:23 -0500

> would be limited to the buildfarm.

Actually, this one was pretty easy to do.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 19 Mar 2025 21:02:42 -0500

> Actually, this one was pretty easy to do.

And here is yet another new version of the full patch set.  I'm planning to
commit 0001 (the new pg_upgrade transfer mode test) tomorrow so that I can
deal with any buildfarm indigestion before committing swap mode.  I did run
the test locally for upgrades from v9.6, v13, and v17, but who knows what
unique configurations I've failed to anticipate...

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 20 Mar 2025 11:11:46 -0500

> unique configurations I've failed to anticipate...

As promised, I've committed just 0001 for now.  I'll watch closely for any
issues in the buildfarm.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 20 Mar 2025 15:23:13 -0500

> As promised, I've committed just 0001 for now.  I'll watch closely for any

Seeing none, here's is a rebased patch set without 0001.  The only changes
are some fleshed-out comments and commit messages.  I'm still aiming to
commit this sometime early next week.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 25 Mar 2025 16:03:57 -0500

On Thu, Mar 20, 2025 at 03:23:13PM -0500, Nathan Bossart wrote:
> I'm still aiming to commit this sometime early next week.

Committed.  Thanks to everyone who chimed in on this thread.

While writing the attributions, I noticed that nobody seems to have
commented specifically on 0001.  The closest thing to a review I see is
Greg's note upthread [0].  This patch is a little bigger than what I'd
ordinarily feel comfortable with committing unilaterally, but it's been
posted in its current form since February 28th, this thread has gotten a
decent amount of traffic since then, and it's not a huge change ("9 files
changed, 96 insertions(+), 37 deletions(-)").  I'm happy to address any
post-commit feedback that folks have.  As noted earlier [1], I'm not wild
about how it's implemented, but this is the nicest approach I've thought of
thus far.

I also wanted to draw attention to this note in 0003:

		/*
		 * XXX: The below line is a hack to deal with the fact that we
 		 * presently don't have an easy way to find the corresponding new
 		 * tablespace's path.  This will need to be fixed if/when we add
 		 * pg_upgrade support for in-place tablespaces.
 		 */
 		new_tablespace = old_tablespace;

I intend to address this in v19, primarily to enable same-version
pg_upgrade testing with non-default tablespaces.  My current thinking is
that we should have pg_upgrade also gather the new cluster tablespace
information and map them to the corresponding tablespaces on the old
cluster.  This might require some refactoring in pg_upgrade.  In any case,
I didn't feel this should block the feature for v18.

[0] https://postgr.es/m/CAKAnmm%2Bi3Q1pZ05N_b8%3DS3B%3DrztQDn--HoW8BRKVtCg53r8NiQ%40mail.gmail.com
[1] https://postgr.es/m/Z9h5Spp76EBygyEL%40nathan

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 28 Apr 2025 13:26:34 -0500

On Mon, Apr 28, 2025 at 09:00:01PM +0300, Alexander Lakhin wrote:
> Thank you for the references! Unfortunately I still can't see where the
> lack of upgrade log files is discussed.

That was briefly discussed here:

	https://postgr.es/m/644cf995-e3a5-4f69-9398-7db500e2673d%40dunslane.net

One other potential problem with this test is that we reuse the directory
names for each transfer mode.  That seems easy enough to fix.

-- 
nathan

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 28 Dec 2025 09:00:00 +0200

Hello Nathan,

> names for each transfer mode.  That seems easy enough to fix.

FWIW, I've counted seven 006_transfer_modes failures happened during this
year. Five are from Windows animals:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2025-04-08%2004%3A18%3A15
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2025-04-21%2008%3A03%3A06
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2025-07-21%2012%3A35%3A58
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2025-08-22%2000%3A04%3A05
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2025-12-28%2003%3A43%3A24

And two from culicidae, which tests EXEC_BACKEND and thus suffers from [1]
([2]):
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2025-11-22%2012%3A31%3A23
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2025-12-14%2018%3A24%3A48

Probably, this number could justify improving the test so that we can
identify the failure reason for sure looking at the upgrade logs. As of
now, we can only guess it, based on the animals' specifics...

[1] 
https://wiki.postgresql.org/wiki/Known_Buildfarm_Test_Failures#culicidae_failed_to_restart_server_due_to_incorrect_checksum_in_control_file
[2] https://www.postgresql.org/message-id/[email-masked]

Best regards,
Alexander