RE: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

From Hayato Kuroda (Fujitsu)
Subject RE: [PoC] pg_upgrade: allow to upgrade publisher node
Date
Msg-id TYCPR01MB58705A6BF145154A251839C8F5D4A@TYCPR01MB5870.jpnprd01.prod.outlook.com
Whole thread Raw
In response to RE: [PoC] pg_upgrade: allow to upgrade publisher node  ("Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>)
List pgsql-hackers
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


pgsql-hackers by date:

Previous
From: "Hayato Kuroda (Fujitsu)"
Date:
Subject: RE: [PoC] pg_upgrade: allow to upgrade publisher node
Next
From: "Hayato Kuroda (Fujitsu)"
Date:
Subject: RE: [PoC] pg_upgrade: allow to upgrade publisher node