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.
```
# Copyright (c) 2024-2025, PostgreSQL Global Development Group
```
2024 can be removed.
```
# 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.
```
$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/.
[1]:
https://www.postgresql.org/message-id/caff0-chhwnx_cv2uy7tkjodubeogprjpw4rpf1jqb16_1bu...@mail.gmail.com
Best regards,
Hayato Kuroda
FUJITSU LIMITED
