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

From: Peter Eisentraut <[email-masked]>
Date: Wed, 8 May 2024 10:09:46 +0200

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.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 8 May 2024 13:38:50 -0500

>> added the code to sync dump files, too.

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
>From 265a999ed65bf56491f76ae013f705ab64491486 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Fri, 3 May 2024 10:35:21 -0500
Subject: [PATCH v2 1/1] add --no-sync to pg_upgrade's calls to pg_dump[all]

---
 src/bin/pg_upgrade/dump.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c
index 29fb45b928..8345f55be8 100644
--- a/src/bin/pg_upgrade/dump.c
+++ b/src/bin/pg_upgrade/dump.c
@@ -22,7 +22,7 @@ generate_old_dump(void)
 	/* run new pg_dumpall binary for globals */
 	exec_prog(UTILITY_LOG_FILE, NULL, true, true,
 			  "\"%s/pg_dumpall\" %s --globals-only --quote-all-identifiers "
-			  "--binary-upgrade %s -f \"%s/%s\"",
+			  "--binary-upgrade %s --no-sync -f \"%s/%s\"",
 			  new_cluster.bindir, cluster_conn_opts(&old_cluster),
 			  log_opts.verbose ? "--verbose" : "",
 			  log_opts.dumpdir,
@@ -53,7 +53,7 @@ generate_old_dump(void)
 
 		parallel_exec_prog(log_file_name, NULL,
 						   "\"%s/pg_dump\" %s --schema-only --quote-all-identifiers "
-						   "--binary-upgrade --format=custom %s --file=\"%s/%s\" %s",
+						   "--binary-upgrade --format=custom %s --no-sync --file=\"%s/%s\" %s",
 						   new_cluster.bindir, cluster_conn_opts(&old_cluster),
 						   log_opts.verbose ? "--verbose" : "",
 						   log_opts.dumpdir,
-- 
2.25.1

---

From: Tom Lane <[email-masked]>
Date: Wed, 08 May 2024 14:49:58 -0400

> we always use the new binary's pg_dump[all], so I removed that in v2.

+1

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Thu, 9 May 2024 09:03:56 +0900

>> we always use the new binary's pg_dump[all], so I removed that in v2.

+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

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 9 May 2024 14:34:25 -0500

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

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 9 May 2024 21:58:37 +0200

> On 9 May 2024, at 21:34, Nathan Bossart <[email-masked]> wrote:
> 

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

--
Daniel Gustafsson