optimize file transfer in pg_upgrade

For clusters with many relations, the file transfer step of pg_upgrade can
take the longest. This step clones, copies, or links the user relation
files from the older cluster to the new cluster, so the amount of time it
takes is closely related to the number of relations. However, since v15,
we've preserved the relfilenodes during pg_upgrade, which means that all of
these user relation files will have the same name. Therefore, it can be
much faster to instead move the entire data directory from the old cluster
to the new cluster and to then swap the catalog relation files.

The attached proof-of-concept patches implement this "catalog-swap" mode
for demonstration purposes. I tested this mode on a cluster with 200
databases, each with 10,000 tables with 1,000 rows and 2 unique constraints
apiece. Each database also had 10,000 sequences. The test used 96 jobs.

pg_upgrade --link --sync-method syncfs --> 10m 23s (~5m linking)
pg_upgrade --catalog-swap --> 5m 32s (~30s linking)

While these results are encouraging, there are a couple of interesting
problems to manage. First, in order to move the data directory from the
old cluster to the new cluster, we will have first moved the new cluster's
data directory (full of files created by pg_restore) aside. After the file
transfer stage, this directory will be filled with useless empty files that
should eventually be deleted. Furthermore, none of these files will have
been synchronized to disk (outside of whatever the kernel has done in the
background), so pg_upgrade's data synchronization step can take a very long
time, even when syncfs() is used (so long that pg_upgrade can take even
longer than before). After much testing, the best way I've found to deal
with this problem is to introduce a special mode for "initdb --sync-only"
that calls fsync() for everything _except_ the actual data files. If we
fsync() the new catalog files as we move them into place, and if we assume
that the old catalog files will have been properly synchronized before
upgrading, there's no reason to synchronize them again at the end.

Another interesting problem is that pg_upgrade currently doesn't transfer
the sequence data files. Since v10, we've restored these via pg_restore.
I believe this was originally done for the introduction of the pg_sequence
catalog, which changed the format of sequence tuples. In the new
catalog-swap mode I am proposing, this means we need to transfer all the
pg_restore-generated sequence data files. If there are many sequences, it
can be difficult to determine which transfer mode and synchronization
method will be faster. Since sequence tuple modifications are very rare, I
think the new catalog-swap mode should just use the sequence data files
from the old cluster whenever possible.

There are a couple of other smaller trade-offs with this approach, too.
First, this new mode complicates rollback if, say, the machine loses power
during file transfer. IME the vast majority of failures happen before this
step, and it should be relatively simple to generate a script that will
safely perform the required rollback steps, so I don't think this is a
deal-breaker. Second, this mode leaves around a bunch of files that users
would likely want to clean up at some point. I think the easiest way to
handle this is to just put all these files in the old cluster's data
directory so that the cleanup script generated by pg_upgrade also takes
care of them.

Thoughts?

--
nathan

On Wed, Nov 6, 2024 at 5:07 PM Nathan Bossart <nathandbossart(at)gmail(dot)com>
wrote:

> Therefore, it can be much faster to instead move the entire data directory
> from the old cluster
> to the new cluster and to then swap the catalog relation files.
>

Thank you for breaking this up so clearly into separate commits. I think it
is a very interesting idea, and anything to speed up pg_upgrade is always
welcome. Some minor thoughts:

> [PATCH v1 3/8] Introduce catalog-swap mode for pg_upgrade.
> .. we don't really expect there to be directories within database
directories,
> so perhaps it would be better to either unconditionally rename or to fail.
Failure seems the best option here, so we can cleanly handle any future
cases in which we decide to put dirs in this directory.

> if (RelFileNumberIsValid(rfn))
> {
> FileNameMap key;
>
> key.relfilenumber = (RelFileNumber) rfn;
> if (bsearch(&key, context->maps, context->size,
> sizeof(FileNameMap), FileNameMapCmp))
> return 0;
> }
>
> snprintf(dst, sizeof(dst), "%s/%s", context->target, filename);
> if (rename(fname, dst) != 0)

I'm not quite clear what we are doing here with falling through
for InvalidOid entries, could you explain?

> .. vm_must_add_frozenbit isn't handled yet. We could either disallow
> using catalog-swap mode if the upgrade involves versions older than v9.6

