RE: [PoC] pg_upgrade: allow to upgrade publisher node

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