Re: [PoC] pg_upgrade: allow to upgrade publisher node

On Mon, 22 May 2023 at 15:50, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Wang,
>
> Thank you for reviewing! PSA new version.

Thanks for the updated patch, Few comments:
Few comments
1) check_for_parameter_settings, check_for_confirmed_flush_lsn and
check_are_logical_slots_active functions all have the same messages,
we can keep it unique so that it is easy for user to interpret:
+check_for_parameter_settings(ClusterInfo *new_cluster)
+{
+ PGresult *res;
+ PGconn *conn = connectToServer(new_cluster, "template1");
+ int max_replication_slots;
+ char *wal_level;
+
+ prep_status("Checking for logical replication slots");
+
+ res = executeQueryOrDie(conn, "SHOW max_replication_slots;");

+check_for_confirmed_flush_lsn(ClusterInfo *cluster)
+{
+ int i,
+ ntups,
+ i_slotname;
+ bool is_error = false;
+ PGresult *res;
+ DbInfo *active_db = &cluster->dbarr.dbs[0];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+
+ Assert(user_opts.include_logical_slots);
+
+ /* --include-logical-replication-slots can be used since PG16. */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 1500)
+ return;
+
+ prep_status("Checking for logical replication slots");

+check_are_logical_slots_active(ClusterInfo *cluster)
+{
+ int i,
+ ntups,
+ i_slotname;
+ bool is_error = false;
+ PGresult *res;
+ DbInfo *active_db = &cluster->dbarr.dbs[0];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+
+ Assert(user_opts.include_logical_slots);
+
+ /* --include-logical-replication-slots can be used since PG16. */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 1500)
+ return;
+
+ prep_status("Checking for logical replication slots");

2) This function can be placed above get_logical_slot_infos and the
prototype from this file can be removed:
+/*
+ * get_logical_slot_infos_per_db()
+ *
+ * gets the LogicalSlotInfos for all the logical replication slots of
the database
+ * referred to by "dbinfo".
+ */
+static void
+get_logical_slot_infos_per_db(ClusterInfo *cluster, DbInfo *dbinfo)
+{
+ PGconn *conn = connectToServer(cluster,
+
dbinfo->db_name);

3) LogicalReplicationSlotInfo should be placed after LogicalRepWorker
to keep the order consistent:
LogicalRepCommitPreparedTxnData
LogicalRepCtxStruct
+LogicalReplicationSlotInfo
LogicalRepMsgType

4) "existence of slots" be changed to "existence of slots."
+ /*
+ * If --include-logical-replication-slots is required, check the
+ * existence of slots
+ */

5) This comment can be removed:
+ *
+ * XXX: add more attributes if needed
+ */

Regards,
Vignesh