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