Re: Add support for specifying tables in pg_createsubscriber.

On Wed, Oct 1, 2025 at 6:00 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Here are some v13 review comments.
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 1.
> - /*
> - * In dry-run mode, we don't create publications, but we still try to drop
> - * those to provide necessary information to the user.
> - */
> if (!drop_all_pubs || dry_run)
> - drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
> - &dbinfo->made_publication);
> + {
> + /*
> + * Since the publications were created before the consistent LSN, they
> + * remain on the subscriber even after the physical replica is
> + * promoted. Only drop publications that were created by
> + * pg_createsubscriber during this operation. Pre-existing
> + * publications are preserved.
> + */
> + if (!drop_all_pubs && dbinfo->made_publication)
> + drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
> + &dbinfo->made_publication);
> + else if (!drop_all_pubs)
> + pg_log_info("preserve existing publication \"%s\" in database \"%s\"",
> + dbinfo->pubname, dbinfo->dbname);
> + }
>
> 1a.
> Sorry, that code looks wrong to me. Notice, the result of that patch
> fragment is essentially:
>
> if (!drop_all_pubs || dry_run)
> {
> if (!drop_all_pubs && dbinfo->made_publication)
> drop_publication(...);
> else if (!drop_all_pubs)
> pg_log_info("preserve existing...";
> }
>
> You've coded *every* condition say !drop_all_pubs, which is basically
> saying when drop_all_pubs == true then dry_run does nothing at all
> (??). Surely, that's not the intention.
>
> Let's see a specific test case: --publication=mynew
> --clean=publications --dry-run
>
> According to your matrix, AFAICT, you expect this to log:
> - "create publication mynew"
> - "dropping all existing"
> - "dropping other existing..." (loop)
> - "drop publication mynew"
>
> But, I don't see how that "drop publication mynew" is possible with
> this code. Can you provide some test/script output as proof that it
> actually works like you say it does?
>
> ~~~
>
> 1b.
> The original code at least had a comment saying what it was trying to do:
>
> - /*
> - * In dry-run mode, we don't create publications, but we still try to drop
> - * those to provide necessary information to the user.
> - */
>
> Why was that comment removed? In my v12 review, I suggested some
> possible better wording [1] for that. Please imagine someone reading
> this code without handy access to that "expected logging" matrix, and
> write explanatory comments accordingly.
>
> ======
> [1] https://www.postgresql.org/message-id/CAHut%2BPtk%3Ds9VweQWXatEZ7i9GiFxZn_3A5wMSE_gDO9h7jEcRA%40mail.gmail.com
>

Hi Peter,

Thanks for the detailed review. You are right — with the earlier code,
the “drop publication mynew” case was indeed not possible. I’ve
reworked the logic so that the behavior now matches the expected
matrix in all scenarios.
I’ve also prepared a script and captured the output to demonstrate
this, which I’ve attached for reference.
Additionally, I have updated the comments per your suggestion to
better explain the intended behavior, especially around the dry-run
handling, so that it’s clearer to a future reader without needing to
refer back to the logging matrix.
The attached patch includes these changes.

Thanks and regards,
Shubham Khanna.