Hi Kuroda-san.
I already posted a review for v22-0003 earlier today, but v23-0003 was already posted so those are not yet addressed.
Here are a few more review comments I noticed when looking at the latest v23-0003.
======
src/bin/pg_upgrade/check.c
1.
+#include "access/xlogdefs.h"
#include "catalog/pg_authid_d.h"
Was this #include needed here? I noticed you've already included the same in the "pg_upgrade.h".
~~~
2. check_for_lost_slots
+ /* Check there are no logical replication slots with a 'lost' state. */
+ res = executeQueryOrDie(conn,
+ "SELECT slot_name FROM pg_catalog.pg_replication_slots "
+ "WHERE wal_status = 'lost' AND "
+ "temporary IS FALSE;");
I can't quite describe my doubts about this, but something seems a bit strange. Didn't we already iterate every single slot in all DBs in the earlier function get_logical_slot_infos_per_db()? There we were only looking for wal_status <> 'lost', but we could have got *every* wal_status and also detected these 'lost' ones at the same time up-front, instead of having this extra function with more SQL to do pretty much the same SELECT.
Perhaps coding the current way there is a clear separation of the fetching code and the checking code, and that might be the best approach, but it somehow seems a shame/waste to be executing almost the same slots data with the same SQL 2x, so I wondered if there is a better way to arrange this.
======
src/bin/pg_upgrade/info.c
3. get_logical_slot_infos
+
+ /* Do additional checks if slots are found */
+ if (slot_count)
+ {
+ check_for_lost_slots(cluster);
+
+ if (!live_check)
+ check_for_confirmed_flush_lsn(cluster);
+ }
Aren't these checks only intended for checking the 'old_cluster'? But AFAICT they are not guarded here so they will be executed by both sides. Previously (in my review of v22-0003) I suggested these calls maybe belonged in the calling function check_and_dump_old_cluster(). I think that.
------
Kind Regards,
Peter Smith.
Fujitsu Australia