Yes, this. No need for more code to handle super old versions when other
options exist.

with this problem is to introduce a special mode for "initdb --sync-only"
> that calls fsync() for everything _except_ the actual data files. If we
> fsync() the new catalog files as we move them into place, and if we assume
> that the old catalog files will have been properly synchronized before
> upgrading, there's no reason to synchronize them again at the end.
>

Very cool approach!

Cheers,
Greg

On Wed, Nov 6, 2024 at 04:07:35PM -0600, Nathan Bossart wrote:
> For clusters with many relations, the file transfer step of pg_upgrade can
> take the longest. This step clones, copies, or links the user relation
> files from the older cluster to the new cluster, so the amount of time it
> takes is closely related to the number of relations. However, since v15,
> we've preserved the relfilenodes during pg_upgrade, which means that all of
> these user relation files will have the same name. Therefore, it can be
> much faster to instead move the entire data directory from the old cluster
> to the new cluster and to then swap the catalog relation files.

That is certainly a creative idea. I am surprised the links take so
long. Obviously rollback would be hard, as you mentioned, while now you
can rollback --link until you start. I think it clearly should be
considered. The patch is smaller than I expected.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

When a patient asks the doctor, "Am I going to die?", he means
"Am I going to die soon?"

On Sun, Nov 17, 2024 at 01:50:53PM -0500, Greg Sabino Mullane wrote:
> On Wed, Nov 6, 2024 at 5:07 PM Nathan Bossart <nathandbossart(at)gmail(dot)com>
> wrote:
>> Therefore, it can be much faster to instead move the entire data directory
>> from the old cluster
>> to the new cluster and to then swap the catalog relation files.
>
> Thank you for breaking this up so clearly into separate commits. I think it
> is a very interesting idea, and anything to speed up pg_upgrade is always
> welcome. Some minor thoughts:

Thank you for reviewing!

>> .. we don't really expect there to be directories within database
> directories,
>> so perhaps it would be better to either unconditionally rename or to fail.
> Failure seems the best option here, so we can cleanly handle any future
> cases in which we decide to put dirs in this directory.

Good point.

>> if (RelFileNumberIsValid(rfn))
>> {
>> FileNameMap key;
>>
>> key.relfilenumber = (RelFileNumber) rfn;
>> if (bsearch(&key, context->maps, context->size,
>> sizeof(FileNameMap), FileNameMapCmp))
>> return 0;
>> }
>>
>> snprintf(dst, sizeof(dst), "%s/%s", context->target, filename);
>> if (rename(fname, dst) != 0)
>
> I'm not quite clear what we are doing here with falling through
> for InvalidOid entries, could you explain?

The idea is that if it looks like a data file that we might want to
transfer (i.e., it starts with a RelFileNumber), we should consult our map
to determine whether to move it. Otherwise, we want to unconditionally
transfer it so that we always use the files generated during pg_restore in
the new cluster (e.g., PG_VERSION and pg_filenode.map). In theory, this
should result in the same end state as what --link mode does today (for the
new cluster, at least).

>> .. vm_must_add_frozenbit isn't handled yet. We could either disallow
>> using catalog-swap mode if the upgrade involves versions older than v9.6
>
> Yes, this. No need for more code to handle super old versions when other
> options exist.

I'm inclined to agree.

>> with this problem is to introduce a special mode for "initdb --sync-only"
>> that calls fsync() for everything _except_ the actual data files. If we
>> fsync() the new catalog files as we move them into place, and if we assume
>> that the old catalog files will have been properly synchronized before
>> upgrading, there's no reason to synchronize them again at the end.
>
> Very cool approach!

:)

--
nathan

On Mon, Nov 18, 2024 at 10:34:00PM -0500, Bruce Momjian wrote:
> On Wed, Nov 6, 2024 at 04:07:35PM -0600, Nathan Bossart wrote:
>> For clusters with many relations, the file transfer step of pg_upgrade can
>> take the longest. This step clones, copies, or links the user relation
>> files from the older cluster to the new cluster, so the amount of time it
>> takes is closely related to the number of relations. However, since v15,
>> we've preserved the relfilenodes during pg_upgrade, which means that all of
>> these user relation files will have the same name. Therefore, it can be
>> much faster to instead move the entire data directory from the old cluster
>> to the new cluster and to then swap the catalog relation files.
>
> That is certainly a creative idea. I am surprised the links take so
> long. Obviously rollback would be hard, as you mentioned, while now you
> can rollback --link until you start. I think it clearly should be
> considered.

