add --no-sync to pg_upgrade's calls to pg_dump and pg_dumpall

On 03.05.24 19:13, Nathan Bossart wrote:

This is likely small potatoes compared to some of the other
pg_upgrade-related improvements I've proposed [0] [1] or plan to propose,
but this is easy enough, and I already wrote the patch, so here it is.
AFAICT there's no reason to bother syncing these dump files to disk. If
someone pulls the plug during pg_upgrade, it's not like you can resume
pg_upgrade from where it left off. Also, I think we skipped syncing before
v10, anyway, as the --no-sync flag was only added in commit 96a7128, which
added the code to sync dump files, too.

Looks good to me.

On Wed, May 08, 2024 at 10:09:46AM +0200, Peter Eisentraut wrote:

On 03.05.24 19:13, Nathan Bossart wrote:

This is likely small potatoes compared to some of the other
pg_upgrade-related improvements I've proposed [0] [1] or plan to propose,
but this is easy enough, and I already wrote the patch, so here it is.
AFAICT there's no reason to bother syncing these dump files to disk. If
someone pulls the plug during pg_upgrade, it's not like you can resume
pg_upgrade from where it left off. Also, I think we skipped syncing before
v10, anyway, as the --no-sync flag was only added in commit 96a7128, which
added the code to sync dump files, too.

Looks good to me.

Thanks for looking. I noticed that the version check is unnecessary since
we always use the new binary's pg_dump[all], so I removed that in v2.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:

Thanks for looking. I noticed that the version check is unnecessary since
we always use the new binary's pg_dump[all], so I removed that in v2.

+1

regards, tom lane

On Wed, May 08, 2024 at 02:49:58PM -0400, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

Thanks for looking. I noticed that the version check is unnecessary since
we always use the new binary's pg_dump[all], so I removed that in v2.

+1

+1.  Could there be an argument in favor of a backpatch?  This is a
performance improvement, but one could also side that the addition of
sync support in pg_dump[all] has made that a regression that we'd
better fix because the flushes don't matter in this context.  They
also bring costs for no gain.
--
Michael

On Thu, May 09, 2024 at 09:03:56AM +0900, Michael Paquier wrote:

+1. Could there be an argument in favor of a backpatch? This is a
performance improvement, but one could also side that the addition of
sync support in pg_dump[all] has made that a regression that we'd
better fix because the flushes don't matter in this context. They
also bring costs for no gain.

I don't see a strong need to back-patch this, if for no other reason than
it seems to have gone unnoticed for 7 major versions. Plus, based on my
admittedly limited testing, this is unlikely to provide significant
improvements.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 9 May 2024, at 21:34, Nathan Bossart <nathandbossart@gmail.com> wrote:

On Thu, May 09, 2024 at 09:03:56AM +0900, Michael Paquier wrote:

+1. Could there be an argument in favor of a backpatch? This is a
performance improvement, but one could also side that the addition of
sync support in pg_dump[all] has made that a regression that we'd
better fix because the flushes don't matter in this context. They
also bring costs for no gain.

I don't see a strong need to back-patch this, if for no other reason than
it seems to have gone unnoticed for 7 major versions. Plus, based on my
admittedly limited testing, this is unlikely to provide significant
improvements.

Agreed, this is a nice little improvement but it's unlikely to be enough of a
speedup to warrant changing the backbranches.

--
Daniel Gustafsson

Committed.

--
nathan