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

On Mon, Aug 28, 2023 at 1:01 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi, here are my review comments for v26-0003
>
> It seems I must defend some of my previous suggestions from v25* [1],
> so here goes...
>
> ======
> src/bin/pg_upgrade/check.c
>
> 1. check_and_dump_old_cluster
>
> CURRENT CODE (with v26-0003 patch applied)
>
> /* Extract a list of logical replication slots */
> get_old_cluster_logical_slot_infos();
>
> ...
>
> /*
> * Logical replication slots can be migrated since PG17. See comments atop
> * get_old_cluster_logical_slot_infos().
> */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> {
> check_old_cluster_for_lost_slots();
>
> /*
> * Do additional checks if a live check is not required. This requires
> * that confirmed_flush_lsn of all the slots is the same as the latest
> * checkpoint location, but it would be satisfied only when the server
> * has been shut down.
> */
> if (!live_check)
> check_old_cluster_for_confirmed_flush_lsn();
> }
>
>
> SUGGESTION
>
> /*
>  * Logical replication slots can be migrated since PG17. See comments atop
>  * get_old_cluster_logical_slot_infos().
>  */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700) // NOTE 1a.
> {
>   /* Extract a list of logical replication slots */
>   get_old_cluster_logical_slot_infos();
>
>   if (count_old_cluster_slots()) // NOTE 1b.
>   {
>     check_old_cluster_for_lost_slots();
>
>     /*
>      * Do additional checks if a live check is not required. This requires
>      * that confirmed_flush_lsn of all the slots is the same as the latest
>      * checkpoint location, but it would be satisfied only when the server
>      * has been shut down.
>      */
>     if (!live_check)
>       check_old_cluster_for_confirmed_flush_lsn();
>   }
> }
>

I think a slightly better way to achieve this is to combine the code
from check_old_cluster_for_lost_slots() and
check_old_cluster_for_confirmed_flush_lsn() into
check_old_cluster_for_valid_slots(). That will even save us a new
connection for the second check.

Also, I think we can simplify another check in the patch:
@@ -1446,8 +1446,10 @@ check_new_cluster_logical_replication_slots(void)
        char       *wal_level;

        /* Logical slots can be migrated since PG17. */
-       if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
-               nslots = count_old_cluster_logical_slots();
+       if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
+               return;
+
+       nslots = count_old_cluster_logical_slots();


-- 
With Regards,
Amit Kapila.