Here are some comments for the patch v51-0002
======
src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
1.
+# Set max_wal_senders to a lower value if the old cluster is prior to PG12.
+# Such clusters regard max_wal_senders as part of max_connections, but the
+# current TAP tester sets these GUCs to the same value.
+if ($old_publisher->pg_version < 12)
+{
+ $old_publisher->append_conf('postgresql.conf', "max_wal_senders = 5");
+}
1a.
I was initially unsure what the above comment meant -- thanks for the
offline explanation.
SUGGESTION
The TAP Cluster.pm assigns default 'max_wal_senders' and
'max_connections' to the same value (10) but PG12 and prior considered
max_walsenders as a subset of max_connections, so setting the same
value will fail.
~
1b.
I also felt it is better to explicitly set both values in the < PG12
configuration because otherwise, you are still assuming knowledge that
the TAP default max_connections is 10.
SUGGESTION
$old_publisher->append_conf('postgresql.conf', qq{
max_wal_senders = 5
max_connections = 10
});
~~~
2.
+# Switch workloads depend on the major version of the old cluster. Upgrading
+# logical replication slots has been supported since PG17.
+if ($old_publisher->pg_version <= 16)
+{
+ test_for_16_and_prior($old_publisher, $new_publisher, $mode);
+}
+else
+{
+ test_for_17_and_later($old_publisher, $new_publisher, $mode);
+}
IMO it is less confusing to have fewer version numbers floating around
in comments and names and code. So instead of referring to 16 and 17,
how about just referring to 17 everywhere?
For example
SUGGESTION
# Test according to the major version of the old cluster.
# Upgrading logical replication slots has been supported only since PG17.
if ($old_publisher->pg_version >= 17)
{
test_upgrade_from_PG17_and_later($old_publisher, $new_publisher, $mode);
}
else
{
test_upgrade_from_pre_PG17($old_publisher, $new_publisher, $mode);
}
======
Kind Regards,
Peter Smith.
Fujitsu Australia
