pg_createsubscriber --dry-run logging concerns
Lately, I've been reviewing some pg_createsubscriber patches and have
been tricked by some of the logging.
The pg_createsubscriber has a '--dry-run' option/mode [1]https://www.postgresql.org/docs/current/app-pgcreatesubscriber.html
----------
--dry-run
Do everything except actually modifying the target directory.
----------
I've noticed that the logging in '--dry-run' mode is indistinguishable
from the logging of "normal" run, although functions like
create_publication(), drop_publication(), etc, are NOP in '--dry-run'
mode, because the actual command execution code is skipped. e.g.
if (!dry_run)
{
res = PQexec(conn, str->data);
...
}
~~~
IMO, it's not good to fool people into thinking something has happened
when in fact nothing happened at all. I think the logging of this tool
should be much more explicit wrt when it is/isn't in dry-run mode.
Perhaps like this:
NORMAL
pg_log_info("creating publication \"%s\" in database \"%s\"", ...)
DRY-RUN
pg_log_info("[dry-run] would create publication \"%s\" in database \"%s\"", ...)
~~~
Thoughts?
======
[1]: https://www.postgresql.org/docs/current/app-pgcreatesubscriber.html
Kind Regards,
Peter Smith.
Fujitsu Australia
Dear Peter,
IMO, it's not good to fool people into thinking something has happened
when in fact nothing happened at all. I think the logging of this tool
should be much more explicit wrt when it is/isn't in dry-run mode.
Perhaps like this:NORMAL
pg_log_info("creating publication \"%s\" in database \"%s\"", ...)DRY-RUN
pg_log_info("[dry-run] would create publication \"%s\" in database \"%s\"", ...)
Per my understanding, almost all the output must be adjusted based on the mode, right?
I feel it introduces a burden.
Can we solve the issue if we print additional message at the beginning if the
command runs with dry-run mode?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
On Wed, Oct 1, 2025 at 1:09 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
Dear Peter,
IMO, it's not good to fool people into thinking something has happened
when in fact nothing happened at all. I think the logging of this tool
should be much more explicit wrt when it is/isn't in dry-run mode.
Perhaps like this:NORMAL
pg_log_info("creating publication \"%s\" in database \"%s\"", ...)DRY-RUN
pg_log_info("[dry-run] would create publication \"%s\" in database \"%s\"", ...)Per my understanding, almost all the output must be adjusted based on the mode, right?
I feel it introduces a burden.
Can we solve the issue if we print additional message at the beginning if the
command runs with dry-run mode?
Hi Kuroda-san,
Yes, that is one way. Something is better than nothing, at least...
I think that not *everything* in dry mode is fake; some of the logged
operations are real. So, it might be good if we can show fake ones
differently. e.g. It may not take much effort to introduce a wrapper
that inserts a prefix. Use this as a replacement for just a few
specific info logs.
(code below may not work; it's just for illustrative purposes)
#define pg_log_info_checkdry(...) do {\
if (dry_run)\
pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, "[dry-run NOP]" __VA_ARGS__);\
else;\
pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__);\
} while (0);
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On 2025-Oct-01, Peter Smith wrote:
(code below may not work; it's just for illustrative purposes)
#define pg_log_info_checkdry(...) do {\
if (dry_run)\
pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, "[dry-run NOP]" __VA_ARGS__);\
else;\
pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__);\
} while (0);
I like this kind of idea best. However I think it might be better to do
it the other way around: have the normal pg_log_info() check dry_run,
and have a special one for the messages that are to be identical in
either mode. I'm not sure how difficult this is to implement, though.
pg_subscriber is not the only program with a dry-run mode; it looks like
pg_archiveclean, pg_combinebackup, pg_resetwal, pg_rewind have one. Is
it worth maybe doing something at the common/logging.c level rather than
specifically pg_createsubscriber?
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
[…] indem ich in meinem Leben oft an euch gedacht, euch glücklich zu machen. Seyd es!
A menudo he pensado en vosotros, en haceros felices. ¡Sedlo, pues!
Heiligenstädter Testament, L. v. Beethoven, 1802
https://de.wikisource.org/wiki/Heiligenstädter_Testament
On Wed, Oct 1, 2025 at 8:37 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Oct-01, Peter Smith wrote:
(code below may not work; it's just for illustrative purposes)
#define pg_log_info_checkdry(...) do {\
if (dry_run)\
pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, "[dry-run NOP]" __VA_ARGS__);\
else;\
pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__);\
} while (0);I like this kind of idea best. However I think it might be better to do
it the other way around: have the normal pg_log_info() check dry_run,
and have a special one for the messages that are to be identical in
either mode. I'm not sure how difficult this is to implement, though.pg_subscriber is not the only program with a dry-run mode; it looks like
pg_archiveclean, pg_combinebackup, pg_resetwal, pg_rewind have one. Is
it worth maybe doing something at the common/logging.c level rather than
specifically pg_createsubscriber?
Hi Alvaro.
Thanks for the feedback and for pointing out the other tools that also
have a dr-run mode.
I did some quick back-of-the-envelope calculations to see what might
be involved.
======
For pg_create_subscriber, there are 38 info logs. Of those 38, I'd be
tempted to change only logs that say they are modifying/executing
something. There's ~15 of those:
pg_log_info("modifying system identifier of subscriber");
pg_log_info("running pg_resetwal on the subscriber");
pg_log_info("subscriber successfully changed the system identifier");
pg_log_info("use existing publication \"%s\" in database \"%s\"",
pg_log_info("create publication \"%s\" in database \"%s\"",
pg_log_info("create replication slot \"%s\" on publisher",
pg_log_info("dropping subscription \"%s\" in database \"%s\"",
pg_log_info("creating the replication slot \"%s\" in database \"%s\"",
pg_log_info("dropping the replication slot \"%s\" in database \"%s\"",
pg_log_info("creating publication \"%s\" in database \"%s\"",
pg_log_info("dropping publication \"%s\" in database \"%s\"",
pg_log_info("dropping all existing publications in database \"%s\"",
pg_log_info("creating subscription \"%s\" in database \"%s\"",
pg_log_info("setting the replication progress (node name \"%s\", LSN
%s) in database \"%s\"",
pg_log_info("enabling subscription \"%s\" in database \"%s\"",
======
For pg_archiveclean:
There only seems to be one logging for this dry run. Currently, it is
using debug logging like:
if (dry_run)
pg_log_debug("would do XXX")
else
pg_log_debug("doing XXX");
======
For pg_combinebackup:
This is the same as above; the differences for dry run logging are
already handled by having separate messages, and it uses debug logging
in many places with this pattern:
if (dry_run)
pg_log_debug("would do XXX")
else
pg_log_debug("doing XXX");
======
For pg_resetwal:
This one is using a 'noupdate' flag instead of a 'dry_run' boolean.
And there doesn't seem to be any logging for this.
======
For pg_rewind:
Has 13 logs. I'd be tempted to change maybe only a couple of these:
pg_rewind/pg_rewind.c: pg_log_info("creating backup label and updating
control file");
pg_rewind/pg_rewind.c: pg_log_info("executing \"%s\" for target server
to complete crash recovery",
======
I also found dry-run logs using the "would do XXX" pattern elsewhere,
in code like contrib/vacuumIo.c
//////
Summary
The idea to change the pg_log_info macro globally seems risky. There
are 400+ usages of this in the PG code, way beyond the scope of these
few tools that have a dry-run.
It seems that all the other --dry-run capable tools are using the pattern
if (dry_run)
pg_log_debug("would do XXX")
else
pg_log_debug("doing XXX");
So, that makes pg_createsubscriber the odd man out. Instead of
introducing a new logging macro, perhaps it's better (for code
consistency) just to change pg_createsubscriber to use that same
logging pattern.
~~~
Kurdoa-san [1]/messages/by-id/OSCPR01MB149668DD062C1457FFAA44A28F5E6A@OSCPR01MB14966.jpnprd01.prod.outlook.com was concerned it might be a big change burden to change
the pg_createsubscriber logs, but AFAICT there are only ~15/38 places
that I'd be tempted to change in pg_createsubscriber.c; that's not
really such a burden.
OTOH, I do think Kuroda-san's idea [1]/messages/by-id/OSCPR01MB149668DD062C1457FFAA44A28F5E6A@OSCPR01MB14966.jpnprd01.prod.outlook.com of giving some up-front logging
to indicate --dry-run might be useful. Even when run-time gives
different log messages, I think those other tools only show
differences when using DEBUG logging. Anybody not debugging might
otherwise have no idea that nothing actually happened.
~~
So, below is now my favoured solution:
1. Add an up-front info log to say when running in dry-run (add for
all tools that have --dry-run mode)
2. Modify ~15 places in pg_createsubscriber.c to use the code pattern
consistent with all the other tools.
if (dry_run)
pg_log_info("would do XXX")
else
pg_log_info("doing XXX");
Thoughts?
======
[1]: /messages/by-id/OSCPR01MB149668DD062C1457FFAA44A28F5E6A@OSCPR01MB14966.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Hello Peter,
Your suggestion makes sense to me, if it's fine with you then I can submit
a patch for this change :).
[
So, below is now my favoured solution:
1. Add an up-front info log to say when running in dry-run (add for
all tools that have --dry-run mode)
2. Modify ~15 places in pg_createsubscriber.c to use the code pattern
consistent with all the other tools.
if (dry_run)
pg_log_info("would do XXX")
else
pg_log_info("doing XXX");
]
Thanks,
Neeraj Shah
Nutanix Inc.
On Fri, Oct 3, 2025 at 5:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
Show quoted text
On Wed, Oct 1, 2025 at 8:37 PM Álvaro Herrera <alvherre@kurilemu.de>
wrote:On 2025-Oct-01, Peter Smith wrote:
(code below may not work; it's just for illustrative purposes)
#define pg_log_info_checkdry(...) do {\
if (dry_run)\
pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, "[dry-run NOP]"__VA_ARGS__);\
else;\
pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__);\
} while (0);I like this kind of idea best. However I think it might be better to do
it the other way around: have the normal pg_log_info() check dry_run,
and have a special one for the messages that are to be identical in
either mode. I'm not sure how difficult this is to implement, though.pg_subscriber is not the only program with a dry-run mode; it looks like
pg_archiveclean, pg_combinebackup, pg_resetwal, pg_rewind have one. Is
it worth maybe doing something at the common/logging.c level rather than
specifically pg_createsubscriber?Hi Alvaro.
Thanks for the feedback and for pointing out the other tools that also
have a dr-run mode.I did some quick back-of-the-envelope calculations to see what might
be involved.======
For pg_create_subscriber, there are 38 info logs. Of those 38, I'd be
tempted to change only logs that say they are modifying/executing
something. There's ~15 of those:pg_log_info("modifying system identifier of subscriber");
pg_log_info("running pg_resetwal on the subscriber");
pg_log_info("subscriber successfully changed the system identifier");
pg_log_info("use existing publication \"%s\" in database \"%s\"",
pg_log_info("create publication \"%s\" in database \"%s\"",
pg_log_info("create replication slot \"%s\" on publisher",
pg_log_info("dropping subscription \"%s\" in database \"%s\"",
pg_log_info("creating the replication slot \"%s\" in database \"%s\"",
pg_log_info("dropping the replication slot \"%s\" in database \"%s\"",
pg_log_info("creating publication \"%s\" in database \"%s\"",
pg_log_info("dropping publication \"%s\" in database \"%s\"",
pg_log_info("dropping all existing publications in database \"%s\"",
pg_log_info("creating subscription \"%s\" in database \"%s\"",
pg_log_info("setting the replication progress (node name \"%s\", LSN
%s) in database \"%s\"",
pg_log_info("enabling subscription \"%s\" in database \"%s\"",======
For pg_archiveclean:
There only seems to be one logging for this dry run. Currently, it is
using debug logging like:if (dry_run)
pg_log_debug("would do XXX")
else
pg_log_debug("doing XXX");======
For pg_combinebackup:
This is the same as above; the differences for dry run logging are
already handled by having separate messages, and it uses debug logging
in many places with this pattern:if (dry_run)
pg_log_debug("would do XXX")
else
pg_log_debug("doing XXX");
======For pg_resetwal:
This one is using a 'noupdate' flag instead of a 'dry_run' boolean.
And there doesn't seem to be any logging for this.======
For pg_rewind:
Has 13 logs. I'd be tempted to change maybe only a couple of these:
pg_rewind/pg_rewind.c: pg_log_info("creating backup label and updating
control file");
pg_rewind/pg_rewind.c: pg_log_info("executing \"%s\" for target server
to complete crash recovery",======
I also found dry-run logs using the "would do XXX" pattern elsewhere,
in code like contrib/vacuumIo.c//////
Summary
The idea to change the pg_log_info macro globally seems risky. There
are 400+ usages of this in the PG code, way beyond the scope of these
few tools that have a dry-run.It seems that all the other --dry-run capable tools are using the pattern
if (dry_run)
pg_log_debug("would do XXX")
else
pg_log_debug("doing XXX");So, that makes pg_createsubscriber the odd man out. Instead of
introducing a new logging macro, perhaps it's better (for code
consistency) just to change pg_createsubscriber to use that same
logging pattern.~~~
Kurdoa-san [1] was concerned it might be a big change burden to change
the pg_createsubscriber logs, but AFAICT there are only ~15/38 places
that I'd be tempted to change in pg_createsubscriber.c; that's not
really such a burden.OTOH, I do think Kuroda-san's idea [1] of giving some up-front logging
to indicate --dry-run might be useful. Even when run-time gives
different log messages, I think those other tools only show
differences when using DEBUG logging. Anybody not debugging might
otherwise have no idea that nothing actually happened.~~
So, below is now my favoured solution:
1. Add an up-front info log to say when running in dry-run (add for
all tools that have --dry-run mode)2. Modify ~15 places in pg_createsubscriber.c to use the code pattern
consistent with all the other tools.
if (dry_run)
pg_log_info("would do XXX")
else
pg_log_info("doing XXX");Thoughts?
======
[1]
/messages/by-id/OSCPR01MB149668DD062C1457FFAA44A28F5E6A@OSCPR01MB14966.jpnprd01.prod.outlook.comKind Regards,
Peter Smith.
Fujitsu Australia
On 2025-Oct-03, Peter Smith wrote:
Summary
The idea to change the pg_log_info macro globally seems risky. There
are 400+ usages of this in the PG code, way beyond the scope of these
few tools that have a dry-run.
Ok.
So, that makes pg_createsubscriber the odd man out. Instead of
introducing a new logging macro, perhaps it's better (for code
consistency) just to change pg_createsubscriber to use that same
logging pattern.
Sure, let's go that way.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Thu, Oct 2, 2025, at 9:04 PM, Peter Smith wrote:
Summary
The idea to change the pg_log_info macro globally seems risky. There
are 400+ usages of this in the PG code, way beyond the scope of these
few tools that have a dry-run.It seems that all the other --dry-run capable tools are using the pattern
if (dry_run)
pg_log_debug("would do XXX")
else
pg_log_debug("doing XXX");So, that makes pg_createsubscriber the odd man out. Instead of
introducing a new logging macro, perhaps it's better (for code
consistency) just to change pg_createsubscriber to use that same
logging pattern.
What do you mean by "use the same logging pattern"? During development we
discussed if it is worth to double the number of messages to have accurate log
messages in dry run mode but decided it isn't.
I didn't understand all details of your proposal but I would like to say that
I'm against changing the 2 level log messages. Sometimes we just want a list of
the steps with the exact object names (hence, --verbose) instead of a bunch of
additional debug messages that exposes the implementation details (hence,
--verbose --verbose).
Kurdoa-san [1] was concerned it might be a big change burden to change
the pg_createsubscriber logs, but AFAICT there are only ~15/38 places
that I'd be tempted to change in pg_createsubscriber.c; that's not
really such a burden.OTOH, I do think Kuroda-san's idea [1] of giving some up-front logging
to indicate --dry-run might be useful. Even when run-time gives
different log messages, I think those other tools only show
differences when using DEBUG logging. Anybody not debugging might
otherwise have no idea that nothing actually happened.
I agree that it seems confusing if you are not the one that wrote the command
line. Maybe it would be less confusing if we have a log message at the
beginning saying "pg_createsubscriber is in dry run mode".
So, below is now my favoured solution:
1. Add an up-front info log to say when running in dry-run (add for
all tools that have --dry-run mode)2. Modify ~15 places in pg_createsubscriber.c to use the code pattern
consistent with all the other tools.
if (dry_run)
pg_log_info("would do XXX")
else
pg_log_info("doing XXX");
I particularly think a prefix increases the translation effort. As Alvaro said
if you want to propose a prefix feature, it should cover the other tools that
use the logging module too.
Since you are improving messages, I suggest 2 changes:
pg_createsubscriber: creating the replication slot "pg_createsubscriber_16384_710455e1" in database "bench1"
pg_createsubscriber: create replication slot "pg_createsubscriber_16384_710455e1" on publisher
to
pg_createsubscriber: creating the replication slot "pg_createsubscriber_16384_710455e1" in database "bench1"
because it is duplicated.
Don't refer to the database name for physical replication slot
pg_createsubscriber: dropping the replication slot "primaryslot" in database "bench1"
Maybe replace "in database foo" with "on primary".
--
Euler Taveira
EDB https://www.enterprisedb.com/
Hi Euler,
On Tue, Oct 7, 2025 at 7:06 AM Euler Taveira <euler@eulerto.com> wrote:
On Thu, Oct 2, 2025, at 9:04 PM, Peter Smith wrote:
Summary
The idea to change the pg_log_info macro globally seems risky. There
are 400+ usages of this in the PG code, way beyond the scope of these
few tools that have a dry-run.It seems that all the other --dry-run capable tools are using the pattern
if (dry_run)
pg_log_debug("would do XXX")
else
pg_log_debug("doing XXX");So, that makes pg_createsubscriber the odd man out. Instead of
introducing a new logging macro, perhaps it's better (for code
consistency) just to change pg_createsubscriber to use that same
logging pattern.What do you mean by "use the same logging pattern"? During development we
discussed if it is worth to double the number of messages to have accurate log
messages in dry run mode but decided it isn't.
Yes, one part I proposed is to do exactly that. i.e. double-up on some
messages (about a dozen of them), because that is what the other tools
that have '--dry-run' mode are doing. They have messages like:
"creating xxx" and related one for --dry-run that says "would create xxx"
So I was proposing to do the same, consistent code pattern with the other tools.
I didn't understand all details of your proposal but I would like to say that
I'm against changing the 2 level log messages. Sometimes we just want a list of
the steps with the exact object names (hence, --verbose) instead of a bunch of
additional debug messages that exposes the implementation details (hence,
--verbose --verbose).
Yeah, I understand that the other places (like pg_combinebackup.c) are
using pg_log_debug instead of pg_log_info, so perhaps your point is
that although it was OK to do this in debug, you think doing the same
for pg_log_info is a bridge too far?
I am not wedded to doing this double-messaging... if people feel just
the one-time logging at the beginning is enough, then that is OK by
me.
Kurdoa-san [1] was concerned it might be a big change burden to change
the pg_createsubscriber logs, but AFAICT there are only ~15/38 places
that I'd be tempted to change in pg_createsubscriber.c; that's not
really such a burden.OTOH, I do think Kuroda-san's idea [1] of giving some up-front logging
to indicate --dry-run might be useful. Even when run-time gives
different log messages, I think those other tools only show
differences when using DEBUG logging. Anybody not debugging might
otherwise have no idea that nothing actually happened.I agree that it seems confusing if you are not the one that wrote the command
line. Maybe it would be less confusing if we have a log message at the
beginning saying "pg_createsubscriber is in dry run mode".
Yes. So, please see the attached patch that implements this. And for
consistency, the change is repeated to all other tools that use
--dry-run mode.
So, below is now my favoured solution:
1. Add an up-front info log to say when running in dry-run (add for
all tools that have --dry-run mode)2. Modify ~15 places in pg_createsubscriber.c to use the code pattern
consistent with all the other tools.
if (dry_run)
pg_log_info("would do XXX")
else
pg_log_info("doing XXX");I particularly think a prefix increases the translation effort. As Alvaro said
if you want to propose a prefix feature, it should cover the other tools that
use the logging module too.
There are no plans to have a prefix feature. That was an initial
thought bubble, but after I saw how all the other tools that have
--dry-run just have pairs of logging, I dropped the prefix idea.
Since you are improving messages, I suggest 2 changes:
pg_createsubscriber: creating the replication slot "pg_createsubscriber_16384_710455e1" in database "bench1"
pg_createsubscriber: create replication slot "pg_createsubscriber_16384_710455e1" on publisherto
pg_createsubscriber: creating the replication slot "pg_createsubscriber_16384_710455e1" in database "bench1"
because it is duplicated.
Don't refer to the database name for physical replication slot
pg_createsubscriber: dropping the replication slot "primaryslot" in database "bench1"
Maybe replace "in database foo" with "on primary".
Actually. I had already created another thread/patch about that
duplicate logging issue [1]/messages/by-id/CAHut+Pv7qDvLbDgc9PQGhULT3rPXTxdu_=w+iW-kMs+zPADR+w@mail.gmail.com.
As for all the other suggestions, I'd rather keep this thread scope
focused on the '--dry-run' logging issue, and not conflate it with all
those other logging problems, which probably all deserve their own
threads/patches.
======
[1]: /messages/by-id/CAHut+Pv7qDvLbDgc9PQGhULT3rPXTxdu_=w+iW-kMs+zPADR+w@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-log-to-say-command-is-executing-in-dry-run-mode.patchapplication/octet-stream; name=v1-0001-log-to-say-command-is-executing-in-dry-run-mode.patchDownload
From 2af6b4ad0f98769bed37e753cca23941839f3077 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Wed, 8 Oct 2025 19:49:53 +1100
Subject: [PATCH v1] log to say command is executing in dry run mode
---
src/bin/pg_archivecleanup/pg_archivecleanup.c | 8 ++++++++
src/bin/pg_basebackup/pg_createsubscriber.c | 9 +++++++++
src/bin/pg_combinebackup/pg_combinebackup.c | 8 ++++++++
src/bin/pg_resetwal/pg_resetwal.c | 8 ++++++++
src/bin/pg_resetwal/t/001_basic.pl | 2 +-
src/bin/pg_rewind/pg_rewind.c | 14 ++++++++++++--
6 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index c25348b..dba1e74 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -375,6 +375,14 @@ main(int argc, char **argv)
exit(2);
}
+ if (dryrun)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("pg_archivecleanup is executing in '--dry-run' mode.");
+ pg_log_info("No files will be removed.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
/*
* 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 3986882..2e8972e 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -2273,6 +2273,15 @@ main(int argc, char **argv)
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
exit(1);
}
+
+ if (dry_run)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("pg_createsubscriber is executing in '--dry-run' mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
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 f5cef99..7cbadb6 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -241,6 +241,14 @@ main(int argc, char *argv[])
if (opt.no_manifest)
opt.manifest_checksums = CHECKSUM_TYPE_NONE;
+ if (opt.dry_run)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("pg_combinebackup is executing in '--dry-run' mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
/* 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 7a4e4eb..83c22c1 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -396,6 +396,14 @@ main(int argc, char *argv[])
exit(1);
}
+ if (noupdate)
+ {
+ printf(_("-----------------------------------------------------"));
+ printf(_("pg_resetwal is executing in '--dry-run' mode."));
+ printf(_("Nothing will be modified."));
+ printf(_("-----------------------------------------------------"));
+ }
+
/*
* Attempt to read the existing pg_control file
*/
diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl
index d6bbbd0..247bd93 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -17,7 +17,7 @@ $node->init;
$node->append_conf('postgresql.conf', 'track_commit_timestamp = on');
command_like([ 'pg_resetwal', '-n', $node->data_dir ],
- qr/checkpoint/, 'pg_resetwal -n produces output');
+ qr/checkpoint/m, 'pg_resetwal -n produces output');
# Permissions on PGDATA should be default
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0c68dd4..b5f4c6a 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -300,8 +300,18 @@ 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("-----------------------------------------------------");
+ pg_log_info("pg_rewind is executing in '--dry-run' mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
+ /*
+ * First, connect to remote server.
*/
if (connstr_source)
{
--
1.8.3.1
On 2025-Oct-08, Peter Smith wrote:
I am not wedded to doing this double-messaging... if people feel just
the one-time logging at the beginning is enough, then that is OK by
me.
Some other tools (not Postgres ones) do that and it always makes me
nervous, because I can never be sure which parts were actually done and
which parts are only dry-run trials. I'd rather stay away from that
approach.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)
On Sat, Oct 4, 2025 at 3:50 AM Neeraj Shah <n4j@liamg.xyz> wrote:
Hello Peter,
Your suggestion makes sense to me, if it's fine with you then I can submit a patch for this change :).
Thanks, but I already had some local patches ready to go.... I was
just waiting for more feedback before posting them.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Oct 8, 2025 at 9:04 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Oct-08, Peter Smith wrote:
I am not wedded to doing this double-messaging... if people feel just
the one-time logging at the beginning is enough, then that is OK by
me.Some other tools (not Postgres ones) do that and it always makes me
nervous, because I can never be sure which parts were actually done and
which parts are only dry-run trials. I'd rather stay away from that
approach.
OK. Here are v2 patches to implement both ways. You can pick one or both.
0001 - a beginning log to say if the tool is in dry-run mode
0002 - alternate pg_log_info messages for pg_createsubscriber, when in
dry-run mode
~
Note: there might be a clash with another thread [1]/messages/by-id/CAHut+Pv7qDvLbDgc9PQGhULT3rPXTxdu_=w+iW-kMs+zPADR+w@mail.gmail.com if that gets
committed soon; I will deal with any rebase if/when it is needed.
======
[1]: /messages/by-id/CAHut+Pv7qDvLbDgc9PQGhULT3rPXTxdu_=w+iW-kMs+zPADR+w@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v2-0001-log-to-say-command-is-executing-in-dry-run-mode.patchapplication/octet-stream; name=v2-0001-log-to-say-command-is-executing-in-dry-run-mode.patchDownload
From 2af6b4ad0f98769bed37e753cca23941839f3077 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Wed, 8 Oct 2025 19:49:53 +1100
Subject: [PATCH v2] log to say command is executing in dry run mode
---
src/bin/pg_archivecleanup/pg_archivecleanup.c | 8 ++++++++
src/bin/pg_basebackup/pg_createsubscriber.c | 9 +++++++++
src/bin/pg_combinebackup/pg_combinebackup.c | 8 ++++++++
src/bin/pg_resetwal/pg_resetwal.c | 8 ++++++++
src/bin/pg_resetwal/t/001_basic.pl | 2 +-
src/bin/pg_rewind/pg_rewind.c | 14 ++++++++++++--
6 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index c25348b..dba1e74 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -375,6 +375,14 @@ main(int argc, char **argv)
exit(2);
}
+ if (dryrun)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("pg_archivecleanup is executing in '--dry-run' mode.");
+ pg_log_info("No files will be removed.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
/*
* 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 3986882..2e8972e 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -2273,6 +2273,15 @@ main(int argc, char **argv)
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
exit(1);
}
+
+ if (dry_run)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("pg_createsubscriber is executing in '--dry-run' mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
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 f5cef99..7cbadb6 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -241,6 +241,14 @@ main(int argc, char *argv[])
if (opt.no_manifest)
opt.manifest_checksums = CHECKSUM_TYPE_NONE;
+ if (opt.dry_run)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("pg_combinebackup is executing in '--dry-run' mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
/* 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 7a4e4eb..83c22c1 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -396,6 +396,14 @@ main(int argc, char *argv[])
exit(1);
}
+ if (noupdate)
+ {
+ printf(_("-----------------------------------------------------"));
+ printf(_("pg_resetwal is executing in '--dry-run' mode."));
+ printf(_("Nothing will be modified."));
+ printf(_("-----------------------------------------------------"));
+ }
+
/*
* Attempt to read the existing pg_control file
*/
diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl
index d6bbbd0..247bd93 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -17,7 +17,7 @@ $node->init;
$node->append_conf('postgresql.conf', 'track_commit_timestamp = on');
command_like([ 'pg_resetwal', '-n', $node->data_dir ],
- qr/checkpoint/, 'pg_resetwal -n produces output');
+ qr/checkpoint/m, 'pg_resetwal -n produces output');
# Permissions on PGDATA should be default
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0c68dd4..b5f4c6a 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -300,8 +300,18 @@ 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("-----------------------------------------------------");
+ pg_log_info("pg_rewind is executing in '--dry-run' mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
+ /*
+ * First, connect to remote server.
*/
if (connstr_source)
{
--
1.8.3.1
v2-0002-add-different-dry-run-logging-for-pg_createsubscr.patchapplication/octet-stream; name=v2-0002-add-different-dry-run-logging-for-pg_createsubscr.patchDownload
From 3329e2269ba9a336f17a8a6ad9caf3a57515fdb7 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 9 Oct 2025 11:40:44 +1100
Subject: [PATCH v2] add different dry-run logging for pg_createsubscriber
---
src/bin/pg_basebackup/pg_createsubscriber.c | 89 +++++++++++++++-------
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl | 6 +-
2 files changed, 65 insertions(+), 30 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 2e8972e..b23c1cd 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -679,13 +679,20 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
cf->system_identifier |= ((uint64) tv.tv_usec) << 12;
cf->system_identifier |= getpid() & 0xFFF;
- if (!dry_run)
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise system identifier would be %" PRIu64 " on subscriber",
+ cf->system_identifier);
+ else
+ {
update_controlfile(subscriber_dir, cf, true);
+ pg_log_info("system identifier is %" PRIu64 " on subscriber",
+ cf->system_identifier);
+ }
- pg_log_info("system identifier is %" PRIu64 " on subscriber",
- cf->system_identifier);
-
- pg_log_info("running pg_resetwal on the subscriber");
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise would run pg_resetwal on the subscriber");
+ else
+ pg_log_info("running pg_resetwal on the subscriber");
cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path,
subscriber_dir, DEVNULL);
@@ -801,10 +808,7 @@ setup_publisher(struct LogicalRepInfo *dbinfo)
if (lsn)
pg_free(lsn);
lsn = create_logical_replication_slot(conn, &dbinfo[i]);
- if (lsn != NULL || dry_run)
- pg_log_info("create replication slot \"%s\" on publisher",
- dbinfo[i].replslotname);
- else
+ if (lsn == NULL && !dry_run)
exit(1);
/*
@@ -1124,11 +1128,14 @@ drop_existing_subscriptions(PGconn *conn, const char *subname, const char *dbnam
subname);
appendPQExpBuffer(query, " DROP SUBSCRIPTION %s;", subname);
- pg_log_info("dropping subscription \"%s\" in database \"%s\"",
- subname, dbname);
-
- if (!dry_run)
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise would drop subscription \"%s\" in database \"%s\"",
+ subname, dbname);
+ else
{
+ pg_log_info("dropping subscription \"%s\" in database \"%s\"",
+ subname, dbname);
+
res = PQexec(conn, query->data);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
@@ -1375,8 +1382,12 @@ create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo)
Assert(conn != NULL);
- pg_log_info("creating the replication slot \"%s\" in database \"%s\"",
- slot_name, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise would create the replication slot \"%s\" in database \"%s\"",
+ slot_name, dbinfo->dbname);
+ else
+ pg_log_info("creating the replication slot \"%s\" in database \"%s\"",
+ slot_name, dbinfo->dbname);
slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
@@ -1424,8 +1435,12 @@ drop_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo,
Assert(conn != NULL);
- pg_log_info("dropping the replication slot \"%s\" in database \"%s\"",
- slot_name, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise would drop the replication slot \"%s\" in database \"%s\"",
+ slot_name, dbinfo->dbname);
+ else
+ pg_log_info("dropping the replication slot \"%s\" in database \"%s\"",
+ slot_name, dbinfo->dbname);
slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
@@ -1651,8 +1666,12 @@ create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
PQclear(res);
resetPQExpBuffer(str);
- pg_log_info("creating publication \"%s\" in database \"%s\"",
- dbinfo->pubname, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise would create publication \"%s\" in database \"%s\"",
+ dbinfo->pubname, dbinfo->dbname);
+ else
+ pg_log_info("creating publication \"%s\" in database \"%s\"",
+ dbinfo->pubname, dbinfo->dbname);
appendPQExpBuffer(str, "CREATE PUBLICATION %s FOR ALL TABLES",
ipubname_esc);
@@ -1694,8 +1713,12 @@ drop_publication(PGconn *conn, const char *pubname, const char *dbname,
pubname_esc = PQescapeIdentifier(conn, pubname, strlen(pubname));
- pg_log_info("dropping publication \"%s\" in database \"%s\"",
- pubname, dbname);
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise would drop publication \"%s\" in database \"%s\"",
+ pubname, dbname);
+ else
+ pg_log_info("dropping publication \"%s\" in database \"%s\"",
+ pubname, dbname);
appendPQExpBuffer(str, "DROP PUBLICATION %s", pubname_esc);
@@ -1803,8 +1826,12 @@ create_subscription(PGconn *conn, const struct LogicalRepInfo *dbinfo)
pubconninfo_esc = PQescapeLiteral(conn, dbinfo->pubconninfo, strlen(dbinfo->pubconninfo));
replslotname_esc = PQescapeLiteral(conn, dbinfo->replslotname, strlen(dbinfo->replslotname));
- pg_log_info("creating subscription \"%s\" in database \"%s\"",
- dbinfo->subname, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise would create subscription \"%s\" in database \"%s\"",
+ dbinfo->subname, dbinfo->dbname);
+ else
+ pg_log_info("creating subscription \"%s\" in database \"%s\"",
+ dbinfo->subname, dbinfo->dbname);
appendPQExpBuffer(str,
"CREATE SUBSCRIPTION %s CONNECTION %s PUBLICATION %s "
@@ -1901,8 +1928,12 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
*/
originname = psprintf("pg_%u", suboid);
- pg_log_info("setting the replication progress (node name \"%s\", LSN %s) in database \"%s\"",
- originname, lsnstr, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise, would set the replication progress (node name \"%s\", LSN %s) in database \"%s\"",
+ originname, lsnstr, dbinfo->dbname);
+ else
+ pg_log_info("setting the replication progress (node name \"%s\", LSN %s) in database \"%s\"",
+ originname, lsnstr, dbinfo->dbname);
resetPQExpBuffer(str);
appendPQExpBuffer(str,
@@ -1947,8 +1978,12 @@ enable_subscription(PGconn *conn, const struct LogicalRepInfo *dbinfo)
subname = PQescapeIdentifier(conn, dbinfo->subname, strlen(dbinfo->subname));
- pg_log_info("enabling subscription \"%s\" in database \"%s\"",
- dbinfo->subname, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise would enable subscription \"%s\" in database \"%s\"",
+ dbinfo->subname, dbinfo->dbname);
+ else
+ pg_log_info("enabling subscription \"%s\" in database \"%s\"",
+ dbinfo->subname, dbinfo->dbname);
appendPQExpBuffer(str, "ALTER SUBSCRIPTION %s ENABLE", subname);
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 229fef5..60a8f8c 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -436,11 +436,11 @@ my ($stdout, $stderr) = run_command(
# Verify that the required logical replication objects are output.
# The expected count 3 refers to postgres, $db1 and $db2 databases.
-is(scalar(() = $stderr =~ /creating publication/g),
+is(scalar(() = $stderr =~ /would create publication/g),
3, "verify publications are created for all databases");
-is(scalar(() = $stderr =~ /creating the replication slot/g),
+is(scalar(() = $stderr =~ /would create the replication slot/g),
3, "verify replication slots are created for all databases");
-is(scalar(() = $stderr =~ /creating subscription/g),
+is(scalar(() = $stderr =~ /would create subscription/g),
3, "verify subscriptions are created for all databases");
# Run pg_createsubscriber on node S. --verbose is used twice
--
1.8.3.1
I think is patch is helpful. A few comments:
On Oct 9, 2025, at 08:55, Peter Smith <smithpb2250@gmail.com> wrote:
<v2-0001-log-to-say-command-is-executing-in-dry-run-mode.patch><v2-0002-add-different-dry-run-logging-for-pg_createsubscr.patch>
1 - 0001
```
+ if (dryrun)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("pg_archivecleanup is executing in '--dry-run' mode.");
+ pg_log_info("No files will be removed.");
+ pg_log_info("-----------------------------------------------------");
+ }
```Putting the program name in log message feels redundant, because pg_log_info() may already prefixes logs with program name. But I like the separator lines that make it stand out visually in logs. So this log can be simplified as:
```
if (dryrun)
{
pg_log_info("------------------------------------------------------------");
pg_log_info("Running in dry-run mode; no files will be removed.");
pg_log_info("------------------------------------------------------------");
}
```
This comment applies to rest of changes in 0001.
2 - 0002
```
- if (!dry_run)
+ if (dry_run)
+ pg_log_info("in dry-run mode, otherwise system identifier would be %" PRIu64 " on subscriber",
+ cf->system_identifier);
```I think this log message can be simplified as:
```
pg_log_info("dry-run: system identifier would be %" PRIu64 " on subscriber",
cf->system_identifier);
```
As a general comment, I think “in dry-run mode” is a little long-winded, we can just put “dry-run:”, and “otherwise” seems not needed because “dry-run” has clearly indicated nothing would actually happen. This comment applies to all changes in pg_createsubscriber.c of 0002.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Thu, Oct 9, 2025 at 4:38 PM Chao Li <li.evan.chao@gmail.com> wrote:
I think is patch is helpful. A few comments:
On Oct 9, 2025, at 08:55, Peter Smith <smithpb2250@gmail.com> wrote:
<v2-0001-log-to-say-command-is-executing-in-dry-run-mode.patch><v2-0002-add-different-dry-run-logging-for-pg_createsubscr.patch>
1 - 0001 ``` + if (dryrun) + { + pg_log_info("-----------------------------------------------------"); + pg_log_info("pg_archivecleanup is executing in '--dry-run' mode."); + pg_log_info("No files will be removed."); + pg_log_info("-----------------------------------------------------"); + } ```Putting the program name in log message feels redundant, because pg_log_info() may already prefixes logs with program name. But I like the separator lines that make it stand out visually in logs. So this log can be simplified as:
```
Fair point. I removed the tool name from the message.
2 - 0002 ``` - if (!dry_run) + if (dry_run) + pg_log_info("in dry-run mode, otherwise system identifier would be %" PRIu64 " on subscriber", + cf->system_identifier); ```I think this log message can be simplified as:
```
pg_log_info("dry-run: system identifier would be %" PRIu64 " on subscriber",
cf->system_identifier);
```As a general comment, I think “in dry-run mode” is a little long-winded, we can just put “dry-run:”, and “otherwise” seems not needed because “dry-run” has clearly indicated nothing would actually happen. This comment applies to all changes in pg_createsubscriber.c of 0002.
OK. Updated as suggested.
~~
The anticipated rebase was necessary, too.
Please see the v3 patches.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v3-0002-add-different-dry-run-logging-for-pg_createsubscr.patchapplication/octet-stream; name=v3-0002-add-different-dry-run-logging-for-pg_createsubscr.patchDownload
From 9af17bcae9a749914fccc0a05befd25e75d2fa37 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 9 Oct 2025 18:58:09 +1100
Subject: [PATCH v3] add different dry-run logging for pg_createsubscriber
---
src/bin/pg_basebackup/pg_createsubscriber.c | 84 ++++++++++++++++------
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl | 6 +-
2 files changed, 64 insertions(+), 26 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 48df17a..18f39c1 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -679,13 +679,20 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
cf->system_identifier |= ((uint64) tv.tv_usec) << 12;
cf->system_identifier |= getpid() & 0xFFF;
- if (!dry_run)
+ if (dry_run)
+ pg_log_info("dry-run: system identifier would be %" PRIu64 " on subscriber",
+ cf->system_identifier);
+ else
+ {
update_controlfile(subscriber_dir, cf, true);
+ pg_log_info("system identifier is %" PRIu64 " on subscriber",
+ cf->system_identifier);
+ }
- pg_log_info("system identifier is %" PRIu64 " on subscriber",
- cf->system_identifier);
-
- pg_log_info("running pg_resetwal on the subscriber");
+ if (dry_run)
+ pg_log_info("dry-run: would run pg_resetwal on the subscriber");
+ else
+ pg_log_info("running pg_resetwal on the subscriber");
cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path,
subscriber_dir, DEVNULL);
@@ -1121,11 +1128,14 @@ drop_existing_subscriptions(PGconn *conn, const char *subname, const char *dbnam
subname);
appendPQExpBuffer(query, " DROP SUBSCRIPTION %s;", subname);
- pg_log_info("dropping subscription \"%s\" in database \"%s\"",
- subname, dbname);
-
- if (!dry_run)
+ if (dry_run)
+ pg_log_info("dry-run: would drop subscription \"%s\" in database \"%s\"",
+ subname, dbname);
+ else
{
+ pg_log_info("dropping subscription \"%s\" in database \"%s\"",
+ subname, dbname);
+
res = PQexec(conn, query->data);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
@@ -1372,8 +1382,12 @@ create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo)
Assert(conn != NULL);
- pg_log_info("creating the replication slot \"%s\" in database \"%s\" on publisher",
- slot_name, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("dry-run: would create the replication slot \"%s\" in database \"%s\" on publisher",
+ slot_name, dbinfo->dbname);
+ else
+ pg_log_info("creating the replication slot \"%s\" in database \"%s\" on publisher",
+ slot_name, dbinfo->dbname);
slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
@@ -1421,8 +1435,12 @@ drop_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo,
Assert(conn != NULL);
- pg_log_info("dropping the replication slot \"%s\" in database \"%s\"",
- slot_name, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("dry-run: would drop the replication slot \"%s\" in database \"%s\"",
+ slot_name, dbinfo->dbname);
+ else
+ pg_log_info("dropping the replication slot \"%s\" in database \"%s\"",
+ slot_name, dbinfo->dbname);
slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
@@ -1648,8 +1666,12 @@ create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
PQclear(res);
resetPQExpBuffer(str);
- pg_log_info("creating publication \"%s\" in database \"%s\"",
- dbinfo->pubname, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("dry-run: would create publication \"%s\" in database \"%s\"",
+ dbinfo->pubname, dbinfo->dbname);
+ else
+ pg_log_info("creating publication \"%s\" in database \"%s\"",
+ dbinfo->pubname, dbinfo->dbname);
appendPQExpBuffer(str, "CREATE PUBLICATION %s FOR ALL TABLES",
ipubname_esc);
@@ -1691,8 +1713,12 @@ drop_publication(PGconn *conn, const char *pubname, const char *dbname,
pubname_esc = PQescapeIdentifier(conn, pubname, strlen(pubname));
- pg_log_info("dropping publication \"%s\" in database \"%s\"",
- pubname, dbname);
+ if (dry_run)
+ pg_log_info("dry-run: would drop publication \"%s\" in database \"%s\"",
+ pubname, dbname);
+ else
+ pg_log_info("dropping publication \"%s\" in database \"%s\"",
+ pubname, dbname);
appendPQExpBuffer(str, "DROP PUBLICATION %s", pubname_esc);
@@ -1800,8 +1826,12 @@ create_subscription(PGconn *conn, const struct LogicalRepInfo *dbinfo)
pubconninfo_esc = PQescapeLiteral(conn, dbinfo->pubconninfo, strlen(dbinfo->pubconninfo));
replslotname_esc = PQescapeLiteral(conn, dbinfo->replslotname, strlen(dbinfo->replslotname));
- pg_log_info("creating subscription \"%s\" in database \"%s\"",
- dbinfo->subname, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("dry-run: would create subscription \"%s\" in database \"%s\"",
+ dbinfo->subname, dbinfo->dbname);
+ else
+ pg_log_info("creating subscription \"%s\" in database \"%s\"",
+ dbinfo->subname, dbinfo->dbname);
appendPQExpBuffer(str,
"CREATE SUBSCRIPTION %s CONNECTION %s PUBLICATION %s "
@@ -1898,8 +1928,12 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
*/
originname = psprintf("pg_%u", suboid);
- pg_log_info("setting the replication progress (node name \"%s\", LSN %s) in database \"%s\"",
- originname, lsnstr, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("dry-run: would set the replication progress (node name \"%s\", LSN %s) in database \"%s\"",
+ originname, lsnstr, dbinfo->dbname);
+ else
+ pg_log_info("setting the replication progress (node name \"%s\", LSN %s) in database \"%s\"",
+ originname, lsnstr, dbinfo->dbname);
resetPQExpBuffer(str);
appendPQExpBuffer(str,
@@ -1944,8 +1978,12 @@ enable_subscription(PGconn *conn, const struct LogicalRepInfo *dbinfo)
subname = PQescapeIdentifier(conn, dbinfo->subname, strlen(dbinfo->subname));
- pg_log_info("enabling subscription \"%s\" in database \"%s\"",
- dbinfo->subname, dbinfo->dbname);
+ if (dry_run)
+ pg_log_info("dry-run: enable subscription \"%s\" in database \"%s\"",
+ dbinfo->subname, dbinfo->dbname);
+ else
+ pg_log_info("enabling subscription \"%s\" in database \"%s\"",
+ dbinfo->subname, dbinfo->dbname);
appendPQExpBuffer(str, "ALTER SUBSCRIPTION %s ENABLE", subname);
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 229fef5..60a8f8c 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -436,11 +436,11 @@ my ($stdout, $stderr) = run_command(
# Verify that the required logical replication objects are output.
# The expected count 3 refers to postgres, $db1 and $db2 databases.
-is(scalar(() = $stderr =~ /creating publication/g),
+is(scalar(() = $stderr =~ /would create publication/g),
3, "verify publications are created for all databases");
-is(scalar(() = $stderr =~ /creating the replication slot/g),
+is(scalar(() = $stderr =~ /would create the replication slot/g),
3, "verify replication slots are created for all databases");
-is(scalar(() = $stderr =~ /creating subscription/g),
+is(scalar(() = $stderr =~ /would create subscription/g),
3, "verify subscriptions are created for all databases");
# Run pg_createsubscriber on node S. --verbose is used twice
--
1.8.3.1
v3-0001-log-to-say-command-is-executing-in-dry-run-mode.patchapplication/octet-stream; name=v3-0001-log-to-say-command-is-executing-in-dry-run-mode.patchDownload
From 8e52963312fcf4046ddcf6532a1adfe284887d20 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 9 Oct 2025 18:41:37 +1100
Subject: [PATCH v3] log to say command is executing in dry run mode
---
src/bin/pg_archivecleanup/pg_archivecleanup.c | 8 ++++++++
src/bin/pg_basebackup/pg_createsubscriber.c | 9 +++++++++
src/bin/pg_combinebackup/pg_combinebackup.c | 8 ++++++++
src/bin/pg_resetwal/pg_resetwal.c | 8 ++++++++
src/bin/pg_resetwal/t/001_basic.pl | 2 +-
src/bin/pg_rewind/pg_rewind.c | 14 ++++++++++++--
6 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index c25348b..dc12627 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -375,6 +375,14 @@ main(int argc, char **argv)
exit(2);
}
+ if (dryrun)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("Executing in dry-run mode.");
+ pg_log_info("No files will be removed.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
/*
* 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 d294074..48df17a 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -2270,6 +2270,15 @@ main(int argc, char **argv)
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
exit(1);
}
+
+ if (dry_run)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("Executing in dry-run mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
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 f5cef99..eca33fb 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -241,6 +241,14 @@ main(int argc, char *argv[])
if (opt.no_manifest)
opt.manifest_checksums = CHECKSUM_TYPE_NONE;
+ if (opt.dry_run)
+ {
+ pg_log_info("-----------------------------------------------------");
+ pg_log_info("Executing in dry-run mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
/* 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 7a4e4eb..6533355 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -396,6 +396,14 @@ main(int argc, char *argv[])
exit(1);
}
+ if (noupdate)
+ {
+ printf(_("-----------------------------------------------------"));
+ printf(_("Executing in dry-run mode."));
+ printf(_("Nothing will be modified."));
+ printf(_("-----------------------------------------------------"));
+ }
+
/*
* Attempt to read the existing pg_control file
*/
diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl
index d6bbbd0..247bd93 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -17,7 +17,7 @@ $node->init;
$node->append_conf('postgresql.conf', 'track_commit_timestamp = on');
command_like([ 'pg_resetwal', '-n', $node->data_dir ],
- qr/checkpoint/, 'pg_resetwal -n produces output');
+ qr/checkpoint/m, 'pg_resetwal -n produces output');
# Permissions on PGDATA should be default
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0c68dd4..3f90be6 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -300,8 +300,18 @@ 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("-----------------------------------------------------");
+ pg_log_info("Executing in dry-run mode.");
+ pg_log_info("The target directory will not be modified.");
+ pg_log_info("-----------------------------------------------------");
+ }
+
+ /*
+ * First, connect to remote server.
*/
if (connstr_source)
{
--
1.8.3.1
On 2025-Oct-09, Peter Smith wrote:
Please see the v3 patches.
Okay, I have pushed 0002 to 18 and master. I wanted to backpatch to 17
but there are too many conflicts there.
I'm not opposed to 0001 (to master only), but I think the lines of
dashes are a little excessively noisy. Are there other opinions on
that? Note that vacuumlo also has it, with no dashes.
I'll mark the commitfest item for version 19.
While looking this over, I noticed that we define USEC_PER_SEC, but why?
We already have USECS_PER_SEC, so let's use that. And WaitPMResult
seems an overgrown boolean, so how about we remove it? Patch for these
things attached. Thoughts?
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)
Attachments:
0001-pg_ctl-use-USECS_PER_SEC-from-datatype-timestamp.h-i.patchtext/x-diff; charset=utf-8Download
From 6ca39215858f0bfdc2ef4676f4f4baf7fc0176c2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Fri, 31 Oct 2025 16:51:22 +0100
Subject: [PATCH 1/2] pg_ctl: use USECS_PER_SEC from datatype/timestamp.h
instead of reinventing the wheel
---
src/bin/pg_basebackup/pg_createsubscriber.c | 5 ++---
src/bin/pg_ctl/pg_ctl.c | 12 ++++++------
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index f59c293d875..52e7c9212a2 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -23,6 +23,7 @@
#include "common/logging.h"
#include "common/pg_prng.h"
#include "common/restricted_token.h"
+#include "datatype/timestamp.h"
#include "fe_utils/recovery_gen.h"
#include "fe_utils/simple_list.h"
#include "fe_utils/string_utils.h"
@@ -129,7 +130,6 @@ static void drop_existing_subscription(PGconn *conn, const char *subname,
static void get_publisher_databases(struct CreateSubscriberOptions *opt,
bool dbnamespecified);
-#define USEC_PER_SEC 1000000
#define WAIT_INTERVAL 1 /* 1 second */
static const char *progname;
@@ -1610,8 +1610,7 @@ wait_for_end_recovery(const char *conninfo, const struct CreateSubscriberOptions
}
/* Keep waiting */
- pg_usleep(WAIT_INTERVAL * USEC_PER_SEC);
-
+ pg_usleep(WAIT_INTERVAL * USECS_PER_SEC);
timer += WAIT_INTERVAL;
}
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 8a405ff122c..b270c0c0d82 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -26,6 +26,7 @@
#include "common/file_perm.h"
#include "common/logging.h"
#include "common/string.h"
+#include "datatype/timestamp.h"
#include "getopt_long.h"
#include "utils/pidfile.h"
@@ -68,9 +69,7 @@ typedef enum
#define DEFAULT_WAIT 60
-#define USEC_PER_SEC 1000000
-
-#define WAITS_PER_SEC 10 /* should divide USEC_PER_SEC evenly */
+#define WAITS_PER_SEC 10 /* should divide USECS_PER_SEC evenly */
static bool do_wait = true;
static int wait_seconds = DEFAULT_WAIT;
@@ -594,6 +593,7 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
{
int i;
+ static_assert(USECS_PER_SEC % WAITS_PER_SEC == 0);
for (i = 0; i < wait_seconds * WAITS_PER_SEC; i++)
{
char **optlines;
@@ -699,7 +699,7 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
print_msg(".");
}
- pg_usleep(USEC_PER_SEC / WAITS_PER_SEC);
+ pg_usleep(USECS_PER_SEC / WAITS_PER_SEC);
}
/* out of patience; report that postmaster is still starting up */
@@ -738,7 +738,7 @@ wait_for_postmaster_stop(void)
if (cnt % WAITS_PER_SEC == 0)
print_msg(".");
- pg_usleep(USEC_PER_SEC / WAITS_PER_SEC);
+ pg_usleep(USECS_PER_SEC / WAITS_PER_SEC);
}
return false; /* timeout reached */
}
@@ -771,7 +771,7 @@ wait_for_postmaster_promote(void)
if (cnt % WAITS_PER_SEC == 0)
print_msg(".");
- pg_usleep(USEC_PER_SEC / WAITS_PER_SEC);
+ pg_usleep(USECS_PER_SEC / WAITS_PER_SEC);
}
return false; /* timeout reached */
}
--
2.47.3
0002-Remove-WaitPMResult-enum-in-pg_createsubscriber.patchtext/x-diff; charset=utf-8Download
From ef79cc503adc6b2154a068b3298e03b494cd46a3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Fri, 31 Oct 2025 17:05:26 +0100
Subject: [PATCH 2/2] Remove WaitPMResult enum in pg_createsubscriber
This is a simple bool
---
src/bin/pg_basebackup/pg_createsubscriber.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 52e7c9212a2..c1120d3643e 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -156,12 +156,6 @@ static char *subscriber_dir = NULL;
static bool recovery_ended = false;
static bool standby_running = false;
-enum WaitPMResult
-{
- POSTMASTER_READY,
- POSTMASTER_STILL_STARTING
-};
-
/*
* Cleanup objects that were created by pg_createsubscriber if there is an
@@ -1584,7 +1578,7 @@ static void
wait_for_end_recovery(const char *conninfo, const struct CreateSubscriberOptions *opt)
{
PGconn *conn;
- int status = POSTMASTER_STILL_STARTING;
+ bool ready = false;
int timer = 0;
pg_log_info("waiting for the target server to reach the consistent state");
@@ -1596,7 +1590,7 @@ wait_for_end_recovery(const char *conninfo, const struct CreateSubscriberOptions
/* Did the recovery process finish? We're done if so. */
if (dry_run || !server_is_in_recovery(conn))
{
- status = POSTMASTER_READY;
+ ready = true;
recovery_ended = true;
break;
}
@@ -1616,7 +1610,7 @@ wait_for_end_recovery(const char *conninfo, const struct CreateSubscriberOptions
disconnect_database(conn, false);
- if (status == POSTMASTER_STILL_STARTING)
+ if (!ready)
pg_fatal("server did not end recovery");
pg_log_info("target server reached the consistent state");
--
2.47.3
On Sat, Nov 1, 2025 at 5:02 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Oct-09, Peter Smith wrote:
Please see the v3 patches.
Okay, I have pushed 0002 to 18 and master. I wanted to backpatch to 17
but there are too many conflicts there.
Thanks for pushing!
I'm not opposed to 0001 (to master only), but I think the lines of
dashes are a little excessively noisy. Are there other opinions on
that? Note that vacuumlo also has it, with no dashes.
It was intentionally "noisy" with dashes to make it impossible to
accidentally overlook dry-run mode in the logs. But if that's contrary
to the way Postgres does things I am fine if you want to remove the
dashes.
I'll mark the commitfest item for version 19.
While looking this over, I noticed that we define USEC_PER_SEC, but why?
We already have USECS_PER_SEC, so let's use that. And WaitPMResult
seems an overgrown boolean, so how about we remove it? Patch for these
things attached. Thoughts?
I took a quick look at your follow-up patches and below are some comments:
//////////
Patch 0001 - USECS_PER_SEC
//////////
+1 to use USECS_PER_SEC from timestamps.h
1.
-#define USEC_PER_SEC 1000000
-
-#define WAITS_PER_SEC 10 /* should divide USEC_PER_SEC evenly */
+#define WAITS_PER_SEC 10 /* should divide USECS_PER_SEC evenly */ static bool do_wait = true;
static int wait_seconds = DEFAULT_WAIT;
@@ -594,6 +593,7 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
{
int i;
+ static_assert(USECS_PER_SEC % WAITS_PER_SEC == 0);
Maybe that static assert might be better placed adjacent to the
#define to enforce that comment about "divide evenly".
SUGGESTION
#define WAITS_PER_SEC 10
/* WAITS_PER_SEC should divide USECS_PER_SEC evenly */
StatiAssertDecl(USECS_PER_SEC % WAITS_PER_SEC == 0);
//////////
Patch 0002 -- boolean
//////////
LGTM.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On 2025-Nov-04, Peter Smith wrote:
On Sat, Nov 1, 2025 at 5:02 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
I'm not opposed to 0001 (to master only), but I think the lines of
dashes are a little excessively noisy. Are there other opinions on
that? Note that vacuumlo also has it, with no dashes.It was intentionally "noisy" with dashes to make it impossible to
accidentally overlook dry-run mode in the logs. But if that's contrary
to the way Postgres does things I am fine if you want to remove the
dashes.
Yeah, I don't know if it's The Postgres Way or just accidental, but I'd
rather stay away from that kind of noise. Also, from a i18n point of
view, the lines of dashes (as implemented here) would get in the message
catalogs, which nobody will appreciate. Please do make the messages a
single string rather than two separate strings though, perhaps with an
embedded newline if more than one line of output is needed.
I took a quick look at your follow-up patches and below are some comments:
Thanks for looking at them. I pushed 0002; I'm waiting on the CI run
https://cirrus-ci.com/build/5618025065349120
to push 0001.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Tue, Nov 4, 2025 at 10:23 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Nov-04, Peter Smith wrote:
On Sat, Nov 1, 2025 at 5:02 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
I'm not opposed to 0001 (to master only), but I think the lines of
dashes are a little excessively noisy. Are there other opinions on
that? Note that vacuumlo also has it, with no dashes.It was intentionally "noisy" with dashes to make it impossible to
accidentally overlook dry-run mode in the logs. But if that's contrary
to the way Postgres does things I am fine if you want to remove the
dashes.Yeah, I don't know if it's The Postgres Way or just accidental, but I'd
rather stay away from that kind of noise. Also, from a i18n point of
view, the lines of dashes (as implemented here) would get in the message
catalogs, which nobody will appreciate. Please do make the messages a
single string rather than two separate strings though, perhaps with an
embedded newline if more than one line of output is needed.
Hi Alvaro,
Here is patch v4-0001 modified as requested:
- dashes are removed
- the message is a single string
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v4-0001-log-to-say-command-is-executing-in-dry-run-mode.patchapplication/octet-stream; name=v4-0001-log-to-say-command-is-executing-in-dry-run-mode.patchDownload
From 9f8c13a2c01f7c7a3a983099ee474c65e0b16896 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Mon, 10 Nov 2025 14:51:30 +1100
Subject: [PATCH v4] log to say command is executing in dry run mode
---
src/bin/pg_archivecleanup/pg_archivecleanup.c | 3 +++
src/bin/pg_basebackup/pg_createsubscriber.c | 4 ++++
src/bin/pg_combinebackup/pg_combinebackup.c | 3 +++
src/bin/pg_resetwal/pg_resetwal.c | 3 +++
src/bin/pg_rewind/pg_rewind.c | 9 +++++++--
5 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index c25348b..d09b761 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -375,6 +375,9 @@ main(int argc, char **argv)
exit(2);
}
+ if (dryrun)
+ pg_log_info("Executing in dry-run mode.\nNo 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 df41836..7aca319 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -2305,6 +2305,10 @@ 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.\nThe 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 3a32512..670205e 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -242,6 +242,9 @@ 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.\nThe 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 a89d72f..b8139d5 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -397,6 +397,9 @@ main(int argc, char *argv[])
exit(1);
}
+ if (noupdate)
+ printf(_("Executing in dry-run mode.\nNothing 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 27c514f..d4fcfef 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -301,8 +301,13 @@ 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.\nThe target directory will not be modified.");
+
+ /*
+ * First, connect to remote server.
*/
if (connstr_source)
{
--
1.8.3.1
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)
Attachments:
v5-0001-Log-a-note-at-program-start-when-running-in-dry-r.patchtext/x-diff; charset=utf-8Download
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
On Tue, Nov 11, 2025 at 8:17 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
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 stringOkay, 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
Right.
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.
It is tempting to implement a "--silent" mode, but if I did that, I
would then feel obliged to document and test it. I don't want to go
further down this rabbit hole for what was originally supposed to be
trivial logging.
So, I am calling it quits for this 0001 patch.
Perhaps it's still of some use to push changes for everything except
the pg_resetwal? Or if you prefer to just abandon the whole patch,
that is OK too. Thanks for trying.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On 2025-Nov-12, Peter Smith wrote:
It is tempting to implement a "--silent" mode, but if I did that, I
would then feel obliged to document and test it. I don't want to go
further down this rabbit hole for what was originally supposed to be
trivial logging.
Yeah, I understand.
So, I am calling it quits for this 0001 patch.
Perhaps it's still of some use to push changes for everything except
the pg_resetwal? Or if you prefer to just abandon the whole patch,
that is OK too. Thanks for trying.
Got it. I pushed it for the other programs and marked the CF entry as
committed -- thanks for the patches and the discussion.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"I'm impressed how quickly you are fixing this obscure issue. I came from
MS SQL and it would be hard for me to put into words how much of a better job
you all are doing on [PostgreSQL]."
Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg00000.php
On Wed, Nov 19, 2025 at 2:39 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Nov-12, Peter Smith wrote:
It is tempting to implement a "--silent" mode, but if I did that, I
would then feel obliged to document and test it. I don't want to go
further down this rabbit hole for what was originally supposed to be
trivial logging.Yeah, I understand.
So, I am calling it quits for this 0001 patch.
Perhaps it's still of some use to push changes for everything except
the pg_resetwal? Or if you prefer to just abandon the whole patch,
that is OK too. Thanks for trying.Got it. I pushed it for the other programs and marked the CF entry as
committed -- thanks for the patches and the discussion.
Thanks for pushing!
======
Kind Regards,
Peter Smith.
Fujitsu Australia