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

On Tue, Feb 25, 2025 at 4:50 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch. Few comments.
>
> 01. CreateSubscriberOptions
> ```
> +       bool            cleanup_existing_publications;  /* drop all publications */
> ```
>
> My previous comment [1] #1 did not intend to update attributes. My point was
> only for the third argument of setup_subscriber().
>
> 02. setup_subscriber()
> ```
> +                       pg_log_info("cleaning up existing publications on the subscriber");
> ```
>
> I don't think this output is needed. check_and_drop_existing_publications() has the similar output.
>
> 03. drop_publication_by_name()
> ```
> +
> +       appendPQExpBuffer(query, "DROP PUBLICATION %s", pubname_esc);
> +       pg_log_info("dropping publication %s in database \"%s\"", pubname, dbname);
> +       pg_log_debug("command is: %s", query->data);
>
> ```
>
> Currently, appendPQExpBuffer() is done after the pg_log_info(). Please reserve current ordering if
> there are no reasons. Also, variable "str" is currently used instead of query, please follow.
>
> 04. drop_publication_by_name()
> ```
>         if (!dry_run)
>         {
> -               res = PQexec(conn, str->data);
> +               res = PQexec(conn, query->data);
>                 if (PQresultStatus(res) != PGRES_COMMAND_OK)
> +                       dbinfo->made_publication = false;
> +               else
>                 {
> -                       pg_log_error("could not drop publication \"%s\" in database \"%s\": %s",
> -                                                dbinfo->pubname, dbinfo->dbname, PQresultErrorMessage(res));
> -                       dbinfo->made_publication = false;       /* don't try again. */
> -
> -                       /*
> -                        * Don't disconnect and exit here. This routine is used by primary
> -                        * (cleanup publication / replication slot due to an error) and
> -                        * subscriber (remove the replicated publications). In both cases,
> -                        * it can continue and provide instructions for the user to remove
> -                        * it later if cleanup fails.
> -                        */
> +                       pg_log_error("could not drop publication %s in database \"%s\": %s",
> +                                                pubname, dbname, PQresultErrorMessage(res));
>                 }
> ```
>
> pg_log_error() is exected when the command succeed: This is not correct.
>
> 05. 040_pg_createsubscriber.pl
> ```
> +# Set up node A as primary
> +my $node_a = PostgreSQL::Test::Cluster->new('node_a');
> +my $aconnstr = $node_a->connstr;
> +$node_a->init(allows_streaming => 'logical');
> +$node_a->append_conf('postgresql.conf', 'autovacuum = off');
> +$node_a->start;
> ```
>
> I don't think new primary is not needed. Can't you reuse node_p?
>
> [1]:
https://www.postgresql.org/message-id/OSCPR01MB14966D845AC77FC46ECEECCE4F5C72%40OSCPR01MB14966.jpnprd01.prod.outlook.com
>

The v10 patch at [1] required rebasing on the latest HEAD. I have
prepared a rebased version and updated the patch accordingly.

[1] - https://www.postgresql.org/message-id/CAHv8RjJtzXc4BWoKTyTB-9WEcAJ4N5qsD6YuXK3EAmsT6237yw%40mail.gmail.com

The attached patch includes the suggested changes.

Thanks and regards,
Shubham Khanna.