> On May 30, 2026, at 03:24, Masahiko Sawada <[email protected]> wrote:
>
> On Thu, May 28, 2026 at 8:39 PM Chao Li <[email protected]> wrote:
>>
>>
>>
>>> On May 29, 2026, at 05:53, Masahiko Sawada <[email protected]> wrote:
>>>
>>> On Thu, May 28, 2026 at 2:09 AM Chao Li <[email protected]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> While testing “Toggle logical decoding dynamically based on logical slot
>>>> presence”, I hit an assertion failure with concurrent logical slot
>>>> creation.
>>>>
>>>> This is a repo:
>>>>
>>>> 1. In session 1, attach the injection point locally and start creating a
>>>> logical slot. The session blocks at logical-decoding-activation:
>>>> ```
>>>> evantest=# set application_name = 'slot_a';
>>>> SET
>>>> evantest=# select injection_points_set_local();
>>>> injection_points_set_local
>>>> ----------------------------
>>>>
>>>> (1 row)
>>>> evantest=# select injection_points_attach('logical-decoding-activation',
>>>> 'wait');
>>>> injection_points_attach
>>>> -------------------------
>>>>
>>>> (1 row)
>>>> evantest=# select pg_create_logical_replication_slot('slot_a', 'pgoutput');
>>>> ```
>>>>
>>>> 2. In session 2, create another logical slot. This succeeds, and
>>>> effective_wal_level becomes logical:
>>>> ```
>>>> evantest=# select pg_create_logical_replication_slot('slot_b', 'pgoutput');
>>>> pg_create_logical_replication_slot
>>>> ------------------------------------
>>>> (slot_b,0/0902E418)
>>>> (1 row)
>>>>
>>>> evantest=# show effective_wal_level;
>>>> effective_wal_level
>>>> ---------------------
>>>> logical
>>>> (1 row)
>>>> ```
>>>>
>>>> 3. In session 2, cancel session 1 instead of waking it up:
>>>> ```
>>>> evantest=# select pg_cancel_backend(pid) from pg_stat_activity where
>>>> application_name = 'slot_a';
>>>> pg_cancel_backend
>>>> -------------------
>>>> t
>>>> (1 row)
>>>> ```
>>>>
>>>> Then the server hits this assertion:
>>>
>>> Thank you for the report! I've confirmed the problem.
>>>
>>>>
>>>> I might be over thinking, but I just feel the safest fix is to make
>>>> EnableLogicalDecoding() serialize. I tried serializing with
>>>> LogicalDecodingControlLock and with a separate lock, but both approaches
>>>> got deadlock around the barrier wait. I ended up with adding an
>>>> activation_in_progress flag in shared memory, protected by
>>>> LogicalDecodingControlLock, with a condition variable to wait for the
>>>> active activation to finish.
>>>>
>>>> With this fix, rerunning the repro makes session 2 wait while session 1 is
>>>> blocked at the injection point. After canceling session 1 from session 3,
>>>> session 2 continues, creates the slot successfully, and
>>>> effective_wal_level becomes logical.
>>>
>>> This serialization idea is very similar to what we tried in older
>>> version patches. While I've confirmed that the patch fixes the
>>> reported issue, I'm somewhat concerned that it could introduce another
>>> race condition as the patch adds a new mechanism.
>>>
>>> I think that the complication stems from the fact that
>>> abort_logical_decoding_activation() and DisableLogicalDecoding() clear
>>> the xlog_logical_info flag in different ways. I guess it would be
>>> simpler if we could delegate all the deactivation process including
>>> clearing xlog_logical_info flag to DisableLogicalDecoding(). It would
>>> allow the system to be in the state of xlog_logical_info == true and
>>> logical_decoding_enabled = false, but if we change
>>> DisableLogicalDecoding() to handle this case, the deactivation process
>>> would become simpler.
>>>
>>>> I didn’t include a test in this patch, as I wasn’t sure such a test would
>>>> be desirable. If others think it is worth adding, I can convert the repro
>>>> into a TAP test.
>>>
>>> I think we should have the tests.
>>>
>>> I've attached the patch for the above idea including the regression
>>> tests. Please review it.
>>>
>>
>> Yeah, your solution of deferring the actual abort to
>> DisableLogicalDecoding() is better. I have a few comments on the new patch:
>>
>
> Thank you for reviewing the patch!
>
>> 1
>> ```
>> + else
>> + {
>> + /*
>> + * Logical decoding is disabled while xlog_logical_info is
>> true.
>> + * This could happen if an activation is interrupted. Reset
>> only
>> + * xlog_logical_info.
>> + */
>> + LogicalDecodingCtl->xlog_logical_info = false;
>> + LogicalDecodingCtl->logical_decoding_enabled = false;
>> + }
>> ```
>>
>> Here, we should also clear LogicalDecodingCtl->pending_disable = false;
>
> Right.
>
>>
>> 2. AKAIK, critical section only works with elog, thus simple shared memory
>> assignments don't have to be inside the critical section. So we can move out
>> LogicalDecodingCtl assignments to outside the critical section, which avoids
>> some duplicate code.
>
> While it works, keeping these codes in a critical section is more
> consistent with what EnableLogicalDecoding() does. Please review the
> patch as I simplified the logic.
The new code looks good.
>
>>
>> 3
>> ```
>> + if (!in_recovery && was_enabled)
>> ereport(LOG,
>> errmsg("logical decoding is disabled because
>> there are no valid logical replication slots"));
>> ```
>>
>> I think this log message can be emitted after releasing the lock, which
>> makes the locking period a bit shorter. That might delay the log output
>> slightly, but since slot creation is not a frequent operation, I think that
>> should be fine.
>
> I'm rather concerned that "disabled" log could be written after
> "enabled" log even if the logical decoding gets enabled. I recall
> there was the same proposal before and we concluded to write the
> deactivation log under the lock. I added the comments so as not to
> confuse it.
>
Thanks for adding the comments which helps code reads better understand the
logic.
I’m still not convinced that emitting the log before releasing the lock is
necessary.
Even if there is a race, the disable path would release the lock and then emit
its log message immediately. The racing enable path would still need to acquire
the lock, set the state, write and flush the logical-decoding status WAL
record, release the lock, and only then emit the “enabled” log message. So the
misleading log order seems possible in theory, but unlikely in practice.
So, this is a trade-off. I think avoiding ereport(LOG) under the LWLock gains
more than loss, but I don’t think this point is critical enough to argue
further.
>>
>> 5. In the test, we may add more checks. For example, after the second slot
>> is created, we can check that the slot is created and “effective_wal_level"
>> has been set to “logical", which proves the concurrent creation is not
>> blocked.
>
> While I agree to have a check if effective_wal_level becomes 'logical'
> after creating 'test_slot2', why does it need to wait for the log
> "logical decoding is enabled upon creating a new logical replication
> slot"? When pg_create_logical_replication_slots() returns the result,
> the log must have been written in the server logs.
Yeah, I thought to also verify the log, but that’s kinda over engineering. I’m
good to remove the wait_for_log.
>
>> After cancelling, we can check that the cancellation happened and that
>> “effective_wal_level" is still “logical".
>
> Agreed.
>
>> I addressed all of these comments in the attached diff. Please take a look.
>
> Thank you for updating the patch!
>
> I've attached the updated patch.
>
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
> <v3-0001-Fix-race-when-logical-decoding-activation-is-conc.patch>
V3 overall looks good to me. A couple of small comments:
1
```
+ * Logging under the lock guarantees our "is disabled" message appear in
```
Here, “appear” maybe better to be “appears”.
2
```
+ # Canceling the backend should not affect the concurrent slot creation.
+ $primary->wait_for_log("canceling statement due to user request”);
```
While the final review, I just noticed the previous test cases also have
pg_cancel_backend(pid), so we may better to get the log offset otherwise
wait_for_log may get the log from a previous cancellation, like this:
```
diff --git a/src/test/recovery/t/051_effective_wal_level.pl
b/src/test/recovery/t/051_effective_wal_level.pl
index e3d8b97c173..0e75b1536a4 100644
--- a/src/test/recovery/t/051_effective_wal_level.pl
+++ b/src/test/recovery/t/051_effective_wal_level.pl
@@ -440,13 +440,14 @@ select
pg_create_logical_replication_slot('slot_canceled2', 'pgoutput');
# Cancel the backend initiated by $psql_create_slot, aborting its
activation
# process.
+ my $log_offset = -s $primary->logfile;
$primary->safe_psql(
'postgres',
qq[
select pg_cancel_backend(pid) from pg_stat_activity where query ~
'slot_canceled2' and pid <> pg_backend_pid()
]);
# Canceling the backend should not affect the concurrent slot creation.
- $primary->wait_for_log("canceling statement due to user request");
+ $primary->wait_for_log("canceling statement due to user request",
$log_offset);
test_wal_level($primary, "replica|logical",
"effective_wal_level remains 'logical' even
after the concurrent activation is interrupted");
}
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
