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

On Mon, Mar 17, 2025, at 9:34 AM, Shubham Khanna wrote:
> I have incorporated the "--remove/-r" parameter in the attached patch,
> as it seems more intuitive and straightforward for users.
> The attached patch contains the latest changes.

There were a lot of discussion around the single vs multiple options since my
last review [1] so I'm answering some of the questions here.

Due to the nature of removing multiple objects, I would say that a general
option that has a value and can be informed multiple times is the way to go. I
saw that the discussion led to this. Regarding the name, my preference is
--drop since we already have other binaries with similar options (pg_receivewal
and pg_recvlogical have --drop-slot).

The commit message needs some adjustments. There are redundant information (1st
and last paragraph).

+ <literal>publications</literal>. This option is useful when
+ transitioning from streaming replication to logical replication as
+ existing objects may no longer be needed. Before using this option,

Use physical replication. That's what we used in the current
pg_createsubscriber documentation and it is also the terminology referred in
the glossary (see Replication).

bool two_phase; /* enable-two-phase option */
+ uint32 remove_objects; /* flag to remove objects on subscriber */

uint32 -> bits32.

-static void setup_subscriber(struct LogicalRepInfo *dbinfo,
- const char *consistent_lsn);
+static void setup_subscriber(const char *consistent_lsn);

This is unrelated to this patch.

- * replication setup.
+ * replication setup. If 'remove' parameter is specified, 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(const char *consistent_lsn)

There is no parameter 'remove' in this function. I understand that you want to
provide information about the remove option but it is not the right place to
add it. If for some reason you need to change or remove such parameter, it is
easier to left this comment because it is not near the place you are removing
some lines of code.

+ struct LogicalRepInfo *dbinfo = &dbinfos.dbinfo[i];

/* Connect to subscriber. */
- conn = connect_database(dbinfo[i].subconninfo, true);
+ conn = connect_database(dbinfo->subconninfo, true);

This new dbinfo pointer is just a way to increase your patch size without
improving readability.

- drop_publication(conn, &dbinfo[i]);
+ if (dbinfos.remove_objects & OBJECT_PUBLICATIONS)
+ drop_all_publications(conn, dbinfo);
+ else
+ drop_publication(conn, dbinfo, dbinfo->pubname);

At first glance, I didn't like this change. You inform dbinfo->pubname as a 3rd
parameter but the 2nd parameter is dbinfo. After reading
drop_all_publication(), I realized that's the cause for this change. Is there a
better way to do it?

+static void
+drop_all_publications(PGconn *conn, struct LogicalRepInfo *dbinfo)
+{
+ PGresult *res;
+

I would add an Assert(conn != NULL) here to follow the same pattern as the
other functions.

+ res = PQexec(conn, "SELECT pubname FROM pg_catalog.pg_publication;");
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("could not obtain publication information: %s",
+ PQresultErrorMessage(res));
+ PQclear(res);
+ return;
+ }

Shouldn't it disconnect_database() here? See the pattern in other functions
that error out while querying the catalog.

+ SimpleStringListCell *cell;
+
+ for (cell = opt.remove_objects.head; cell; cell = cell->next)
+ {

You could use SimpleStringListCell in the for loop without a separate declaration.

+ pg_log_error("wrong remove object is specified");
+ pg_log_error_hint("Only \"publications\" can be removed.");
+ exit(1);

The main message sounds strange. Did you mean 'wrong object is specified'?

+ok($node_s->safe_psql($db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
+ 'two publications created before --remove is run');
+

two pre-existing publications on subscriber ?

+is($node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;"),
+ '0', 'all publications dropped after --remove is run');
+

all publications on subscriber have been removed ?

+ primary_conninfo = '$pconnstr'
+ max_logical_replication_workers = 5

Do you need to set m_l_r_w here?

+# --verbose is used twice to show more information.

This comment is superfluous. Remove it.

+# Confirm publications still remain after running 'pg_createsubscriber'
+is($node_u->safe_psql($db3, "SELECT COUNT(*) FROM pg_publication;"),
+ '2', 'all publications remain after running pg_createsubscriber');

I would remove the semicolon here because none of the SQL commands in this test
file includes it.

Files=1, Tests=43, 14 wallclock secs ( 0.04 usr 0.01 sys + 1.65 cusr 2.06 csys = 3.76 CPU)
Result: PASS

Do we really need this extra test? It increases the test duration by 4 seconds.
Again, that's still unacceptable for such an optional feature. Maybe you should
experiment my suggestion in the previous review. You don't need a real run to
prove that pg_createsubscriber is not removing unintended objects.

[1] http://postgr.es/m/6f266ce2-38d4-433f-afc9-b47c48c17509@app.fastmail.com

--
Euler Taveira
EDB https://www.enterprisedb.com/