On Wed, Oct 22, 2025 at 1:30 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Sawada-san,
>
> Thanks for updating the patch. Here are comments for v19.
> I spent sometime to find race condition but nothing found. Below were for test code.
Thank you for testing and reviewing the patch!
>
> ```
> # Copyright (c) 2024-2025, PostgreSQL Global Development Group
> ```
>
> 2024 can be removed.
Okay.
>
> ```
> # Create a temporary logical slot and exits without releasing it explicitly.
> # This enables logical decoding but skips disabling it and delegates to the
> # checkpointer.
> $primary->safe_psql('postgres',
> qq[select pg_create_logical_replication_slot('test_tmp_slot', 'test_decoding', true)]
> );
>
> # Wait for the checkpointer to disable logical decoding.
> wait_for_logical_decoding_deactivation($primary);
> ```
>
> Can we ensure the log has the line "requested disabling logical decoding"?
> The test looks like we spend checkpoint_timeout seconds here, but actually
> backend kicks the checkpointer.
Since we use the lazy behavior in all deactivation cases, the log
"requested disabling logical decoding" is written whenever we try to
disable logical decoding. I'm not sure adding the check improves the
test stability.
>
> ```
> $primary->adjust_conf('postgresql.conf', 'wal_level', 'minimal');
> ```
>
> To clarify, adjust_conf() is used for wal_level and max_wal_senders. Is there
> a reason you choose?
>
> ```
> my $res = run_log(
> [
> 'pg_ctl',
> '--pgdata' => $primary->data_dir,
> '--log' => $primary->logfile,
> 'start',
> ]);
> ok(!$res,
> "cannot server with wal_level='minimal' as there is in-use logical slot");
> ```
>
> We can use command_fails() to just confirm the command fails. $res is not needed
> if we use it.
>
> ```
> ok( $logfile =~
> qr/logical replication slot "test_slot" exists, but "wal_level" < "replica"/,
> 'logical slots requires logical decoding enabled at server startup');
> ```
>
> Per [1], we should use like() instead of ok(). Same thing can be said for other
> ok().
>
> ```
>
> ```
> $standby1->set_standby_mode();
> ```
>
> This seems not needed because init_from_backup() with has_streaming does the same
> thing. Same thing can be said for other standbys.
>
> ```
> # Initialize standby4 node and starts it with wal_level = 'logical'.
> my $standby4 = PostgreSQL::Test::Cluster->new('standby4');
> $standby4->init_from_backup($primary, 'my_backup', has_streaming => 1);
> $standby4->set_standby_mode();
> ```
>
> The comment looks not correct. Same thing can be said for standby5.
>
> ```
> # Check if we cannot use the invalidated slot on the standby.
> ($result, $stdout, $stderr) = $standby4->psql('postgres',
> qq[select pg_logical_slot_get_changes('standby4_slot', null, null)]);
> ok( $stderr =~
> /ERROR: can no longer access replication slot "standby4_slot"/,
> "cannot use invalidated logical slot");
> ```
>
> Not sure it should be tested in the file becasue 035 seems to verify that invalidated
> slots are no longer usable.
>
> ```
> # Test the abort process of logical decoding activation. We drop the the primary's
> # slot to decrease its effective_wal_level to 'replica'.
> ```
>
> s/drop the the primary's/drop the primary's/.
Agreed with the above comments.
I've attached a new patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