I've yet to try, but I'm cautiously optimistic that it will be possible to
generate simple scripts that can unwind things by just looking at the
directory entries, even if pg_upgrade crashed halfway through the linking
stage.

> The patch is smaller than I expected.

I was surprised by this, too. Obviously, this one is a bit smaller than
the "real" patches will be because it's just a proof-of-concept, but it
should still be pretty manageable.

--
nathan

Here is a rebased patch set for cfbot. I'm planning to spend some time
getting these patches into a more reviewable state in the near future.

--
nathan

I've spent quite a bit of time recently trying to get this patch set into a
reasonable state. It's still a little rough around the edges, and the code
for the generated scripts is incomplete, but I figured I'd at least get
some CI testing going.

--
nathan

On Wed, Nov 6, 2024 at 5:07 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> these user relation files will have the same name. Therefore, it can be
> much faster to instead move the entire data directory from the old cluster
> to the new cluster and to then swap the catalog relation files.

This is a cool idea.

> Another interesting problem is that pg_upgrade currently doesn't transfer
> the sequence data files. Since v10, we've restored these via pg_restore.
> I believe this was originally done for the introduction of the pg_sequence
> catalog, which changed the format of sequence tuples. In the new
> catalog-swap mode I am proposing, this means we need to transfer all the
> pg_restore-generated sequence data files. If there are many sequences, it
> can be difficult to determine which transfer mode and synchronization
> method will be faster. Since sequence tuple modifications are very rare, I
> think the new catalog-swap mode should just use the sequence data files
> from the old cluster whenever possible.

Maybe we should rethink the decision not to transfer relfilenodes for
sequences. Or have more than one way to do it. pg_upgrade
--binary-upgrade --binary-upgrade-even-for-sequences, or whatever.

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

On Fri, Feb 28, 2025 at 2:40 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Maybe we should rethink the decision not to transfer relfilenodes for
> sequences. Or have more than one way to do it. pg_upgrade
> --binary-upgrade --binary-upgrade-even-for-sequences, or whatever.

Sorry, I meant: pg_dump --binary-upgrade --binary-upgrade-even-for-sequences

i.e. pg_upgrade could decide which way to ask pg_dump to do it,
depending on versions and flags.

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

On Fri, Feb 28, 2025 at 02:41:22PM -0500, Robert Haas wrote:
> On Fri, Feb 28, 2025 at 2:40 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Maybe we should rethink the decision not to transfer relfilenodes for
>> sequences. Or have more than one way to do it. pg_upgrade
>> --binary-upgrade --binary-upgrade-even-for-sequences, or whatever.
>
> Sorry, I meant: pg_dump --binary-upgrade --binary-upgrade-even-for-sequences
>
> i.e. pg_upgrade could decide which way to ask pg_dump to do it,
> depending on versions and flags.

That's exactly where I landed (see v3-0002). I haven't measured whether
transferring relfilenodes or dumping the sequence data is faster for the
existing modes, but for now I've left those alone, i.e., they still dump
sequence data. The new "swap" mode just uses the old cluster's sequence
files, and I've disallowed using swap mode for upgrades from <v10 to avoid
the sequence tuple format change (along with other incompatible changes).

I'll admit I'm a bit concerned that this will cause problems if and when
someone wants to change the sequence tuple format again. But that hasn't
happened for a while, AFAIK nobody's planning to change it, and even if it
does happen, we just need to have my proposed new mode transfer the
sequence files like it transfers the catalog files. That will make this
mode slower, especially if you have a ton of sequences, but maybe it'll
still be a win in most cases. Of course, we probably will need to have
pg_upgrade handle other kinds of format changes, too, but IMHO it's still
worth trying to speed up pg_upgrade despite the potential future
complexities.

--
nathan

On Fri, Feb 28, 2025 at 3:01 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> That's exactly where I landed (see v3-0002). I haven't measured whether
> transferring relfilenodes or dumping the sequence data is faster for the
> existing modes, but for now I've left those alone, i.e., they still dump
> sequence data. The new "swap" mode just uses the old cluster's sequence
> files, and I've disallowed using swap mode for upgrades from <v10 to avoid
> the sequence tuple format change (along with other incompatible changes).

