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

On Mon, 22 May 2023 at 15:50, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.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