Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.

On Fri, Feb 21, 2025 at 3:06 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch. Here are comments.
>
> 01.
> ```
> /*
>  * Create the subscriptions, adjust the initial location for logical
>  * replication and enable the subscriptions. That's the last step for logical
>  * replication setup. If 'drop_publications' is true, existing publications on
>  * the subscriber will be dropped before creating new subscriptions.
>  */
> static void
> setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
>                                  bool drop_publications)
> ```
>
> Even drop_publications is false, at least one publication would be dropped. The
> argument is not correct. I prefer previous name.
>

Fixed.

> 02.
> ```
>                 /* Drop all existing publications if requested */
>                 if (drop_publications)
>                 {
>                         pg_log_info("Dropping all existing publications in database \"%s\"",
>                                                 dbinfo[i].dbname);
>                         drop_all_publications(conn, dbinfo[i].dbname);
>                 }
>
>                 /*
>                  * Since the publication was created before the consistent LSN, it is
>                  * available on the subscriber when the physical replica is promoted.
>                  * Remove publications from the subscriber because it has no use.
>                  * Additionally, drop publications existed before this command if
>                  * requested.
>                  */
>                 drop_publication(conn, &dbinfo[i]);
> ```
>
> It is quite strange that drop_publication() is called even when drop_all_publications() is called.
>

Fixed.

> 03.
> Also, I'm not sure pg_log_info() is needed here. _drop_one_publication() outputs dropping publications.
>

Fixed.

> 04.
> ```
> /* Helper function to drop a single publication by name. */
> static void
> _drop_one_publication(PGconn *conn, const char *pubname, const char *dbname)
> ```
>
> Functions recently added do not have prefix "_". I think it can be removed.
>

Removed this function.

> 05.
> ```
>         pg_log_debug("Executing: %s", query->data);
>         pg_log_info("Dropping publication %s in database \"%s\"", pubname, dbinfo->dbname);
> ```
> In _drop_one_publication(), dbname is used only for the message. Can we move to pg_log_info()
> outside the function and reduce the argument?
>

Fixed.

> 06.
> Also, please start with lower case.
>

Fixed.

> 07.
> Also, please preserve the message as much as possible. Previously they are:
> ```
> pg_log_info("dropping publication \"%s\" in database \"%s\"", dbinfo->pubname, dbinfo->dbname);
> pg_log_debug("command is: %s", str->data);
> ```
>

Fixed.

> 08.
> I feel we must update made_publication.
>

Fixed.

The attached patch at [1] contains the suggested changes.

[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BzmkwunNubo%2B_Gp26S_qXbD%3Dp%2BrMfEnPnjiEE1A6GTXQ%40mail.gmail.com

Thanks and regards,
Shubham Khanna.