Ah. Perhaps I should have read the thread more carefully before
commenting. Sounds good, at any rate.

> I'll admit I'm a bit concerned that this will cause problems if and when
> someone wants to change the sequence tuple format again. But that hasn't
> happened for a while, AFAIK nobody's planning to change it, and even if it
> does happen, we just need to have my proposed new mode transfer the
> sequence files like it transfers the catalog files. That will make this
> mode slower, especially if you have a ton of sequences, but maybe it'll
> still be a win in most cases. Of course, we probably will need to have
> pg_upgrade handle other kinds of format changes, too, but IMHO it's still
> worth trying to speed up pg_upgrade despite the potential future
> complexities.

I think it's fine. If somebody comes along and says "hey, when v23
came out Nathan's feature only sped up pg_upgrade by 2x instead of 3x
like it did for v22, so Nathan is a bad person," I think we can fairly
reply "thanks for sharing your opinion, feel free not to use the
feature and run at 1x speed". There's no rule saying that every
optimization must always produce the maximum possible benefit in every
scenario. We're just concerned about regressions, and "only delivers
some of the speedup if the sequence format has changed on disk" is not
a regression.

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

On Fri, Feb 28, 2025 at 03:37:49PM -0500, Robert Haas wrote:
> On Fri, Feb 28, 2025 at 3:01 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>> That's exactly where I landed (see v3-0002). I haven't measured whether
>> transferring relfilenodes or dumping the sequence data is faster for the
>> existing modes, but for now I've left those alone, i.e., they still dump
>> sequence data. The new "swap" mode just uses the old cluster's sequence
>> files, and I've disallowed using swap mode for upgrades from <v10 to avoid
>> the sequence tuple format change (along with other incompatible changes).
>
> Ah. Perhaps I should have read the thread more carefully before
> commenting. Sounds good, at any rate.

On the contrary, I'm glad you independently came to the same conclusion.

>> I'll admit I'm a bit concerned that this will cause problems if and when
>> someone wants to change the sequence tuple format again. But that hasn't
>> happened for a while, AFAIK nobody's planning to change it, and even if it
>> does happen, we just need to have my proposed new mode transfer the
>> sequence files like it transfers the catalog files. That will make this
>> mode slower, especially if you have a ton of sequences, but maybe it'll
>> still be a win in most cases. Of course, we probably will need to have
>> pg_upgrade handle other kinds of format changes, too, but IMHO it's still
>> worth trying to speed up pg_upgrade despite the potential future
>> complexities.
>
> I think it's fine. If somebody comes along and says "hey, when v23
> came out Nathan's feature only sped up pg_upgrade by 2x instead of 3x
> like it did for v22, so Nathan is a bad person," I think we can fairly
> reply "thanks for sharing your opinion, feel free not to use the
> feature and run at 1x speed". There's no rule saying that every
> optimization must always produce the maximum possible benefit in every
> scenario. We're just concerned about regressions, and "only delivers
> some of the speedup if the sequence format has changed on disk" is not
> a regression.

Cool. I appreciate the design feedback.

--
nathan

On Fri, Feb 28, 2025 at 02:51:27PM -0600, Nathan Bossart wrote:
> Cool. I appreciate the design feedback.

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 believe that's worth considering for the
following reasons:

* 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
suspect that failures during file transfer are typically due to OS
crashes, power losses, etc., and hopefully those are rare.

* I've spent quite some time trying to generate a portable script, but it's
quite complicated and difficult to reason about its correctness. And I
haven't even started on the Windows version. Leaving this part out would
simplify the patch set quite a bit.

* If we give up the idea of reverting to the old cluster, we also can avoid
a bunch of intermediate fsync() calls which I only included to help
reason about the state of the files in case you failed halfway through.
This might not add up to much, but it's at least another area of
simplification.

Of course, rollback would still be possible, but you'd really need to
understand what "swap" mode does behind the scenes to do so safely. In any
case, I'm growing skeptical that a probably-not-super-well-tested script
that extremely few people will need and fewer will use is worth the
complexity.

--
nathan

