Dear Hou,
Thanks for reviewing! New patch can be available in [1].
> Thanks for updating the patch, here are few comments for the test.
>
> 1.
>
> >
> # 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.
> if ($old_publisher->pg_version->major < 12)
> {
> $old_publisher->append_conf(
> 'postgresql.conf', qq[
> max_wal_senders = 5
> max_connections = 10
> ]);
> >
>
> I think we already set max_wal_senders to 5 in init() function(in Cluster.pm),
> so is this necessary ? And 002_pg_upgrade.pl doesn't seems set this.
I thought you mentioned about Cluster::V_11::init(). I analyzed based on that
and
found a fault. Could you please check [1]?
> 2.
>
> SELECT pg_create_logical_replication_slot('test_slot1',
> 'test_decoding', false, true);
> SELECT pg_create_logical_replication_slot('test_slot2',
> 'test_decoding', false, true);
>
> I think we don't need to set the last two parameters here as we don't check
> these info in the tests.
Removed.
> 3.
>
> # Set extra params if cross-version checks are required. This is needed to
> # avoid using previously initdb'd cluster
> if (defined($ENV{oldinstall}))
> {
> my @initdb_params = ();
> push @initdb_params, ('--encoding', 'UTF-8');
> push @initdb_params, ('--locale', 'C');
>
> I am not sure I understand the comment, would it be possible provide a bit
> more
> explanation about the purpose of this setting ? And I see 002_pg_upgrade
> always
> have these setting even if oldinstall is not defined, so shall we follow the
> same ?
Fixed.
Actually settings are not needed for new cluster, but seems better to follow
002.
> 4.
>
> + command_ok(
> + [
> + 'pg_upgrade', '--no-sync',
> + '-d', $old_publisher->data_dir,
> + '-D', $new_publisher->data_dir,
> + '-b', $oldbindir,
> + '-B', $newbindir,
> + '-s', $new_publisher->host,
> + '-p', $old_publisher->port,
> + '-P', $new_publisher->port,
> + $mode,
> + ],
>
> I think all the pg_upgrade commands in the test are the same, so we can save
> the
> cmd
> in a variable and pass them to command_xx(). I think it can save some effort
> to
> check the difference of each command and can also reduce some codes.
Fixed.
[1]:
https://www.postgresql.org/message-id/TYCPR01MB587007EA2F9AB92F0E1F5957F5D4A%40TYCPR01MB5870.jpnprd01.prod.outlook.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
