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

On Friday, October 20, 2023 9:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
> 
> Here are some review comments for v54-0001

Thanks for the review.

> 
> ======
> src/backend/replication/slot.c
> 
> 1.
> + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade) {
> + ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("replication slots must not be invalidated during the
> + upgrade"), errhint("\"max_slot_wal_keep_size\" must not be set to -1
> + during the
> upgrade"));
> + }
> 
> This new error is replacing the old code:
> + Assert(max_slot_wal_keep_size_mb == -1);
> 
> Is that errhint correct? Shouldn't it say "must" instead of "must not"?

Fixed.

> 
> ======
> src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
> 
> 2. General formating
> 
> Some of the "]);" formatting and indenting for the multiple SQL commands is
> inconsistent.
> 
> For example,
> 
> + $old_publisher->safe_psql(
> + 'postgres', qq[
> + SELECT pg_create_logical_replication_slot('test_slot1',
> + 'test_decoding'); SELECT
> + pg_create_logical_replication_slot('test_slot2', 'test_decoding'); ]
> + );
> 
> versus
> 
> + $old_publisher->safe_psql(
> + 'postgres', qq[
> + CREATE TABLE tbl AS SELECT generate_series(1, 10) AS a; SELECT
> + pg_replication_slot_advance('test_slot2', NULL); SELECT count(*) FROM
> + pg_logical_emit_message('false', 'prefix',
> 'This is a non-transactional message');
> + ]);
> 

Fixed.

> ~~~
> 
> 3.
> +# Set up some settings for the old cluster, so that we can ensures that
> +initdb # will be done.
> +my @initdb_params = ();
> +push @initdb_params, ('--encoding', 'UTF-8'); push @initdb_params,
> +('--locale', 'C'); $node_params{extra} = \@initdb_params;
> +
> +$old_publisher->init(%node_params);
> 
> Why would initdb not be done if these were not set? I didn't understand the
> comment.
> 
> /so that we can ensures/to ensure/

The node->init() will use a previously initialized cluster if no parameter was
specified, but that cluster could be of wrong version when doing cross-version
test, so we set something to let the initdb happen.

I added some explanation in the comment.

> ~~~
> 
> 4.
> +# XXX: For PG9.6 and prior, the TAP Cluster.pm assigns
> +'max_wal_senders' and # 'max_connections' to the same value (10). But
> +these versions considered # max_wal_senders as a subset of
> +max_connections, so setting the same value # will fail. This adjustment
> +will not be needed when packages for older #versions are defined.
> +if ($old_publisher->pg_version->major <= 9.6) {
> +$old_publisher->append_conf(  'postgresql.conf', qq[  max_wal_senders =
> +5  max_connections = 10  ]); }
> 
> 4a.
> IMO remove the complicated comment trying to explain the problem and just
> to unconditionally set the values you want.
> 
> SUGGESTION#1
> # Older PG version had different rules for the inter-dependency of
> 'max_wal_senders' and 'max_connections', # so assign values which will work
> for all PG versions.
> $old_publisher->append_conf(
>   'postgresql.conf', qq[
>   max_wal_senders = 5
>   max_connections = 10
>   ]);
> 
> ~~

As Kuroda-san mentioned, we may fix Cluster.pm later, so I kept the XXX comment
but simplify it based on your suggestion.

Attach the new version patch.

Best Regards,
Hou zj