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

Dear Amit,

Thank you for giving comments! PSA new version. I ran the pgindent.

> > 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.

They are combined into one function.

> 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();
>

Fixed.

Also, I have tested the combination of this patch and the physical standby.

1. Logical slots defined on old physical standby *cannot be upgraded*
2. Logical slots defined on physical primary *are migrated* to new physical standby

The primal reason is that pg_upgrade cannot be used for physical standby. If
users want to upgrade standby, rsync command is used instead. The command
creates the cluster based on the based on the new primary, hence they are
replicated to new standby. In contrast, the old cluster is basically ignored so
that slots on old cluster is not upgraded.  I updated the doc accordingly.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED