On Thu, Sep 25, 2025 at 9:02 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> On Sep 24, 2025, at 14:37, Shubham Khanna <khannashubham1197@gmail.com> wrote:
>
>
>
> The attached patch contains the suggested changes.
>
> Thanks and regards,
> Shubham Khanna.
> <v11-0001-Support-existing-publications-in-pg_createsubscr.patch>
>
>
> 1.
> ```
> + if (dbinfo->made_publication)
> + drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
> + &dbinfo->made_publication);
> + else
> + pg_log_info("preserve existing publication \"%s\" in database \"%s\"",
> + dbinfo->pubname, dbinfo->dbname);
> + }
> ```
>
> Should we preserve “|| dry_run”? Because based on the old comment, in dry-run mode, even if we don’t create
publications,we still want to inform the user.
>
We don’t need to add an explicit "|| dry_run" here, since the
made_publication flag already accounts for that case. In dry-run mode,
no publications are actually created, so made_publication is never
set. This ensures we still hit the “preserve existing publication …”
branch and inform the user accordingly.
> 2.
> ```
> + create_publication(conn, &dbinfo[i]);
> + pg_log_info("create publication \"%s\" in database \"%s\"",
> + dbinfo[i].pubname, dbinfo[i].dbname);
> + dbinfo[i].made_publication = true;
> ```
>
> dbinfo[i].made_publication = true; seems a redundant as create_publication() has done the assignment.
>
I have removed the redundant dbinfo[i].made_publication = true;, since
create_publication() already sets that flag.
> 3.
> ```
> @@ -1729,11 +1769,9 @@ drop_publication(PGconn *conn, const char *pubname, const char *dbname,
> /*
> * Retrieve and drop the publications.
> *
> - * Since the publications were created before the consistent LSN, they
> - * remain on the subscriber even after the physical replica is
> - * promoted. Remove these publications from the subscriber because
> - * they have no use. Additionally, if requested, drop all pre-existing
> - * publications.
> + * If --clean=publications is specified, drop all existing
> + * publications in the database. Otherwise, only drop publications that were
> + * created by pg_createsubscriber.
> */
> static void
> check_and_drop_publications(PGconn *conn, struct LogicalRepInfo *dbinfo)
> ```
>
> The old comment clearly stated that deleting publication on target server, the updated comment loses that important
information.
>
I have adjusted the comments so that the function header now contains
the general description, while the else block comment explains the
subscriber (target server) context that was missing earlier. This way,
the header stays concise, and the important detail about where the
publications are being dropped is still preserved in the right place.
The attached patch contains the suggested changes. It also contains
the fix for Peter's comments at [1].
[1] - https://www.postgresql.org/message-id/CAHut%2BPsCQxWoPh-UXBUWu%3D6Pc6GuEQ4wnHZtDOwUnZN%3DkrMxvQ%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
