Dear Peter, Thank you for reviewing!
> 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(); > } > } > > ~~ > > Benefits: > > 1a. > One version check instead of multiple. > > ~ > > 1b. > Upfront slot counting means > - only call 1 time to count_old_cluster_slots(). > - unnecessary calls to other check* functions are avoided > > ~ > > 1c. > get_old_cluster_logical_slot_infos > - No version check is needed. > > check_old_cluster_for_lost_slots > - Call to count_old_cluster_slots is not needed > - Quick exit not needed. > > check_old_cluster_for_confirmed_flush_lsn > - Call to count_old_cluster_slots is not needed > - Quick exit not needed. > > ~~~ > > 2. check_old_cluster_for_lost_slots > > + /* Quick exit if the cluster does not have logical slots. */ > + if (count_old_cluster_logical_slots() == 0) > + return; > > Refer [1]#4. Can remove this because #1b above. > > 3. check_old_cluster_for_confirmed_flush_lsn > > + /* Quick exit if the cluster does not have logical slots. */ > + if (count_old_cluster_logical_slots() == 0) > + return; > > Refer [1]#5. Can remove this because #1b above. IIUC these points were disagreed by Amit, so I would keep my code until he posts opinions. > 4. .../t/003_logical_replication_slots.pl > > /shipped/replicated/ > > Kuroda-san 26/8 wrote: > You meant to say s/replicated/shipped/, right? Fixed. > > No, I meant what I wrote for [1]#7. I was referring to the word > "shipped" in the message 'check changes are shipped to the > subscriber'. Now there are 2 places to change instead of one. > Oh, sorry for that. Both places was fixed. Best Regards, Hayato Kuroda FUJITSU LIMITED