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

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