On Friday, August 18, 2023 9:52 PM Kuroda, Hayato/黒田 隼人
<kuroda.hay...@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
