On Wednesday, October 18, 2023 5:26 PM Kuroda, Hayato/黒田 隼人 <kuroda.hay...@fujitsu.com> wrote: > > Thank you for reviewing! PSA new version. > Note that 0001 and 0002 are combined into one patch.
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. 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. 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 ? 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. Best Regards, Hou zj