On Thu, Oct 9, 2025 at 4:38 PM Chao Li <[email protected]> wrote: > > I think is patch is helpful. A few comments: > > On Oct 9, 2025, at 08:55, Peter Smith <[email protected]> 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
v3-0002-add-different-dry-run-logging-for-pg_createsubscr.patch
Description: Binary data
v3-0001-log-to-say-command-is-executing-in-dry-run-mode.patch
Description: Binary data
