Feedback-ID: ie3de48e3:Fastmail
Date: Tue, 11 Nov 2025 10:17:41 +0100
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Chao Li <li.evan.chao@gmail.com>, Euler Taveira <euler@eulerto.com>,
	"kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_createsubscriber --dry-run logging concerns
Message-ID: <202511110904.ea4rsawszq65@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ibfr3ah5qy6tcbhw"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAHut+PvYQZ+XdMZwWf+V-Hz=3JjN-F-X5jUAcnJEBKP01msAjA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202511110904.ea4rsawszq65%40alvherre.pgsql>
Precedence: bulk


--ibfr3ah5qy6tcbhw
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2025-Nov-10, Peter Smith wrote:

> Hi Alvaro,
> 
> Here is patch v4-0001 modified as requested:
> - dashes are removed
> - the message is a single string

Okay, thanks.  I split the strings in two lines, as we customarily do
when they contain embedded newlines.  I also noticed pg_resetwal uses
stdout rather than stderr and set out to change it, because I don't
think it's sensible to have one program behave one way (print to stdout)
when all others behave in another (to stderr).  I wrote a commit message
and was about ready to push.

However, I then found out that the reason you used stdout instead of
stderr in pg_resetwal is that with the latter, tests fail all over the
place because of pg_resetwal -n being used for pg_upgrade internally via
popen(), and making it write to stderr results in confusing pg_upgrade
output as well as test failures.  A very simple fix for this problem
would be, of course, to add " 2>/dev/null" to the popen call, but that
is not only cheating, it is also dangerous: if pg_resetwal ever finds
reason to complain, we won't get very good information because of that
redirection.

(I also don't think this line belongs in stdout, in case you're thinking
of changing it in the other direction for all other programs.)

Maybe we should add a -q,--silent mode that suppresses the "Running in
dry-run mode" line.  I do wonder if this is getting too far into the
weeds for such a small thing.  I won't blame you if you want to just
drop this whole idea, but I also won't stop you if you want to introduce
--silent.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)

--ibfr3ah5qy6tcbhw
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v5-0001-Log-a-note-at-program-start-when-running-in-dry-r.patch"
Content-Transfer-Encoding: 8bit

From ebb8b62d2fa0c668f47b2e87b42587fd790a3263 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Tue, 11 Nov 2025 09:54:19 +0100
Subject: [PATCH v5] Log a note at program start when running in dry-run mode
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Users might get some peace of mind knowing their data is not being
destroyed or whatever.

Author: Peter Smith <smithpb2250@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CAHut+PsvQJQnQO0KT0S2oegenkvJ8FUuY-QS5syyqmT24R2xFQ@mail.gmail.com
---
 src/bin/pg_archivecleanup/pg_archivecleanup.c |  4 ++++
 src/bin/pg_basebackup/pg_createsubscriber.c   |  5 +++++
 src/bin/pg_combinebackup/pg_combinebackup.c   |  4 ++++
 src/bin/pg_resetwal/pg_resetwal.c             |  4 ++++
 src/bin/pg_rewind/pg_rewind.c                 | 10 ++++++----
 5 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index c25348bcb85..ab686b4748c 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -375,6 +375,10 @@ main(int argc, char **argv)
 		exit(2);
 	}
 
+	if (dryrun)
+		pg_log_info("Executing in dry-run mode.\n"
+					"No files will be removed.");
+
 	/*
 	 * Check archive exists and other initialization if required.
 	 */
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index df41836e70f..cc4be5d6ef4 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -2305,6 +2305,11 @@ main(int argc, char **argv)
 		pg_log_error_hint("Try \"%s --help\" for more information.", progname);
 		exit(1);
 	}
+
+	if (dry_run)
+		pg_log_info("Executing in dry-run mode.\n"
+					"The target directory will not be modified.");
+
 	pg_log_info("validating publisher connection string");
 	pub_base_conninfo = get_base_conninfo(opt.pub_conninfo_str,
 										  &dbname_conninfo);
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 3a325127209..c9bf0a9e105 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -242,6 +242,10 @@ main(int argc, char *argv[])
 	if (opt.no_manifest)
 		opt.manifest_checksums = CHECKSUM_TYPE_NONE;
 
+	if (opt.dry_run)
+		pg_log_info("Executing in dry-run mode.\n"
+					"The target directory will not be modified.");
+
 	/* Check that the platform supports the requested copy method. */
 	if (opt.copy_method == COPY_METHOD_CLONE)
 	{
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index a89d72fc5cf..dd5d279521e 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -397,6 +397,10 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
+	if (noupdate)
+		pg_log_info("Executing in dry-run mode.\n"
+					"Nothing will be modified.");
+
 	/*
 	 * Attempt to read the existing pg_control file
 	 */
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 27c514f934a..e9364d04f76 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -300,10 +300,12 @@ main(int argc, char **argv)
 
 	atexit(disconnect_atexit);
 
-	/*
-	 * Ok, we have all the options and we're ready to start. First, connect to
-	 * remote server.
-	 */
+	/* Ok, we have all the options and we're ready to start. */
+	if (dry_run)
+		pg_log_info("Executing in dry-run mode.\n"
+					"The target directory will not be modified.");
+
+	/* First, connect to remote server. */
 	if (connstr_source)
 	{
 		conn = PQconnectdb(connstr_source);
-- 
2.47.3


--ibfr3ah5qy6tcbhw--