On Mon, Nov 3, 2025 at 12:55 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shubham.
>
> A comment about the v17-0001.
>
> ======
> 1.
> + if (check_publication_exists(conn, dbinfo[i].pubname, dbinfo[i].dbname))
> + {
> + /* Reuse existing publication on publisher. */
> + pg_log_info("dry-run: would use existing publication \"%s\" in
> database \"%s\"",
> + dbinfo[i].pubname, dbinfo[i].dbname);
> + dbinfo[i].made_publication = false;
> + }
>
> Is that correct? Won't this code now unconditionally log with the
> "dry-run:" prefix, even when the tool is *not* doing a dry-run?
>
> I thought code would be something like:
>
> SUGGESTION #1 (if/else)
> /* Reuse existing publication on publisher. */
> if (dry_run)
> pg_log_info("dry-run: would use existing publication ...);
> else
> pg_log_info("use existing publication ...);
>
> ~~~
>
> OTOH, (since here is just an info message with no destructive
> operation) perhaps it would be harmless also to keep the original log
> message for both dry-run and normal mode.
>
> SUGGESTION #2 (do nothing)
> pg_log_info("use existing publication ...);
>
> ======
Hi Peter,
Thank you for your review and suggestions.
I agree with your reasoning regarding the logging behavior. I will
proceed with Suggestion #2 and retain the existing `pg_log_info("use
existing publication ...");` message for both dry-run and normal
modes. This message is purely informational and does not perform any
destructive action, making it suitable for both scenarios.
The attached patch includes these changes.
Thanks and regards,
Shubham Khanna.
