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

Hi Kuroda-san,

Here are my review comments for patch v25-0003.

======
src/bin/pg_upgrade/check.c

1. GENERAL

+static void check_for_confirmed_flush_lsn(void);
+static void check_for_lost_slots(void);

For more clarity, I wonder if it is better to rename some functions:

check_for_confirmed_flush_lsn() -> check_old_cluster_for_confirmed_flush_lsn()
check_for_lost_slots() -> check_old_cluster_for_lost_slots()

~~~

2.
+ /*
+ * Logical replication slots can be migrated since PG17. See comments atop
+ * get_logical_slot_infos().
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
+ {
+ check_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_for_confirmed_flush_lsn();
+ }
+

2a.
If my suggestions from v25-0002 [1] are adopted then this comment
needs to change to say like "See atop file pg_upgrade.c..."

~

2b.
Hmm. If my suggestions from v25-0002 [1] are adopted then the version
checking and the slot counting would *already* be in this calling
function. In that case, why can't this whole fragment be put in the
same place? E.g. IIUC there is no reason to call these at checks all
when the old_cluster slot count is already known to be 0. Similarly,
there is no reason that both these functions need to be independently
checking count_logical_slots again since we have already done that
(again, assuming my suggestions from v25-0002 [1] are adopted).

~~~

3. check_for_lost_slots

+/*
+ * Verify that all logical replication slots are usable.
+ */
+void
+check_for_lost_slots(void)

This was forward-declared to be static, but the static function
modifier is absent here.

~

4. check_for_lost_slots

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

AFAICT this quick exit can be removed. See my comment #2b.

~~~

5. check_for_confirmed_flush_lsn

+check_for_confirmed_flush_lsn(void)
+{
+ int i,
+ ntups,
+ i_slotname;
+ PGresult   *res;
+ DbInfo    *active_db = &old_cluster.dbarr.dbs[0];
+ PGconn    *conn;
+
+ /* Quick exit if the cluster does not have logical slots. */
+ if (count_logical_slots() == 0)
+ return;

AFAICT this quick exit can be removed. See my comment #2b.

======
.../t/003_logical_replication_slots.pl

6.
+# 2. Temporarily disable the subscription
+$subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub DISABLE");
 $old_publisher->stop;

In my previous 0003 review ([2] #10b) I was not questioning the need
for the $old_publisher->stop; before the pg_upgrade. I was only asking
why it was done at this location (after the DISABLE) instead of
earlier.

~~~

7.
+# Check whether changes on the new publisher get replicated to the subscriber
+$new_publisher->safe_psql('postgres',
+ "INSERT INTO tbl VALUES (generate_series(11, 20))");
+$new_publisher->wait_for_catchup('sub');
+$result = $subscriber->safe_psql('postgres', "SELECT count(*) FROM tbl");
+is($result, qq(20), 'check changes are shipped to the subscriber');

/shipped/replicated/

------
[1] My review of patch v25-0002 -
https://www.postgresql.org/message-id/CAHut%2BPtQcou3Bfm9A5SbhFuo2uKK-6u4_j_59so3skAi8Ns03A%40mail.gmail.com
[2] My review of v24-0003 -
https://www.postgresql.org/message-id/CAHut%2BPs5%3D9q1CCyrrytyv-8oUBqE6rv-%3DYFSRuuQwVf%2BsmC-Kw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia