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

On Tue, Mar 4, 2025 at 4:31 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Tue, Mar 4, 2025 at 10:30 AM Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > Fixed the suggested changes. The attached patch contains the required changes.
> >
>
> Hi Shubham,
>
> Thanks for the patch! I tested its functionality and didn't find any
> issues so far. I'll continue with further testing.
> Here are some review comments for v12 patch:
>
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 1)
> + printf(_(" -c --cleanup-existing-publications\n"
> + " drop all publications on the
> subscriber\n"));
>
> Similar to docs, here too, we should clarify that publications will be
> dropped only from the specified databases, not all databases.
> Suggestion -
> "drop all publications from specified databases on the subscriber\n"
>

The suggested message was exceeding 95 words, so I have modified it to:-
"drop all publications from specified subscriber databases\n"

> 2)
> @@ -1171,10 +1179,12 @@ check_and_drop_existing_subscriptions(PGconn *conn,
> /*
> * Create the subscriptions, adjust the initial location for logical
> * replication and enable the subscriptions. That's the last step for logical
> - * replication setup.
> + * 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)
> +setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
> + bool cleanup_existing_publications)
> {
>
> It is not clear that the 'drop_publications' value comes from the
> command. How about replacing it with:
> /If 'drop_publications' is/If 'drop_publications' option is/
>
> OR
>
> If you meant to refer to the specific parameter
> 'cleanup_existing_publications', please use the exact name.
>

Fixed.

> 3)
> @@ -1195,8 +1205,13 @@ setup_subscriber(struct LogicalRepInfo *dbinfo,
> const char *consistent_lsn)
> * 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]);
> + if (cleanup_existing_publications)
> + check_and_drop_existing_publications(conn, dbinfo[i].dbname);
> + else
> + drop_publication_by_name(conn, dbinfo[i].pubname, dbinfo[i].dbname);
>
> The existing comment only refers to removing the new publication
> created for the current process and does not mention existing ones.
> With this patch changes, it is unclear which publications are being
> referenced when saying "Remove publications ..."(plural), and the
> phrase "before this command" is ambiguous—it's not clear which command
> is being referred to. The comment should be reworded for better
> clarity.
>
> Suggestion -
>
> "Since the publication was created before the consistent LSN, it
> remains on the subscriber even after the physical replica is
> promoted. Remove this publication from the subscriber because
> it has no use. Additionally, if requested, drop all pre-existing
> publications."
>

Fixed.

> 4)
> +/* Drop a single publication, given in dbinfo. */
> +static void
> +drop_publication_by_name(PGconn *conn, const char *pubname, const char *dbname)
> +{
>
> Since "dbinfo" is no longer passed to this function, the function
> comments should be updated accordingly. Also, use the same format as
> other function comments.
> Suggestion-
> /*
> * Drop the specified publication of the given database.
> */
>

Fixed.

> 5)
> + pg_log_info("dropping publication %s in database \"%s\"", pubname, dbname);
> +
>
> Publication names should be enclosed in double quotes ("") in logs, as
> previously done.
>

Fixed.

> 6)
> + pg_log_error("could not drop publication %s in database \"%s\": %s",
> + pubname, dbname, PQresultErrorMessage(res));
>
> Same as #5, use double quotes for the publication name.
>
> --

Fixed.

The attached Patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.