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

On Wednesday, October 18, 2023 5:26 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@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