Dear Peter,
Thank you for reviewing! New patch is available in [1].
> ======
> 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.
Fixed.
> 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
> });
Fixed.
> 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);
> }
In HEAD code, the pg_version seems "17devel". The string seemed smaller than 17 for Perl.
(i.e., "17devel" >= 17 means false)
For the purpose of comparing only the major version, pg_version->major was used.
Also, I removed the support for ~PG9.4. I cannot find descriptions, but according to [2],
Cluster.pm does not support such binaries.
(cluster_name is set when the server process is started, but the GUC has been added in PG9.5)
[1]:
https://www.postgresql.org/message-id/TYCPR01MB5870EBEBC89F5224F6B3788CF5D5A%40TYCPR01MB5870.jpnprd01.prod.outlook.com
[2]: https://www.postgresql.org/message-id/YsUrUDrRhUbuU/6k%40paquier.xyz
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
