RE: POC: enable logical decoding when wal_level = 'replica' without a server restart - Mailing list pgsql-hackers

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_1bU2sg@mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED