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

Notwithstanding the test errors I am getting for v24-0003, here are
some code review comments for this patch anyway.

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

1. check_for_lost_slots

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

1a.
AFAIK we don't ever need to call this also for 'new_cluster'. So the
function should have no parameter and just access 'old_cluster'
directly.

~

1b.
Can't this be a static function now?

~

2.
+ for (i = 0; i < ntups; i++)
+ pg_log(PG_WARNING,
+ "\nWARNING: logical replication slot \"%s\" is in 'lost' state.",
+ PQgetvalue(res, i, i_slotname));

Is it correct that this message also includes the word "WARNING"?
Other PG_WARNING messages don't do that.

~~~

3. check_for_confirmed_flush_lsn

+/*
+ * Verify that all logical replication slots consumed all WALs, except a
+ * CHECKPOINT_SHUTDOWN record.
+ */
+static void
+check_for_confirmed_flush_lsn(ClusterInfo *cluster)

AFAIK we don't ever need to call this also for 'new_cluster'. So the
function should have no parameter and just access 'old_cluster'
directly.

~

4.
+ for (i = 0; i < ntups; i++)
+ pg_log(PG_WARNING,
+ "\nWARNING: logical replication slot \"%s\" has not consumed WALs yet",
+ PQgetvalue(res, i, i_slotname));

Is it correct that this message also includes the word "WARNING"?
Other PG_WARNING messages don't do that.

======
src/bin/pg_upgrade/controldata.c

5. get_control_data

+ else if ((p = strstr(bufin, "Latest checkpoint location:")) != NULL)
+ {
+ /*
+ * Gather the latest checkpoint location if the cluster is PG17
+ * or later. This is used for upgrading logical replication
+ * slots.
+ */
+ if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)

But we are not "gathering" anything. It's just one LSN. I think this
ought to just say "Read the latest..."

~

6.
+ /*
+ * The upper and lower part of LSN must be read separately
+ * because it is reported in %X/%X format.
+ */

/reported/stored as/

======
src/bin/pg_upgrade/pg_upgrade.h

7.
+void check_for_lost_slots(ClusterInfo *cluster);\

Why is this needed here? Can't this be a static function?

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

8.
+# 2. Consume WAL records to avoid another type of upgrade failure. It will be
+# tested in subsequent cases.
+$old_publisher->safe_psql('postgres',
+ "SELECT count(*) FROM pg_logical_slot_get_changes('test_slot1', NULL, NULL);"
+);

I wondered if that step really needed. Why will there be WAL records to consume?

IIUC we haven't published anything yet.

~~~

9.
+# ------------------------------
+# TEST: Successful upgrade
+
+# Preparations for the subsequent test:
+# 1. Remove the remained slot
+$old_publisher->start;
+$old_publisher->safe_psql('postgres',
+ "SELECT * FROM pg_drop_replication_slot('test_slot1');"
+);

Should removal of the slot be done as part of the cleanup of the
previous test, instead of preparing for this one?

~~~

10.
# 3. Disable the subscription once
$subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub DISABLE");
$old_publisher->stop;

10a.
What do you mean by "once"?

~

10b.
That old_publisher->stop; seems strangely placed. Why is it here?

~~~

11.
# Check that the slot 'test_slot1' has migrated to the new cluster
$new_publisher->start;
my $result = $new_publisher->safe_psql('postgres',
"SELECT slot_name, two_phase FROM pg_replication_slots");
is($result, qq(sub|t), 'check the slot exists on new cluster');

~

That comment now seems wrong. That slot was previously removed, right?

~~~

12.
# Update the connection
my $new_connstr = $new_publisher->connstr . ' dbname=postgres';
$subscriber->safe_psql('postgres',
"ALTER SUBSCRIPTION sub CONNECTION '$new_connstr'");
$subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub ENABLE");

~

Maybe better to combine both SQL.

------
Kind Regards,
Peter Smith.
Fujitsu Australia