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

Dear Peter,

Thanks for giving comments! New version can be available in [1].

>
1.
+#include "access/xlogdefs.h"
#include "catalog/pg_authid_d.h"

Was this #include needed here? I noticed you've already included the same in the "pg_upgrade.h".
>

It was needed because the macro LSN_FORMAT_ARGS() was used in the file.
I preferred all the needed file are included even if it has already been done in header, so
#include was written here.

>
2. check_for_lost_slots

+ /* Check there are no logical replication slots with a 'lost' state. */
+ res = executeQueryOrDie(conn,
+ "SELECT slot_name FROM pg_catalog.pg_replication_slots "
+ "WHERE wal_status = 'lost' AND "
+ "temporary IS FALSE;");

I can't quite describe my doubts about this, but something seems a bit strange. Didn't we already iterate every single slot in all DBs in the earlier function get_logical_slot_infos_per_db()? There we were only looking for wal_status <> 'lost', but we could have got *every* wal_status and also detected these 'lost' ones at the same time up-front, instead of having this extra function with more SQL to do pretty much the same SELECT.

Perhaps coding the current way there is a clear separation of the fetching code and the checking code, and that might be the best approach, but it somehow seems a shame/waste to be executing almost the same slots data with the same SQL 2x, so I wondered if there is a better way to arrange this.
>

Hmm, but you did not like to do additional checks in the get_logical_slot_infos(),
right? They cannot go together. In case of check_new_cluster(), information for
relations is extracted in get_db_and_rel_infos() and then checked whether it is
empty or not in check_new_cluster_is_empty(). The phase is also separated.

>
src/bin/pg_upgrade/info.c

3. get_logical_slot_infos

+
+ /* Do additional checks if slots are found */
+ if (slot_count)
+ {
+ check_for_lost_slots(cluster);
+
+ if (!live_check)
+ check_for_confirmed_flush_lsn(cluster);
+ }

Aren't these checks only intended for checking the 'old_cluster'? But AFAICT they are not guarded here so they will be executed by both sides. Previously (in my review of v22-0003) I suggested these calls maybe belonged in the calling function check_and_dump_old_cluster(). I think that.
>

Moved to check_and_dump_old_cluster().

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866DD3348B5224E0A1BFC3EF51CA%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED