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

On Friday, August 18, 2023 9:52 PM Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)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