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.
