On Wed, Oct 8, 2025 at 11:10 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shubham,
>
> Here are some v14 review comments.
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> check_and_drop_publications:
>
> 1.
> + * Publications copied during physical replication remain on the subscriber
> + * after promotion. If --clean=publications is specified, drop all existing
> + * publications in the subscriber database. Otherwise, only drop publications
> + * that were created by pg_createsubscriber during this operation.
> + *
> + * In dry-run mode, create_publication() and drop_publication() only
> log actions
> + * without modifying the database. Importantly, since no publication
> is actually
> + * created in dry-run, the query for existing publications won't include the
> + * "would-be" made publication. Thus, we must call drop_publication() for it
> + * regardless of drop_all_pubs to ensure the user sees the intended
> log message.
> */
>
> I think the "In dry-mode ..." paragraph is a good comment, but it
> doesn't need to be at the function level. IMO, this can all be
> cut/paste to later (see the next review comment).
>
Updated as suggested — the detailed explanation about dry-run handling
has been moved to the appropriate block-level comment.
> ~~~
>
> 2.
> /*
> - * In dry-run mode, we don't create publications, but we still try to drop
> - * those to provide necessary information to the user.
> + * Handle the publication created (or would-be created) by
> + * pg_createsubscriber. In dry-run mode, enter this block regardless of
> + * drop_all_pubs to log the drop action for the made publication (which
> + * isn't actually present).
> */
>
> The wording in the function header seemed better. IMO, this entire
> comment can be replaced like:
>
> /*
> * Handle publications created by pg_createsubscriber.
> *
> * <here cut/paste all the "In dry-mode..." wording from the function
> header comment>
> */
> ~~~
>
> I provided a top-up diff patch to illustrate my point.
>
> ////////////////////
>
Fixed. The comments have been restructured to improve clarity and
placement per your suggestion.
> I also had a look at the 'results' logging from your script.
>
> ~~~
>
> For test case 1. (new pub pub2 + clean):
>
> I hoped to see some evidence that --clean is dropping all other
> existing pubs; e.g. pub1,pub2,pub3, whatever...
>
> ~~~
>
> For test case 3. (Existing pub pub1 + clean):
>
> I hoped to see some evidence that --clean is dropping existing pubs;
> e.g. pub1,pub2,pub3, whatever...
>
> ~~~
>
> For test case 5. (Auto pub + clean):
>
> I hoped to see some evidence that --clean is dropping existing pubs;
> e.g. pub1,pub2,pub3, whatever...
>
For these cases, I manually created multiple publications in the
postgres, db1, and db2 databases to make the --clean behavior clearly
visible in the logs.
I’ve attached the updated test script, captured logs, and the expected
log matrix for all cases to demonstrate the behavior.
Additionally, following your note in [1], I have removed the redundant
pg_log_info("create publication..."); statement since the same message
was already being logged inside create_publication().
The attached patch incorporates the revised comments and test-related updates.
[1] - https://www.postgresql.org/message-id/CAHut%2BPsYKXKmMSim5FcFo177gosY7a5xgMH_p4AY1_25HMVL7Q%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