On Wed, Mar 5, 2025 at 2:42 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> Of course, rollback would still be possible, but you'd really need to
> understand what "swap" mode does behind the scenes to do so safely. In any
> case, I'm growing skeptical that a probably-not-super-well-tested script
> that extremely few people will need and fewer will use is worth the
> complexity.

I don't have a super-strong view on what the right thing to do is
here, but I'm definitely in favor of not doing something half-baked.
If you think the revert script is going to suck, then let's not have
it at all and just be clear about what the requirements for using this
mode are.

One serious danger that you didn't mention here is that, if pg_upgrade
does fail, you may well try several times. And if you forget the
revert script at some point, or run it more than once, or run the
wrong version, you will be in trouble. I feel like this is something
someone could very easily mess up even if in theory it works
perfectly. Upgrades are often stressful times.

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

On Wed, Mar 5, 2025 at 2:43 PM Nathan Bossart <nathandbossart(at)gmail(dot)com>
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.

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

On Wed, Mar 05, 2025 at 03:40:52PM -0500, Greg Sabino Mullane wrote:
> 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.

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.

--
nathan

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 <nathandbossart(at)gmail(dot)com> 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 <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

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

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

On Mon, Mar 17, 2025 at 3:34 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> * Once committed, I should update one of my buildfarm animals to use
> 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

On Mon, Mar 17, 2025 at 04:04:45PM -0400, Robert Haas wrote:
> On Mon, Mar 17, 2025 at 3:34 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>> * Once committed, I should update one of my buildfarm animals to use
>> 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?

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

On Mon, Mar 17, 2025 at 4:30 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> On Mon, Mar 17, 2025 at 04:04:45PM -0400, Robert Haas wrote:
> > On Mon, Mar 17, 2025 at 3:34 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> >> * Once committed, I should update one of my buildfarm animals to use
> >> 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?
>
> 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?

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.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 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.

+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

Hi,

On 2025-03-18 10:04:41 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > 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.
>
> +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.

+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

On 2025-Mar-18, Robert Haas wrote:

> 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.

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)

On Tue, Mar 18, 2025 at 10:12:51AM -0400, Andres Freund wrote:
> On 2025-03-18 10:04:41 -0400, Tom Lane wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> > 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.
>>
>> +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.
>
> +1

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

Hi,

On 2025-03-18 12:29:02 -0500, Nathan Bossart wrote:
> 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?

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

On Tue, Mar 18, 2025 at 01:37:02PM -0400, Andres Freund wrote:
> 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.

+1

> - Perhaps create a tablespace?

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

> - Do we need a new old cluster for each of the modes? That seems like wasted
> 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

On 2025-03-18 12:47:01 -0500, Nathan Bossart wrote:
> On Tue, Mar 18, 2025 at 01:37:02PM -0400, Andres Freund wrote:
> > - Do we need a new old cluster for each of the modes? That seems like wasted
> > 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*

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

On Tue, Mar 18, 2025 at 01:50:10PM -0400, Andres Freund wrote:
> On 2025-03-18 12:47:01 -0500, Nathan Bossart wrote:
>> On Tue, Mar 18, 2025 at 01:37:02PM -0400, Andres Freund wrote:
>> > - Do we need a new old cluster for each of the modes? That seems like wasted
>> > 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*
>
> 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

On Tue, Mar 18, 2025 at 02:08:42PM -0500, Nathan Bossart wrote:
> 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).

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

--
nathan

On Tue, Mar 18, 2025 at 09:14:22PM -0500, Nathan Bossart wrote:
> 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

Hi,

On 2025-03-18 21:14:22 -0500, Nathan Bossart wrote:
> From 8b6a5e0148c2f7a663f5003f12ae9461d2b06a5c Mon Sep 17 00:00:00 2001
> From: Nathan Bossart <nathan(at)postgresql(dot)org>
> 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

Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
> 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).

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

regards, tom lane

Hi,

On 2025-03-19 12:28:33 -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
> > 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).
>
> Agreed that that shouldn't block this, but we need some kind of
> 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

On Wed, Mar 19, 2025 at 12:44:38PM -0400, Andres Freund wrote:
> On 2025-03-19 12:28:33 -0400, Tom Lane wrote:
>> Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
>> > 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).
>>
>> Agreed that that shouldn't block this, but we need some kind of
>> 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?

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

