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

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

~~

Benefits:

1a.
One version check instead of multiple.

~

1b.
Upfront slot counting means
- only call 1 time to count_old_cluster_slots().
- unnecessary calls to other check* functions are avoided

~

1c.
get_old_cluster_logical_slot_infos
- No version check is needed.

check_old_cluster_for_lost_slots
- Call to count_old_cluster_slots is not needed
- Quick exit not needed.

check_old_cluster_for_confirmed_flush_lsn
- Call to count_old_cluster_slots is not needed
- Quick exit not needed.

~~~

2. check_old_cluster_for_lost_slots

+ /* Quick exit if the cluster does not have logical slots. */
+ if (count_old_cluster_logical_slots() == 0)
+ return;

Refer [1]#4. Can remove this because #1b above.

~~~

3. check_old_cluster_for_confirmed_flush_lsn

+ /* Quick exit if the cluster does not have logical slots. */
+ if (count_old_cluster_logical_slots() == 0)
+ return;

Refer [1]#5. Can remove this because #1b above.

~~~

4. .../t/003_logical_replication_slots.pl

/shipped/replicated/

Kuroda-san 26/8 wrote:
You meant to say s/replicated/shipped/, right? Fixed.

No, I meant what I wrote for [1]#7. I was referring to the word
"shipped" in the message 'check changes are shipped to the
subscriber'. Now there are 2 places to change instead of one.

------
[1] my review of v25-0003.
https://www.postgresql.org/message-id/CAHut%2BPsdkhcVG5GY4ZW0DMUF8FG%3DWvjaGN%2BNA4XFLrzxWSQXVA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia