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”
seemsnot needed because “dry-run” has clearly indicated nothing would actually happen. This comment applies to all
changesin 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