On Wed, Mar 19, 2025 at 02:32:01PM -0500, Nathan Bossart wrote:
> 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.

Actually, this one was pretty easy to do.

--
nathan

On Wed, Mar 19, 2025 at 04:28:23PM -0500, Nathan Bossart wrote:
> On Wed, Mar 19, 2025 at 02:32:01PM -0500, Nathan Bossart wrote:
>> 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.
>
> 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

On Wed, Mar 19, 2025 at 09:02:42PM -0500, Nathan Bossart wrote:
> On Wed, Mar 19, 2025 at 04:28:23PM -0500, Nathan Bossart wrote:
>> On Wed, Mar 19, 2025 at 02:32:01PM -0500, Nathan Bossart wrote:
>>> 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.
>>
>> 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...

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

--
nathan

On Thu, Mar 20, 2025 at 11:11:46AM -0500, Nathan Bossart wrote:
> As promised, I've committed just 0001 for now. I'll watch closely for any
> issues in the buildfarm.

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

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

Hello Nathan,

20.03.2025 04:02, Nathan Bossart wrote:
> On Wed, Mar 19, 2025 at 04:28:23PM -0500, Nathan Bossart wrote:
> 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...

I found a couple of the 006_transfer_modes failures during the past month:
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

Both happened on Windows, but what's worse is that the failure logs
contain no information on the exact reason. We can see:
#   Failed test 'pg_upgrade with transfer mode --swap: stdout matches'
#   at C:/tools/xmsys64/home/pgrunner/bf/root/HEAD/pgsql/src/bin/pg_upgrade/t/006_transfer_modes.pl line 61.
...
# Restoring database schemas in the new cluster
# *failure*
#
# Consult the last few lines of
"C:/tools/xmsys64/home/pgrunner/bf/root/HEAD/pgsql.build/testrun/pg_upgrade/006_transfer_modes/data/t_006_transfer_modes_new_data/pgdata/pg_upgrade_output.d/20250421T081115.575/log/pg_upgrade_dump_1.log"
for
# the probable cause of the failure.
# Failure, exiting
# '
#     doesn't match '(?^:.* not supported on this platform|could not .* between old and new data directories: .*)'

there is a reference to pg_upgrade_dump_x.log, but no such files saved.

