On Tue, Feb 25, 2025 at 4:50 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@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(). >
Fixed.
> 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.
>
Fixed.
> 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.
>
Fixed.
> 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.
>
Fixed.
> 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?
>
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v10-0001-Support-for-dropping-all-publications-in-pg_crea.patch
Description: Binary data
