Dear Peter, Thanks for reviewing! New patch can be available in [1]. > > ====== > 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().
Fixed. > 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./ Fixed. > 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. Fixed. > 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. > Fixed. [1]: https://www.postgresql.org/message-id/TYAPR01MB5866F7D8ED15BA1E8E4A2AB0F5E4A%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best Regards, Hayato Kuroda FUJITSU LIMITED
