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

On Friday, August 18, 2023 9:52 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:
> 
> Dear Peter,
> 
> PSA new version patch set.

Thanks for updating the patch!
Here are few comments about 0003 patch.

1.

+check_for_lost_slots(ClusterInfo *cluster)
+{
+    int            i,
+                ntups,
+                i_slotname;
+    PGresult   *res;
+    DbInfo       *active_db = &cluster->dbarr.dbs[0];
+    PGconn       *conn = connectToServer(cluster, active_db->db_name);
+ 
+    /* logical slots can be migrated since PG17. */
+    if (GET_MAJOR_VERSION(cluster->major_version) <= 1600)
+        return;

I think we should build connection after this check, otherwise the connection
may be left open after returning.


2.
+check_for_confirmed_flush_lsn(ClusterInfo *cluster)
+{
+    int            i,
+                ntups,
+                i_slotname;
+    PGresult   *res;
+    DbInfo       *active_db = &cluster->dbarr.dbs[0];
+    PGconn       *conn = connectToServer(cluster, active_db->db_name);
+
+    /* logical slots can be migrated since PG17. */
+    if (GET_MAJOR_VERSION(cluster->major_version) <= 1600)
+        return;

Same as above.

3.
+                if (GET_MAJOR_VERSION(cluster->major_version) >= 17)
+                {

I think you mean 1700 here.


4.
+                    p = strpbrk(p, "01234567890ABCDEF");
+
+                    /*
+                     * Upper and lower part of LSN must be read separately
+                     * because it is reported as %X/%X format.
+                     */
+                    upper_lsn = strtoul(p, &slash, 16);
+                    lower_lsn = strtoul(++slash, NULL, 16);

Maybe we'd better add a sanity check after strpbrk like "if (p == NULL ||
strlen(p) <= 1)" to be consistent with other similar code.

Best Regards,
Hou zj