On Mon, Aug 28, 2023 at 1:01 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Hi, here are my review comments for v26-0003 > > It seems I must defend some of my previous suggestions from v25* [1], > so here goes... > > ====== > src/bin/pg_upgrade/check.c > > 1. check_and_dump_old_cluster > > CURRENT CODE (with v26-0003 patch applied) > > /* Extract a list of logical replication slots */ > get_old_cluster_logical_slot_infos(); > > ... > > /* > * Logical replication slots can be migrated since PG17. See comments atop > * get_old_cluster_logical_slot_infos(). > */ > if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700) > { > check_old_cluster_for_lost_slots(); > > /* > * Do additional checks if a live check is not required. This requires > * that confirmed_flush_lsn of all the slots is the same as the latest > * checkpoint location, but it would be satisfied only when the server > * has been shut down. > */ > if (!live_check) > check_old_cluster_for_confirmed_flush_lsn(); > } > > > SUGGESTION > > /* > * Logical replication slots can be migrated since PG17. See comments atop > * get_old_cluster_logical_slot_infos(). > */ > if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700) // NOTE 1a. > { > /* Extract a list of logical replication slots */ > get_old_cluster_logical_slot_infos(); > > if (count_old_cluster_slots()) // NOTE 1b. > { > check_old_cluster_for_lost_slots(); > > /* > * Do additional checks if a live check is not required. This requires > * that confirmed_flush_lsn of all the slots is the same as the latest > * checkpoint location, but it would be satisfied only when the server > * has been shut down. > */ > if (!live_check) > check_old_cluster_for_confirmed_flush_lsn(); > } > } >
I think a slightly better way to achieve this is to combine the code from check_old_cluster_for_lost_slots() and check_old_cluster_for_confirmed_flush_lsn() into check_old_cluster_for_valid_slots(). That will even save us a new connection for the second check. Also, I think we can simplify another check in the patch: @@ -1446,8 +1446,10 @@ check_new_cluster_logical_replication_slots(void) char *wal_level; /* Logical slots can be migrated since PG17. */ - if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700) - nslots = count_old_cluster_logical_slots(); + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600) + return; + + nslots = count_old_cluster_logical_slots(); -- With Regards, Amit Kapila.