Dear Hou,
Thank you for reviewing! The patch can be available in [1].
> 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.
Fixed.
> 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.
Fixed.
> 3.
> + if
> (GET_MAJOR_VERSION(cluster->major_version) >= 17)
> + {
>
> I think you mean 1700 here.
Right, fixed.
> 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.
Added.
[1]:
https://www.postgresql.org/message-id/TYCPR01MB5870B5C0FE0C61CD04CBD719F51EA%40TYCPR01MB5870.jpnprd01.prod.outlook.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