I tried to reproduce this failure locally, but failed. Still I discovered
that when the test fails, the target directory containing pgdata/ gets
removed, because of this coding:
    my $result = command_ok_or_fails_like(
...
    # If pg_upgrade was successful, check that all of our test objects reached
    # the new version.
    if ($result)
    {
...
    }

    $old->clean_node();
    $new->clean_node();

Moreover, even when pg_upgrade succeeds, IPC::Run::run inside
command_ok_or_fails_like() returns false, as we can see from a
successful test run:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2025-04-27%2001%3A03%3A06&stg=misc-check

pgsql.build/testrun/pg_upgrade/006_transfer_modes/log/regress_log_006_transfer_modes
[01:18:38.210](21.036s) ok 1 - pg_upgrade with transfer mode --clone: stdout matches
[01:18:38.211](0.001s) ok 2 - pg_upgrade with transfer mode --clone: stderr matches

The corresponding code is:
    print("# Running: " . join(" ", @{$cmd}) . "\n");
    my $result = IPC::Run::run $cmd, '>' => \$stdout, '2>' => \$stderr;
    if (!$result)
    {
        like($stdout, $expected_stdout, "$test_name: stdout matches");
        like($stderr, $expected_stderr, "$test_name: stderr matches");
    }

So maybe it's worth to adjust the test somehow to have interesting logs
left after a failure?

Best regards,
Alexander Lakhin
Neon (https://neon.tech)

On Sun, Apr 27, 2025 at 05:00:01PM +0300, Alexander Lakhin wrote:
> Both happened on Windows, but what's worse is that the failure logs
> contain no information on the exact reason. We can see:
> #   Failed test 'pg_upgrade with transfer mode --swap: stdout matches'
> #   at C:/tools/xmsys64/home/pgrunner/bf/root/HEAD/pgsql/src/bin/pg_upgrade/t/006_transfer_modes.pl line 61.
> ...
> # Restoring database schemas in the new cluster
> # *failure*

I see a couple of other pg_upgrade failures on drongo and fairywren that
look similar, although these are for different tests:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2025-03-10%2019%3A26%3A35
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2025-03-30%2013%3A03%3A05

> Moreover, even when pg_upgrade succeeds, IPC::Run::run inside
> command_ok_or_fails_like() returns false, as we can see from a
> successful test run:
> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2025-04-27%2001%3A03%3A06&stg=misc-check
>
> pgsql.build/testrun/pg_upgrade/006_transfer_modes/log/regress_log_006_transfer_modes
> [01:18:38.210](21.036s) ok 1 - pg_upgrade with transfer mode --clone: stdout matches
> [01:18:38.211](0.001s) ok 2 - pg_upgrade with transfer mode --clone: stderr matches

That's expected for platforms that don't support all of the modes. We
verify the output matches a known error message in that case.

> So maybe it's worth to adjust the test somehow to have interesting logs
> left after a failure?

I see some other discussion about failures with similar symptoms [0] [1].
Commit 6f97ef0 seems to have helped with one of the tests, and there is a
proposed patch in the latest thread [2] that AFAICT aims to fix the
underlying issue.

[0] https://postgr.es/m/TYAPR01MB5866AB7FD922CE30A2565B8BF5A8A%40TYAPR01MB5866.jpnprd01.prod.outlook.com
[1] https://postgr.es/m/CALDaNm3tjY44HoSwY84%3DXGEbTg0ruVfD4hAMTm%3DTgBqVysH4Qw%40mail.gmail.com
[2] https://postgr.es/m/CALDaNm2y%2Bnf-V9tjKwvbPprobZs1t_UrcCpJ0qYD5-KkOUFAyg%40mail.gmail.com

--
nathan

Hello Nathan,

28.04.2025 18:15, Nathan Bossart wrote:
> I see a couple of other pg_upgrade failures on drongo and fairywren that
> look similar, although these are for different tests:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2025-03-10%2019%3A26%3A35
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2025-03-30%2013%3A03%3A05

Yeah, I've categorized the first one as [1], but now I see that it's
something different, because "pg_upgrade_output.d/ removed after
successful pg_upgrade" is not the only (and not the first) failure there.
Will fix. As to the second one, yes, it's similar in the sense that the
failed test log doesn't contain information needed to understand the
cause.

>> Moreover, even when pg_upgrade succeeds, IPC::Run::run inside
>> command_ok_or_fails_like() returns false, as we can see from a
>> successful test run:
>> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2025-04-27%2001%3A03%3A06&stg=misc-check
>>
>> pgsql.build/testrun/pg_upgrade/006_transfer_modes/log/regress_log_006_transfer_modes
>> [01:18:38.210](21.036s) ok 1 - pg_upgrade with transfer mode --clone: stdout matches
>> [01:18:38.211](0.001s) ok 2 - pg_upgrade with transfer mode --clone: stderr matches
> That's expected for platforms that don't support all of the modes. We
> verify the output matches a known error message in that case.

Yes, I meant that in that case we can't determine whether to preserve logs
of the failed pg_upgrade outside of command_ok_or_fails_like().

>> So maybe it's worth to adjust the test somehow to have interesting logs
>> left after a failure?
> I see some other discussion about failures with similar symptoms [0] [1].
> Commit 6f97ef0 seems to have helped with one of the tests, and there is a
> proposed patch in the latest thread [2] that AFAICT aims to fix the
> underlying issue.

Thank you for the references! Unfortunately I still can't see where the
lack of upgrade log files is discussed.

In other words, if we had logs like in the case [2], it could be helpful.

[1]
https://wiki.postgresql.org/wiki/Known_Buildfarm_Test_Failures#Upgrade_tests_fail_on_Windows_due_to_pg_upgrade_output.d.2F_not_removed
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=copperhead&dt=2025-02-20%2017%3A01%3A23

Best regards,
Alexander Lakhin
Neon (https://neon.tech)

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

Hello Nathan,

28.04.2025 21:26, Nathan Bossart wrote:
> One other potential problem with this test is that we reuse the directory
> 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/7ff9de7f-7203-cad9-76d9-45497cbedac7@gmail.com

Best regards,
Alexander