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
