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
