Here are some review comments for v29-0002 ====== src/bin/pg_upgrade/check.c
1. check_old_cluster_for_valid_slots + /* Quick exit if the cluster does not have logical slots. */ + if (count_old_cluster_logical_slots() == 0) + return; /Quick exit/Quick return/ I know they are kind of the same, but the reason I previously suggested this change was to keep it consistent with the similar comment that is already in check_new_cluster_logical_replication_slots(). ~~~ 2. check_old_cluster_for_valid_slots + /* + * Do additional checks to ensure that confirmed_flush LSN of all the slots + * is the same as the latest checkpoint location. + * + * Note: This can be satisfied only when the old cluster has been shut + * down, so we skip this live checks. + */ + if (!live_check) missing word /skip this live checks./skip this for live checks./ ====== src/bin/pg_upgrade/controldata.c 3. + /* + * Read the latest checkpoint location if the cluster is PG17 + * or later. This is used for upgrading logical replication + * slots. Currently, we need it only for the old cluster but + * didn't add additional check for the similicity. + */ + if (GET_MAJOR_VERSION(cluster->major_version) >= 1700) /similicity/simplicity/ SUGGESTION Currently, we need it only for the old cluster but for simplicity chose not to have additional checks. ====== src/bin/pg_upgrade/info.c 4. get_old_cluster_logical_slot_infos_per_db + /* + * The temporary slots are expressly ignored while checking because such + * slots cannot exist after the upgrade. During the upgrade, clusters are + * started and stopped several times so that temporary slots will be + * removed. + */ + res = executeQueryOrDie(conn, "SELECT slot_name, plugin, two_phase " + "FROM pg_catalog.pg_replication_slots " + "WHERE slot_type = 'logical' AND " + "wal_status <> 'lost' AND " + "database = current_database() AND " + "temporary IS FALSE;"); IIUC, the removal of temp slots is just a side-effect of the start/stop; not the *reason* for the start/stop. So, the last sentence needs some modification BEFORE During the upgrade, clusters are started and stopped several times so that temporary slots will be removed. SUGGESTION During the upgrade, clusters are started and stopped several times causing any temporary slots to be removed. ------ Kind Regards, Peter Smith. Fujitsu Australia