public inbox for [email protected]
help / color / mirror / Atom feedFix race in ReplicationSlotRelease for ephemeral slots
27+ messages / 5 participants
[nested] [flat]
* Fix race in ReplicationSlotRelease for ephemeral slots
@ 2026-05-27 11:50 Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 08:09 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
0 siblings, 2 replies; 27+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-05-27 11:50 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Hi,
While testing the slot release logic, I noticed a bug in
ReplicationSlotRelease() where it may access a replication slot array entry that
has already been released by itself.
The detail is: When releasing an ephemeral replication slot,
ReplicationSlotRelease() first drops the slot via ReplicationSlotDropAcquired().
After this point, the slot's shared memory slot array entry can be immediately
reused by another backend creating a new slot.
However, ReplicationSlotRelease() continued executing common cleanup code that
still dereferenced the old slot pointer and updated shared memory fields such as
effective_xmin. If the slot array entry had already been reallocated, these
writes could inadvertently affect a different, unrelated slot.
I am attaching a patch that avoids touching slot shared-memory state after
dropping an ephemeral slot. Keep the post-release shared-memory updates only for
non-ephemeral slots, where the slot remains valid after release.
To reproduce, we can use the following steps:
1. Attach gdb to the backend and set a breakpoint in ReplicationSlotRelease()
right after ReplicationSlotDropAcquired() is called.
2. Create an ephemeral slot in the above backend with an invalid output plugin:
SELECT pg_create_logical_replication_slot('test_slot_dropped', 'pgoutput2', false, false, true);
3. Once the breakpoint is hit, start another backend and create a new slot
named 'test_slot_created'.
4. Release the breakpoint and allow the first backend to continue. At this
point, you will see it updating the new slot 'test_slot_created' -> active_proc
(and effective_xmin, if a snapshot is being exported) to invalid values.
5. Start a third backend and attempt to acquire the same slot
'test_slot_created' ? this should not be possible under normal circumstances,
but the bug allows it.
I haven't attached a test for this fix, as the change is straightforward and the
likelihood of encountering this bug is low, so it may not be worth adding test
cycles for it. However, if others feel differently, I'm OK to add one.
Best Regards,
Hou zj
Attachments:
[application/octet-stream] v1-0001-Fix-race-in-ReplicationSlotRelease-for-ephemeral-.patch (3.9K, 2-v1-0001-Fix-race-in-ReplicationSlotRelease-for-ephemeral-.patch)
download | inline diff:
From d58d49e585abf4f1c2cc29de172dcb33595017d7 Mon Sep 17 00:00:00 2001
From: Zhijie Hou <[email protected]>
Date: Wed, 27 May 2026 18:24:39 +0800
Subject: [PATCH v1] Fix race in ReplicationSlotRelease for ephemeral slots
When releasing an ephemeral replication slot, ReplicationSlotRelease() first
drops the slot via ReplicationSlotDropAcquired(). After this point, the slot's
shared memory slot array entry can be immediately reused by another backend
creating a new slot.
However, ReplicationSlotRelease() continued executing common cleanup code that
still dereferenced the old slot pointer and updated shared memory fields such as
effective_xmin. If the slot array entry had already been reallocated, these
writes could inadvertently affect a different, unrelated slot.
This commit avoids touching slot shared-memory state after dropping an ephemeral
slot. Keep the post-release shared-memory updates only for non-ephemeral slots,
where the slot remains valid after release.
---
src/backend/replication/slot.c | 68 +++++++++++++++++-----------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c0c9f514f7b..37745867930 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -797,44 +797,46 @@ ReplicationSlotRelease(void)
if (is_logical)
RequestDisableLogicalDecoding();
}
-
- /*
- * If slot needed to temporarily restrain both data and catalog xmin to
- * create the catalog snapshot, remove that temporary constraint.
- * Snapshots can only be exported while the initial snapshot is still
- * acquired.
- */
- if (!TransactionIdIsValid(slot->data.xmin) &&
- TransactionIdIsValid(slot->effective_xmin))
+ else
{
- SpinLockAcquire(&slot->mutex);
- slot->effective_xmin = InvalidTransactionId;
- SpinLockRelease(&slot->mutex);
- ReplicationSlotsComputeRequiredXmin(false);
- }
-
- /*
- * Set the time since the slot has become inactive. We get the current
- * time beforehand to avoid system call while holding the spinlock.
- */
- now = GetCurrentTimestamp();
+ /*
+ * If slot needed to temporarily restrain both data and catalog xmin
+ * to create the catalog snapshot, remove that temporary constraint.
+ * Snapshots can only be exported while the initial snapshot is still
+ * acquired.
+ */
+ if (!TransactionIdIsValid(slot->data.xmin) &&
+ TransactionIdIsValid(slot->effective_xmin))
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->effective_xmin = InvalidTransactionId;
+ SpinLockRelease(&slot->mutex);
+ ReplicationSlotsComputeRequiredXmin(false);
+ }
- if (slot->data.persistency == RS_PERSISTENT)
- {
/*
- * Mark persistent slot inactive. We're not freeing it, just
- * disconnecting, but wake up others that may be waiting for it.
+ * Set the time since the slot has become inactive. We get the current
+ * time beforehand to avoid system call while holding the spinlock.
*/
- SpinLockAcquire(&slot->mutex);
- slot->active_proc = INVALID_PROC_NUMBER;
- ReplicationSlotSetInactiveSince(slot, now, false);
- SpinLockRelease(&slot->mutex);
- ConditionVariableBroadcast(&slot->active_cv);
- }
- else
- ReplicationSlotSetInactiveSince(slot, now, true);
+ now = GetCurrentTimestamp();
- MyReplicationSlot = NULL;
+ if (slot->data.persistency == RS_PERSISTENT)
+ {
+ /*
+ * Mark persistent slot inactive. We're not freeing it, just
+ * disconnecting, but wake up others that may be waiting for it.
+ */
+ SpinLockAcquire(&slot->mutex);
+ slot->active_proc = INVALID_PROC_NUMBER;
+ ReplicationSlotSetInactiveSince(slot, now, false);
+ SpinLockRelease(&slot->mutex);
+ ConditionVariableBroadcast(&slot->active_cv);
+ }
+ else
+ ReplicationSlotSetInactiveSince(slot, now, true);
+
+ MyReplicationSlot = NULL;
+ }
/* might not have been set when we've been a plain slot */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
--
2.43.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
@ 2026-05-29 08:09 ` Fujii Masao <[email protected]>
2026-05-29 13:31 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
1 sibling, 1 reply; 27+ messages in thread
From: Fujii Masao @ 2026-05-29 08:09 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Wed, May 27, 2026 at 8:50 PM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> Hi,
>
> While testing the slot release logic, I noticed a bug in
> ReplicationSlotRelease() where it may access a replication slot array entry that
> has already been released by itself.
>
> The detail is: When releasing an ephemeral replication slot,
> ReplicationSlotRelease() first drops the slot via ReplicationSlotDropAcquired().
> After this point, the slot's shared memory slot array entry can be immediately
> reused by another backend creating a new slot.
>
> However, ReplicationSlotRelease() continued executing common cleanup code that
> still dereferenced the old slot pointer and updated shared memory fields such as
> effective_xmin. If the slot array entry had already been reallocated, these
> writes could inadvertently affect a different, unrelated slot.
Good catch!
> I am attaching a patch that avoids touching slot shared-memory state after
> dropping an ephemeral slot. Keep the post-release shared-memory updates only for
> non-ephemeral slots, where the slot remains valid after release.
Thanks for the patch! It looks good to me.
Barring any objections, I will commit it.
> I haven't attached a test for this fix, as the change is straightforward and the
> likelihood of encountering this bug is low, so it may not be worth adding test
> cycles for it.
+1
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 08:09 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
@ 2026-05-29 13:31 ` Fujii Masao <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Fujii Masao @ 2026-05-29 13:31 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Fri, May 29, 2026 at 5:09 PM Fujii Masao <[email protected]> wrote:
> > I am attaching a patch that avoids touching slot shared-memory state after
> > dropping an ephemeral slot. Keep the post-release shared-memory updates only for
> > non-ephemeral slots, where the slot remains valid after release.
>
> Thanks for the patch! It looks good to me.
> Barring any objections, I will commit it.
Since this fix should be backpatched to all supported branches,
I prepared patches for them. Attached.
Regards,
--
Fujii Masao
From 56e8c1068e19c3ccd41cecb75b1ee26affb52abf Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Fri, 29 May 2026 18:43:50 +0900
Subject: [PATCH v1] Fix race in ReplicationSlotRelease for ephemeral slots
When releasing an ephemeral replication slot, ReplicationSlotRelease() first
drops the slot via ReplicationSlotDropAcquired(). After this point, the slot's
shared memory slot array entry can be immediately reused by another backend
creating a new slot.
However, ReplicationSlotRelease() continued executing common cleanup code that
still dereferenced the old slot pointer and updated shared memory fields such as
effective_xmin. If the slot array entry had already been reallocated, these
writes could inadvertently affect a different, unrelated slot.
This commit avoids touching slot shared-memory state after dropping an ephemeral
slot. Keep the post-release shared-memory updates only for non-ephemeral slots,
where the slot remains valid after release.
---
src/backend/replication/slot.c | 68 +++++++++++++++++-----------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 4246d0a51e1..e60ccc424dd 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -737,44 +737,46 @@ ReplicationSlotRelease(void)
*/
ReplicationSlotDropAcquired();
}
-
- /*
- * If slot needed to temporarily restrain both data and catalog xmin to
- * create the catalog snapshot, remove that temporary constraint.
- * Snapshots can only be exported while the initial snapshot is still
- * acquired.
- */
- if (!TransactionIdIsValid(slot->data.xmin) &&
- TransactionIdIsValid(slot->effective_xmin))
+ else
{
- SpinLockAcquire(&slot->mutex);
- slot->effective_xmin = InvalidTransactionId;
- SpinLockRelease(&slot->mutex);
- ReplicationSlotsComputeRequiredXmin(false);
- }
-
- /*
- * Set the time since the slot has become inactive. We get the current
- * time beforehand to avoid system call while holding the spinlock.
- */
- now = GetCurrentTimestamp();
+ /*
+ * If slot needed to temporarily restrain both data and catalog xmin
+ * to create the catalog snapshot, remove that temporary constraint.
+ * Snapshots can only be exported while the initial snapshot is still
+ * acquired.
+ */
+ if (!TransactionIdIsValid(slot->data.xmin) &&
+ TransactionIdIsValid(slot->effective_xmin))
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->effective_xmin = InvalidTransactionId;
+ SpinLockRelease(&slot->mutex);
+ ReplicationSlotsComputeRequiredXmin(false);
+ }
- if (slot->data.persistency == RS_PERSISTENT)
- {
/*
- * Mark persistent slot inactive. We're not freeing it, just
- * disconnecting, but wake up others that may be waiting for it.
+ * Set the time since the slot has become inactive. We get the current
+ * time beforehand to avoid system call while holding the spinlock.
*/
- SpinLockAcquire(&slot->mutex);
- slot->active_pid = 0;
- ReplicationSlotSetInactiveSince(slot, now, false);
- SpinLockRelease(&slot->mutex);
- ConditionVariableBroadcast(&slot->active_cv);
- }
- else
- ReplicationSlotSetInactiveSince(slot, now, true);
+ now = GetCurrentTimestamp();
- MyReplicationSlot = NULL;
+ if (slot->data.persistency == RS_PERSISTENT)
+ {
+ /*
+ * Mark persistent slot inactive. We're not freeing it, just
+ * disconnecting, but wake up others that may be waiting for it.
+ */
+ SpinLockAcquire(&slot->mutex);
+ slot->active_pid = 0;
+ ReplicationSlotSetInactiveSince(slot, now, false);
+ SpinLockRelease(&slot->mutex);
+ ConditionVariableBroadcast(&slot->active_cv);
+ }
+ else
+ ReplicationSlotSetInactiveSince(slot, now, true);
+
+ MyReplicationSlot = NULL;
+ }
/* might not have been set when we've been a plain slot */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
--
2.53.0
From 8ee2b85c2a68e6efc68e517e0d20e6cbc96eadbd Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Fri, 29 May 2026 18:43:50 +0900
Subject: [PATCH v1] Fix race in ReplicationSlotRelease for ephemeral slots
When releasing an ephemeral replication slot, ReplicationSlotRelease() first
drops the slot via ReplicationSlotDropAcquired(). After this point, the slot's
shared memory slot array entry can be immediately reused by another backend
creating a new slot.
However, ReplicationSlotRelease() continued executing common cleanup code that
still dereferenced the old slot pointer and updated shared memory fields such as
effective_xmin. If the slot array entry had already been reallocated, these
writes could inadvertently affect a different, unrelated slot.
This commit avoids touching slot shared-memory state after dropping an ephemeral
slot. Keep the post-release shared-memory updates only for non-ephemeral slots,
where the slot remains valid after release.
---
src/backend/replication/slot.c | 52 ++++++++++++++++++----------------
1 file changed, 27 insertions(+), 25 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index aa017451ccc..2034722f0a4 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -589,35 +589,37 @@ ReplicationSlotRelease(void)
*/
ReplicationSlotDropAcquired();
}
-
- /*
- * If slot needed to temporarily restrain both data and catalog xmin to
- * create the catalog snapshot, remove that temporary constraint.
- * Snapshots can only be exported while the initial snapshot is still
- * acquired.
- */
- if (!TransactionIdIsValid(slot->data.xmin) &&
- TransactionIdIsValid(slot->effective_xmin))
- {
- SpinLockAcquire(&slot->mutex);
- slot->effective_xmin = InvalidTransactionId;
- SpinLockRelease(&slot->mutex);
- ReplicationSlotsComputeRequiredXmin(false);
- }
-
- if (slot->data.persistency == RS_PERSISTENT)
+ else
{
/*
- * Mark persistent slot inactive. We're not freeing it, just
- * disconnecting, but wake up others that may be waiting for it.
+ * If slot needed to temporarily restrain both data and catalog xmin
+ * to create the catalog snapshot, remove that temporary constraint.
+ * Snapshots can only be exported while the initial snapshot is still
+ * acquired.
*/
- SpinLockAcquire(&slot->mutex);
- slot->active_pid = 0;
- SpinLockRelease(&slot->mutex);
- ConditionVariableBroadcast(&slot->active_cv);
- }
+ if (!TransactionIdIsValid(slot->data.xmin) &&
+ TransactionIdIsValid(slot->effective_xmin))
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->effective_xmin = InvalidTransactionId;
+ SpinLockRelease(&slot->mutex);
+ ReplicationSlotsComputeRequiredXmin(false);
+ }
- MyReplicationSlot = NULL;
+ if (slot->data.persistency == RS_PERSISTENT)
+ {
+ /*
+ * Mark persistent slot inactive. We're not freeing it, just
+ * disconnecting, but wake up others that may be waiting for it.
+ */
+ SpinLockAcquire(&slot->mutex);
+ slot->active_pid = 0;
+ SpinLockRelease(&slot->mutex);
+ ConditionVariableBroadcast(&slot->active_cv);
+ }
+
+ MyReplicationSlot = NULL;
+ }
/* might not have been set when we've been a plain slot */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
--
2.53.0
From 28da67621c97824730030094cdfd1ad02b2655e8 Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Fri, 29 May 2026 18:43:50 +0900
Subject: [PATCH v1] Fix race in ReplicationSlotRelease for ephemeral slots
When releasing an ephemeral replication slot, ReplicationSlotRelease() first
drops the slot via ReplicationSlotDropAcquired(). After this point, the slot's
shared memory slot array entry can be immediately reused by another backend
creating a new slot.
However, ReplicationSlotRelease() continued executing common cleanup code that
still dereferenced the old slot pointer and updated shared memory fields such as
effective_xmin. If the slot array entry had already been reallocated, these
writes could inadvertently affect a different, unrelated slot.
This commit avoids touching slot shared-memory state after dropping an ephemeral
slot. Keep the post-release shared-memory updates only for non-ephemeral slots,
where the slot remains valid after release.
---
src/backend/replication/slot.c | 76 +++++++++++++++++-----------------
1 file changed, 39 insertions(+), 37 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 4e54a8d0f14..2f1696f4111 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -702,48 +702,50 @@ ReplicationSlotRelease(void)
*/
ReplicationSlotDropAcquired();
}
-
- /*
- * If slot needed to temporarily restrain both data and catalog xmin to
- * create the catalog snapshot, remove that temporary constraint.
- * Snapshots can only be exported while the initial snapshot is still
- * acquired.
- */
- if (!TransactionIdIsValid(slot->data.xmin) &&
- TransactionIdIsValid(slot->effective_xmin))
+ else
{
- SpinLockAcquire(&slot->mutex);
- slot->effective_xmin = InvalidTransactionId;
- SpinLockRelease(&slot->mutex);
- ReplicationSlotsComputeRequiredXmin(false);
- }
-
- /*
- * Set the time since the slot has become inactive. We get the current
- * time beforehand to avoid system call while holding the spinlock.
- */
- now = GetCurrentTimestamp();
+ /*
+ * If slot needed to temporarily restrain both data and catalog xmin
+ * to create the catalog snapshot, remove that temporary constraint.
+ * Snapshots can only be exported while the initial snapshot is still
+ * acquired.
+ */
+ if (!TransactionIdIsValid(slot->data.xmin) &&
+ TransactionIdIsValid(slot->effective_xmin))
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->effective_xmin = InvalidTransactionId;
+ SpinLockRelease(&slot->mutex);
+ ReplicationSlotsComputeRequiredXmin(false);
+ }
- if (slot->data.persistency == RS_PERSISTENT)
- {
/*
- * Mark persistent slot inactive. We're not freeing it, just
- * disconnecting, but wake up others that may be waiting for it.
+ * Set the time since the slot has become inactive. We get the current
+ * time beforehand to avoid system call while holding the spinlock.
*/
- SpinLockAcquire(&slot->mutex);
- slot->active_pid = 0;
- slot->inactive_since = now;
- SpinLockRelease(&slot->mutex);
- ConditionVariableBroadcast(&slot->active_cv);
- }
- else
- {
- SpinLockAcquire(&slot->mutex);
- slot->inactive_since = now;
- SpinLockRelease(&slot->mutex);
- }
+ now = GetCurrentTimestamp();
- MyReplicationSlot = NULL;
+ if (slot->data.persistency == RS_PERSISTENT)
+ {
+ /*
+ * Mark persistent slot inactive. We're not freeing it, just
+ * disconnecting, but wake up others that may be waiting for it.
+ */
+ SpinLockAcquire(&slot->mutex);
+ slot->active_pid = 0;
+ slot->inactive_since = now;
+ SpinLockRelease(&slot->mutex);
+ ConditionVariableBroadcast(&slot->active_cv);
+ }
+ else
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->inactive_since = now;
+ SpinLockRelease(&slot->mutex);
+ }
+
+ MyReplicationSlot = NULL;
+ }
/* might not have been set when we've been a plain slot */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
--
2.53.0
Attachments:
[text/plain] pg18-v1-0001-Fix-race-in-ReplicationSlotRelease-for-ephemeral-.txt (3.8K, 2-pg18-v1-0001-Fix-race-in-ReplicationSlotRelease-for-ephemeral-.txt)
download | inline diff:
From 56e8c1068e19c3ccd41cecb75b1ee26affb52abf Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Fri, 29 May 2026 18:43:50 +0900
Subject: [PATCH v1] Fix race in ReplicationSlotRelease for ephemeral slots
When releasing an ephemeral replication slot, ReplicationSlotRelease() first
drops the slot via ReplicationSlotDropAcquired(). After this point, the slot's
shared memory slot array entry can be immediately reused by another backend
creating a new slot.
However, ReplicationSlotRelease() continued executing common cleanup code that
still dereferenced the old slot pointer and updated shared memory fields such as
effective_xmin. If the slot array entry had already been reallocated, these
writes could inadvertently affect a different, unrelated slot.
This commit avoids touching slot shared-memory state after dropping an ephemeral
slot. Keep the post-release shared-memory updates only for non-ephemeral slots,
where the slot remains valid after release.
---
src/backend/replication/slot.c | 68 +++++++++++++++++-----------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 4246d0a51e1..e60ccc424dd 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -737,44 +737,46 @@ ReplicationSlotRelease(void)
*/
ReplicationSlotDropAcquired();
}
-
- /*
- * If slot needed to temporarily restrain both data and catalog xmin to
- * create the catalog snapshot, remove that temporary constraint.
- * Snapshots can only be exported while the initial snapshot is still
- * acquired.
- */
- if (!TransactionIdIsValid(slot->data.xmin) &&
- TransactionIdIsValid(slot->effective_xmin))
+ else
{
- SpinLockAcquire(&slot->mutex);
- slot->effective_xmin = InvalidTransactionId;
- SpinLockRelease(&slot->mutex);
- ReplicationSlotsComputeRequiredXmin(false);
- }
-
- /*
- * Set the time since the slot has become inactive. We get the current
- * time beforehand to avoid system call while holding the spinlock.
- */
- now = GetCurrentTimestamp();
+ /*
+ * If slot needed to temporarily restrain both data and catalog xmin
+ * to create the catalog snapshot, remove that temporary constraint.
+ * Snapshots can only be exported while the initial snapshot is still
+ * acquired.
+ */
+ if (!TransactionIdIsValid(slot->data.xmin) &&
+ TransactionIdIsValid(slot->effective_xmin))
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->effective_xmin = InvalidTransactionId;
+ SpinLockRelease(&slot->mutex);
+ ReplicationSlotsComputeRequiredXmin(false);
+ }
- if (slot->data.persistency == RS_PERSISTENT)
- {
/*
- * Mark persistent slot inactive. We're not freeing it, just
- * disconnecting, but wake up others that may be waiting for it.
+ * Set the time since the slot has become inactive. We get the current
+ * time beforehand to avoid system call while holding the spinlock.
*/
- SpinLockAcquire(&slot->mutex);
- slot->active_pid = 0;
- ReplicationSlotSetInactiveSince(slot, now, false);
- SpinLockRelease(&slot->mutex);
- ConditionVariableBroadcast(&slot->active_cv);
- }
- else
- ReplicationSlotSetInactiveSince(slot, now, true);
+ now = GetCurrentTimestamp();
- MyReplicationSlot = NULL;
+ if (slot->data.persistency == RS_PERSISTENT)
+ {
+ /*
+ * Mark persistent slot inactive. We're not freeing it, just
+ * disconnecting, but wake up others that may be waiting for it.
+ */
+ SpinLockAcquire(&slot->mutex);
+ slot->active_pid = 0;
+ ReplicationSlotSetInactiveSince(slot, now, false);
+ SpinLockRelease(&slot->mutex);
+ ConditionVariableBroadcast(&slot->active_cv);
+ }
+ else
+ ReplicationSlotSetInactiveSince(slot, now, true);
+
+ MyReplicationSlot = NULL;
+ }
/* might not have been set when we've been a plain slot */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
--
2.53.0
[text/plain] pg14-pg16-v1-0001-Fix-race-in-ReplicationSlotRelease-for-ephemeral-.txt (3.2K, 3-pg14-pg16-v1-0001-Fix-race-in-ReplicationSlotRelease-for-ephemeral-.txt)
download | inline diff:
From 8ee2b85c2a68e6efc68e517e0d20e6cbc96eadbd Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Fri, 29 May 2026 18:43:50 +0900
Subject: [PATCH v1] Fix race in ReplicationSlotRelease for ephemeral slots
When releasing an ephemeral replication slot, ReplicationSlotRelease() first
drops the slot via ReplicationSlotDropAcquired(). After this point, the slot's
shared memory slot array entry can be immediately reused by another backend
creating a new slot.
However, ReplicationSlotRelease() continued executing common cleanup code that
still dereferenced the old slot pointer and updated shared memory fields such as
effective_xmin. If the slot array entry had already been reallocated, these
writes could inadvertently affect a different, unrelated slot.
This commit avoids touching slot shared-memory state after dropping an ephemeral
slot. Keep the post-release shared-memory updates only for non-ephemeral slots,
where the slot remains valid after release.
---
src/backend/replication/slot.c | 52 ++++++++++++++++++----------------
1 file changed, 27 insertions(+), 25 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index aa017451ccc..2034722f0a4 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -589,35 +589,37 @@ ReplicationSlotRelease(void)
*/
ReplicationSlotDropAcquired();
}
-
- /*
- * If slot needed to temporarily restrain both data and catalog xmin to
- * create the catalog snapshot, remove that temporary constraint.
- * Snapshots can only be exported while the initial snapshot is still
- * acquired.
- */
- if (!TransactionIdIsValid(slot->data.xmin) &&
- TransactionIdIsValid(slot->effective_xmin))
- {
- SpinLockAcquire(&slot->mutex);
- slot->effective_xmin = InvalidTransactionId;
- SpinLockRelease(&slot->mutex);
- ReplicationSlotsComputeRequiredXmin(false);
- }
-
- if (slot->data.persistency == RS_PERSISTENT)
+ else
{
/*
- * Mark persistent slot inactive. We're not freeing it, just
- * disconnecting, but wake up others that may be waiting for it.
+ * If slot needed to temporarily restrain both data and catalog xmin
+ * to create the catalog snapshot, remove that temporary constraint.
+ * Snapshots can only be exported while the initial snapshot is still
+ * acquired.
*/
- SpinLockAcquire(&slot->mutex);
- slot->active_pid = 0;
- SpinLockRelease(&slot->mutex);
- ConditionVariableBroadcast(&slot->active_cv);
- }
+ if (!TransactionIdIsValid(slot->data.xmin) &&
+ TransactionIdIsValid(slot->effective_xmin))
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->effective_xmin = InvalidTransactionId;
+ SpinLockRelease(&slot->mutex);
+ ReplicationSlotsComputeRequiredXmin(false);
+ }
- MyReplicationSlot = NULL;
+ if (slot->data.persistency == RS_PERSISTENT)
+ {
+ /*
+ * Mark persistent slot inactive. We're not freeing it, just
+ * disconnecting, but wake up others that may be waiting for it.
+ */
+ SpinLockAcquire(&slot->mutex);
+ slot->active_pid = 0;
+ SpinLockRelease(&slot->mutex);
+ ConditionVariableBroadcast(&slot->active_cv);
+ }
+
+ MyReplicationSlot = NULL;
+ }
/* might not have been set when we've been a plain slot */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
--
2.53.0
[text/plain] pg17-v1-0001-Fix-race-in-ReplicationSlotRelease-for-ephemeral-.txt (3.9K, 4-pg17-v1-0001-Fix-race-in-ReplicationSlotRelease-for-ephemeral-.txt)
download | inline diff:
From 28da67621c97824730030094cdfd1ad02b2655e8 Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Fri, 29 May 2026 18:43:50 +0900
Subject: [PATCH v1] Fix race in ReplicationSlotRelease for ephemeral slots
When releasing an ephemeral replication slot, ReplicationSlotRelease() first
drops the slot via ReplicationSlotDropAcquired(). After this point, the slot's
shared memory slot array entry can be immediately reused by another backend
creating a new slot.
However, ReplicationSlotRelease() continued executing common cleanup code that
still dereferenced the old slot pointer and updated shared memory fields such as
effective_xmin. If the slot array entry had already been reallocated, these
writes could inadvertently affect a different, unrelated slot.
This commit avoids touching slot shared-memory state after dropping an ephemeral
slot. Keep the post-release shared-memory updates only for non-ephemeral slots,
where the slot remains valid after release.
---
src/backend/replication/slot.c | 76 +++++++++++++++++-----------------
1 file changed, 39 insertions(+), 37 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 4e54a8d0f14..2f1696f4111 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -702,48 +702,50 @@ ReplicationSlotRelease(void)
*/
ReplicationSlotDropAcquired();
}
-
- /*
- * If slot needed to temporarily restrain both data and catalog xmin to
- * create the catalog snapshot, remove that temporary constraint.
- * Snapshots can only be exported while the initial snapshot is still
- * acquired.
- */
- if (!TransactionIdIsValid(slot->data.xmin) &&
- TransactionIdIsValid(slot->effective_xmin))
+ else
{
- SpinLockAcquire(&slot->mutex);
- slot->effective_xmin = InvalidTransactionId;
- SpinLockRelease(&slot->mutex);
- ReplicationSlotsComputeRequiredXmin(false);
- }
-
- /*
- * Set the time since the slot has become inactive. We get the current
- * time beforehand to avoid system call while holding the spinlock.
- */
- now = GetCurrentTimestamp();
+ /*
+ * If slot needed to temporarily restrain both data and catalog xmin
+ * to create the catalog snapshot, remove that temporary constraint.
+ * Snapshots can only be exported while the initial snapshot is still
+ * acquired.
+ */
+ if (!TransactionIdIsValid(slot->data.xmin) &&
+ TransactionIdIsValid(slot->effective_xmin))
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->effective_xmin = InvalidTransactionId;
+ SpinLockRelease(&slot->mutex);
+ ReplicationSlotsComputeRequiredXmin(false);
+ }
- if (slot->data.persistency == RS_PERSISTENT)
- {
/*
- * Mark persistent slot inactive. We're not freeing it, just
- * disconnecting, but wake up others that may be waiting for it.
+ * Set the time since the slot has become inactive. We get the current
+ * time beforehand to avoid system call while holding the spinlock.
*/
- SpinLockAcquire(&slot->mutex);
- slot->active_pid = 0;
- slot->inactive_since = now;
- SpinLockRelease(&slot->mutex);
- ConditionVariableBroadcast(&slot->active_cv);
- }
- else
- {
- SpinLockAcquire(&slot->mutex);
- slot->inactive_since = now;
- SpinLockRelease(&slot->mutex);
- }
+ now = GetCurrentTimestamp();
- MyReplicationSlot = NULL;
+ if (slot->data.persistency == RS_PERSISTENT)
+ {
+ /*
+ * Mark persistent slot inactive. We're not freeing it, just
+ * disconnecting, but wake up others that may be waiting for it.
+ */
+ SpinLockAcquire(&slot->mutex);
+ slot->active_pid = 0;
+ slot->inactive_since = now;
+ SpinLockRelease(&slot->mutex);
+ ConditionVariableBroadcast(&slot->active_cv);
+ }
+ else
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->inactive_since = now;
+ SpinLockRelease(&slot->mutex);
+ }
+
+ MyReplicationSlot = NULL;
+ }
/* might not have been set when we've been a plain slot */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
--
2.53.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
@ 2026-05-29 16:44 ` Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
1 sibling, 1 reply; 27+ messages in thread
From: Srinath Reddy Sadipiralla @ 2026-05-29 16:44 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hi,
On Wed, May 27, 2026 at 5:20 PM Zhijie Hou (Fujitsu) <[email protected]>
wrote:
> Hi,
>
> While testing the slot release logic, I noticed a bug in
> ReplicationSlotRelease() where it may access a replication slot array
> entry that
> has already been released by itself.
>
> The detail is: When releasing an ephemeral replication slot,
> ReplicationSlotRelease() first drops the slot via
> ReplicationSlotDropAcquired().
> After this point, the slot's shared memory slot array entry can be
> immediately
> reused by another backend creating a new slot.
>
> However, ReplicationSlotRelease() continued executing common cleanup code
> that
> still dereferenced the old slot pointer and updated shared memory fields
> such as
> effective_xmin. If the slot array entry had already been reallocated, these
> writes could inadvertently affect a different, unrelated slot.
>
> I am attaching a patch that avoids touching slot shared-memory state after
> dropping an ephemeral slot. Keep the post-release shared-memory updates
> only for
> non-ephemeral slots, where the slot remains valid after release.
>
> To reproduce, we can use the following steps:
>
> 1. Attach gdb to the backend and set a breakpoint in
> ReplicationSlotRelease()
> right after ReplicationSlotDropAcquired() is called.
> 2. Create an ephemeral slot in the above backend with an invalid output
> plugin:
> SELECT pg_create_logical_replication_slot('test_slot_dropped',
> 'pgoutput2', false, false, true);
> 3. Once the breakpoint is hit, start another backend and create a new slot
> named 'test_slot_created'.
> 4. Release the breakpoint and allow the first backend to continue. At this
> point, you will see it updating the new slot 'test_slot_created' ->
> active_proc
> (and effective_xmin, if a snapshot is being exported) to invalid values.
> 5. Start a third backend and attempt to acquire the same slot
> 'test_slot_created' ? this should not be possible under normal
> circumstances,
> but the bug allows it.
>
patch LGTM.
>
> I haven't attached a test for this fix, as the change is straightforward
> and the
> likelihood of encountering this bug is low, so it may not be worth adding
> test
> cycles for it. However, if others feel differently, I'm OK to add one.
>
+1 for a test. The fix is just an else, so a future refactor could change
it and silently
reintroduce the corruption, since it scribbles on an unrelated reused slot,
nothing
would catch it. Injection points make it deterministic; I've attached a
diff patch that adds
a test that fails without the fix and passes with it.
--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
Attachments:
[application/octet-stream] nocfbot-test.patch (4.3K, 3-nocfbot-test.patch)
download | inline diff:
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index f012e99c9d7..a91082efb9c 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -787,6 +787,14 @@ ReplicationSlotRelease(void)
* decoding be disabled.
*/
ReplicationSlotDropAcquired(is_logical);
+
+ /*
+ * The slot's array entry is now free and may be reused by another
+ * backend at any moment. This injection point lets tests open that
+ * window deterministically and verify we never touch the (now stale)
+ * slot pointer afterwards.
+ */
+ INJECTION_POINT("ephemeral-slot-release-after-drop", NULL);
}
/*
diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index 97d11f98b59..fa5a0222dcd 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -275,6 +275,80 @@ is( $node_primary->safe_psql(
qq(Check that reset timestamp is later after resetting stats for slot '$stats_test_slot1' again.)
);
+# Regression test for the race in ReplicationSlotRelease() when releasing an
+# ephemeral slot. Releasing an ephemeral slot first drops it via
+# ReplicationSlotDropAcquired(), which frees the slot's array entry for reuse.
+# ReplicationSlotRelease() must not touch that (now stale) slot pointer
+# afterwards, or it can corrupt an unrelated slot that grabbed the freed entry.
+# This needs an injection point, so it only runs on builds that support them.
+SKIP:
+{
+ skip "Injection points not supported by this build", 2
+ if $ENV{enable_injection_points} ne 'yes';
+ skip "Extension injection_points not installed", 2
+ unless $node_primary->check_extension('injection_points');
+
+ $node_primary->safe_psql('postgres', q(CREATE EXTENSION injection_points));
+
+ # Freeze a backend right after it drops an ephemeral slot, i.e. once the
+ # slot's array entry has been freed and is available for reuse.
+ $node_primary->safe_psql('postgres',
+ q{SELECT injection_points_attach('ephemeral-slot-release-after-drop', 'wait')}
+ );
+
+ # Create a logical slot with a non-existent output plugin. The slot is
+ # created as RS_EPHEMERAL; the bogus plugin makes creation ERROR out, and
+ # the error path calls ReplicationSlotRelease(), which drops the ephemeral
+ # slot and then blocks on the injection point. on_error_stop => 0 keeps the
+ # session alive past the ERROR so it can still report 'done_release'.
+ my $dropper =
+ $node_primary->background_psql('postgres', on_error_stop => 0);
+ $dropper->query_until(
+ qr/start_drop/, q(
+\echo start_drop
+SELECT pg_create_logical_replication_slot('test_slot_dropped', 'no_such_plugin', false);
+\echo done_release
+));
+
+ # Wait until the backend is parked on the injection point, with the array
+ # entry of 'test_slot_dropped' freed and up for grabs.
+ $node_primary->wait_for_event('client backend',
+ 'ephemeral-slot-release-after-drop');
+
+ # A second backend creates a new persistent slot, which reuses the just
+ # freed array entry -- the very entry the frozen backend still points at.
+ $node_primary->safe_psql('postgres',
+ q{SELECT pg_create_physical_replication_slot('test_slot_created', true, false)}
+ );
+
+ my $before = $node_primary->safe_psql('postgres',
+ q{SELECT inactive_since FROM pg_replication_slots WHERE slot_name = 'test_slot_created'}
+ );
+
+ # Let the frozen backend finish releasing. With the bug it now scribbles on
+ # the reused entry; with the fix it leaves it untouched.
+ $node_primary->safe_psql('postgres',
+ q{SELECT injection_points_wakeup('ephemeral-slot-release-after-drop')});
+ $dropper->query_until(qr/done_release/, '');
+
+ my $after = $node_primary->safe_psql('postgres',
+ q{SELECT inactive_since FROM pg_replication_slots WHERE slot_name = 'test_slot_created'}
+ );
+
+ is($after, $before,
+ 'releasing an ephemeral slot must not modify a slot that reused its entry'
+ );
+
+ my $slot = $node_primary->safe_psql('postgres',
+ q{SELECT slot_name, slot_type FROM pg_replication_slots WHERE slot_name = 'test_slot_created'}
+ );
+ is($slot, "test_slot_created|physical", 'reused slot is intact');
+
+ $dropper->quit;
+ $node_primary->safe_psql('postgres',
+ q{SELECT injection_points_detach('ephemeral-slot-release-after-drop')});
+}
+
# done with the node
$node_primary->stop;
^ permalink raw reply [nested|flat] 27+ messages in thread
* RE: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
@ 2026-05-30 08:14 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-05-30 08:14 UTC (permalink / raw)
To: Srinath Reddy Sadipiralla <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Saturday, May 30, 2026 1:44 AM Srinath Reddy Sadipiralla <[email protected]> wrote:
> > On Wed, May 27, 2026 at 5:20 PM Zhijie Hou (Fujitsu) <mailto:[email protected]> wrote:
> > I haven't attached a test for this fix, as the change is straightforward and the
> > Likelihood of encountering this bug is low, so it may not be worth adding test
> > cycles for it. However, if others feel differently, I'm OK to add one.
>
> +1 for a test. The fix is just an else, so a future refactor could change it and silently
> reintroduce the corruption, since it scribbles on an unrelated reused slot, nothing
> would catch it. Injection points make it deterministic; I've attached a diff patch that adds
> a test that fails without the fix and passes with it.
Thanks for the test.
I'm not sure if adding an injection point for this rare case is worthwhile. Even
if we were to add one, future refactoring of that function could shift the
position of the injection point, so its long-term usefulness is uncertain. I
don't have a strong opinion on this, so I'll leave it to Fujii-San to decide.
Best Regards,
Hou zj
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
@ 2026-06-02 06:00 ` Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-02 06:00 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; Fujii Masao <[email protected]>; +Cc: Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On Sat, May 30, 2026 at 4:14 PM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> On Saturday, May 30, 2026 1:44 AM Srinath Reddy Sadipiralla <[email protected]> wrote:
>
> > > On Wed, May 27, 2026 at 5:20 PM Zhijie Hou (Fujitsu) <mailto:[email protected]> wrote:
> > > I haven't attached a test for this fix, as the change is straightforward and the
> > > Likelihood of encountering this bug is low, so it may not be worth adding test
> > > cycles for it. However, if others feel differently, I'm OK to add one.
> >
> > +1 for a test. The fix is just an else, so a future refactor could change it and silently
> > reintroduce the corruption, since it scribbles on an unrelated reused slot, nothing
> > would catch it. Injection points make it deterministic; I've attached a diff patch that adds
> > a test that fails without the fix and passes with it.
>
> Thanks for the test.
>
> I'm not sure if adding an injection point for this rare case is worthwhile. Even
> if we were to add one, future refactoring of that function could shift the
> position of the injection point, so its long-term usefulness is uncertain. I
> don't have a strong opinion on this, so I'll leave it to Fujii-San to decide.
Thanks for reporting this issue. I can reproduce it with lldb on Mac.
postgres=# SELECT pg_create_logical_replication_slot(
postgres(# 'test_slot_dropped',
postgres(# 'pgoutput2',
postgres(# false,
postgres(# false,
postgres(# true
postgres(# );
ERROR: could not access file "pgoutput2": No such file or directory
787 * decoding be disabled.
788 */
789 ReplicationSlotDropAcquired(is_logical);
-> 790 }
791
792 /*
793 * If slot needed to temporarily restrain both data and catalog xmin to
Target 0: (postgres) stopped.
(lldb) expr -- slot->data.name.data
(char[64]) $0 = "test_slot_created"
(lldb) expr -- slot->data.persistency
(ReplicationSlotPersistency) $1 = RS_PERSISTENT
(lldb) expr -- slot->active_proc
(ProcNumber) $2 = 126
The fix looks good to me.
There's an adjacent bug around drop_local_obsolete_slots. The root
cause of them looks similar -- ReplicationSlot * is a pointer to a
reusable shared-memory array cell, not a durable identity for the same
slot. In drop_local_obsolete_slots, the issue is that the slot has
been freed after ReplicationSlotDropAcquired(false); however, another
backend may reuse the same cell before the unlock/log reads. This
seems less severe -- it does not normally corrupt slot state, because
the code only read after the drop. But it can still unlock/log
misusing the identity of a different slot. Attached a test using
injection point to reproduce it and a patch to fix it.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
Attachments:
[text/x-perl-script] repro_slotsync_stale_pointer.pl (3.2K, 2-repro_slotsync_stale_pointer.pl)
download | inline:
use strict;
use warnings FATAL => 'all';
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
my $tag = $$;
my $primary = PostgreSQL::Test::Cluster->new("repro_ip_primary_$tag");
$primary->init(allows_streaming => 'logical');
$primary->append_conf(
'postgresql.conf', qq(
autovacuum = off
log_min_messages = 'debug2'
));
$primary->start;
if (!$primary->check_extension('injection_points'))
{
plan skip_all => 'Extension injection_points not installed';
}
# Create the extension before the base backup so the standby can call its
# functions while in recovery.
$primary->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
my $backup_name = 'backup';
$primary->backup($backup_name);
my $standby = PostgreSQL::Test::Cluster->new("repro_ip_standby_$tag");
$standby->init_from_backup(
$primary, $backup_name,
has_streaming => 1,
has_restoring => 1);
my $primary_connstr = $primary->connstr;
$standby->append_conf(
'postgresql.conf', qq(
hot_standby_feedback = on
primary_slot_name = 'phys_slot'
primary_conninfo = '$primary_connstr dbname=postgres'
log_min_messages = 'debug2'
));
$primary->safe_psql(
'postgres',
q{
SELECT pg_create_logical_replication_slot('victim_slot', 'pgoutput', false, false, true);
SELECT pg_create_physical_replication_slot('phys_slot');
});
$standby->start;
$primary->advance_wal(1);
$primary->wait_for_replay_catchup($standby);
note('attach injection point on standby');
$standby->safe_psql(
'postgres',
q{SELECT injection_points_attach('slotsync-obsolete-after-drop', 'wait');}
);
note('sync failover slot to standby');
$standby->safe_psql('postgres', 'SELECT pg_sync_replication_slots();');
is( $standby->safe_psql(
'postgres',
q{SELECT synced FROM pg_replication_slots WHERE slot_name = 'victim_slot';}
),
't',
'victim slot is synchronized to the standby');
note('drop remote failover slot');
$primary->safe_psql('postgres',
q{SELECT pg_drop_replication_slot('victim_slot');});
my $log_offset = -s $standby->logfile;
note('start slot sync and wait at injection point after local drop');
my $sync = $standby->background_psql('postgres', on_error_stop => 0);
$sync->query_until(
qr/start_sync/,
q(
\echo start_sync
SELECT pg_sync_replication_slots();
));
$standby->wait_for_event('client backend', 'slotsync-obsolete-after-drop');
ok( $standby->poll_query_until(
'postgres',
q{SELECT NOT EXISTS (SELECT 1 FROM pg_replication_slots WHERE slot_name = 'victim_slot');}
),
'victim slot has been dropped locally');
note('reuse freed slot array cell with a physical slot');
$standby->safe_psql('postgres',
q{SELECT pg_create_physical_replication_slot('replacement_slot');});
note('release injection point');
$standby->safe_psql(
'postgres',
q{
SELECT injection_points_wakeup('slotsync-obsolete-after-drop');
SELECT injection_points_detach('slotsync-obsolete-after-drop');
});
ok( $standby->wait_for_log(
qr/dropped replication slot "replacement_slot" of database with OID 0|you don't own a lock of type AccessShareLock/,
$log_offset),
'stale local_slot pointer was observed after drop');
is( $standby->safe_psql(
'postgres',
q{SELECT slot_type FROM pg_replication_slots WHERE slot_name = 'replacement_slot';}
),
'physical',
'replacement slot still exists');
$sync->quit;
done_testing();
[application/octet-stream] v1-0001-Avoid-stale-slot-access-after-dropping-obsolete-s.patch (3.0K, 3-v1-0001-Avoid-stale-slot-access-after-dropping-obsolete-s.patch)
download | inline diff:
From e920adcb5c01510f94ee75f26133e0efa6089038 Mon Sep 17 00:00:00 2001
From: alterego655 <[email protected]>
Date: Tue, 2 Jun 2026 13:14:54 +0800
Subject: [PATCH v1] Avoid stale slot access after dropping obsolete synced
slots
drop_local_obsolete_slots() kept using local_slot after calling
ReplicationSlotDropAcquired(). Once the drop completes, the slot array entry can
be reused by another backend, so later reads of local_slot->data could refer to a
different slot.
Copy the slot name and database OID before dropping the slot, and use those
saved values for unlocking and logging after the drop.
---
src/backend/replication/logical/slotsync.c | 25 ++++++++++++++--------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 93f41be32af..ea73f0aa262 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -541,6 +541,9 @@ drop_local_obsolete_slots(List *remote_slot_list)
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ bool dropped = false;
+ NameData slot_name = {0};
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
/*
@@ -548,8 +551,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ LockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
/*
* In the small window between getting the slot to drop and
@@ -562,6 +565,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
*/
SpinLockAcquire(&local_slot->mutex);
synced_slot = local_slot->in_use && local_slot->data.synced;
+ if (synced_slot)
+ slot_name = local_slot->data.name;
SpinLockRelease(&local_slot->mutex);
if (synced_slot)
@@ -572,17 +577,19 @@ drop_local_obsolete_slots(List *remote_slot_list)
* a standby, which derives its logical decoding state from
* the primary, it would be wrong to do so.
*/
- ReplicationSlotAcquire(NameStr(local_slot->data.name), true, false);
+ ReplicationSlotAcquire(NameStr(slot_name), true, false);
ReplicationSlotDropAcquired(false);
+ dropped = true;
}
- UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
- ereport(LOG,
- errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(local_slot->data.name),
- local_slot->data.database));
+ if (dropped)
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
}
}
}
--
2.51.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
@ 2026-06-03 12:03 ` Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Fujii Masao @ 2026-06-03 12:03 UTC (permalink / raw)
To: Xuneng Zhou <[email protected]>; +Cc: Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Jun 2, 2026 at 3:00 PM Xuneng Zhou <[email protected]> wrote:
>
> Hi,
>
> On Sat, May 30, 2026 at 4:14 PM Zhijie Hou (Fujitsu)
> <[email protected]> wrote:
> > I'm not sure if adding an injection point for this rare case is worthwhile. Even
> > if we were to add one, future refactoring of that function could shift the
> > position of the injection point, so its long-term usefulness is uncertain. I
> > don't have a strong opinion on this, so I'll leave it to Fujii-San to decide.
I've pushed the patch. Thanks!
IMO the proposed test looks a bit too narrow, so I'm not sure it's worth
adding at this point. For now, I've committed only the code fix.
> There's an adjacent bug around drop_local_obsolete_slots. The root
> cause of them looks similar -- ReplicationSlot * is a pointer to a
> reusable shared-memory array cell, not a durable identity for the same
> slot. In drop_local_obsolete_slots, the issue is that the slot has
> been freed after ReplicationSlotDropAcquired(false); however, another
> backend may reuse the same cell before the unlock/log reads. This
> seems less severe -- it does not normally corrupt slot state, because
> the code only read after the drop. But it can still unlock/log
> misusing the identity of a different slot. Attached a test using
> injection point to reproduce it and a patch to fix it.
Thanks again for the report and patch!
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ bool dropped = false;
+ NameData slot_name = {0};
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
Is it really safe to read slot_database before acquiring the database lock?
BTW, I'm also wondering whether it's safe for
local_sync_slot_required() to access local_slot without holding a lock.
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
@ 2026-06-05 11:45 ` Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-05 11:45 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi Fujii-san,
On Wed, Jun 3, 2026 at 8:03 PM Fujii Masao <[email protected]> wrote:
>
> On Tue, Jun 2, 2026 at 3:00 PM Xuneng Zhou <[email protected]> wrote:
> >
> > Hi,
> >
> > On Sat, May 30, 2026 at 4:14 PM Zhijie Hou (Fujitsu)
> > <[email protected]> wrote:
> > > I'm not sure if adding an injection point for this rare case is worthwhile. Even
> > > if we were to add one, future refactoring of that function could shift the
> > > position of the injection point, so its long-term usefulness is uncertain. I
> > > don't have a strong opinion on this, so I'll leave it to Fujii-San to decide.
>
> I've pushed the patch. Thanks!
>
> IMO the proposed test looks a bit too narrow, so I'm not sure it's worth
> adding at this point. For now, I've committed only the code fix.
>
>
> > There's an adjacent bug around drop_local_obsolete_slots. The root
> > cause of them looks similar -- ReplicationSlot * is a pointer to a
> > reusable shared-memory array cell, not a durable identity for the same
> > slot. In drop_local_obsolete_slots, the issue is that the slot has
> > been freed after ReplicationSlotDropAcquired(false); however, another
> > backend may reuse the same cell before the unlock/log reads. This
> > seems less severe -- it does not normally corrupt slot state, because
> > the code only read after the drop. But it can still unlock/log
> > misusing the identity of a different slot. Attached a test using
> > injection point to reproduce it and a patch to fix it.
>
> Thanks again for the report and patch!
>
> /* Drop the local slot if it is not required to be retained. */
> if (!local_sync_slot_required(local_slot, remote_slot_list))
> {
> + bool dropped = false;
> + NameData slot_name = {0};
> + Oid slot_database = local_slot->data.database;
> bool synced_slot;
>
> Is it really safe to read slot_database before acquiring the database lock?
Reading slot_database before taking the database lock seems not
inherently unsafe by itself. The comment suggests that the lock is
primarily used to prevent conflicts with the startup process running
ReplicationSlotsDropDBSlots() during db-drop replay; it does not
protect replication slot array reuse.
The unsafe part could be reading slot_database from local_slot after
ReplicationSlotControlLock has been released. At this point, the slot
array cell may already have been freed and reused, so the value read
may no longer belong to the slot that get_local_synced_slots()
originally collected. As a result, we could end up locking the wrong
database.
There seems to be two related issues:
1) Before drop: reading local_slot->data.database /
local_slot->data.name after the slot-array lock was released, before
verifying the cell still represents the same synced slot.
2) After drop: calling ReplicationSlotDropAcquired(false) and then
reading local_slot->data.database / local_slot->data.name for
unlocking/logging after the cell may have been reused by another
backend.
The prior patch targets to fix 2), but leaves 1) unfixed.
> BTW, I'm also wondering whether it's safe for
> local_sync_slot_required() to access local_slot without holding a lock.
I share the same concern here. The issue smells similar to the one
discussed above -- reading values from the array cell directly without
the protection of array lock. To solve them altogether, it might be
better to stop carrying raw ReplicationSlot * values across
unprotected windows. We can get the snapshot values such as slot name
& database oid from get_local_synced_slots() instead. Then
local_sync_slot_required() works from the snapshot, and
drop_local_obsolete_slots() uses the copied database OID to take the
database lock. Before dropping, it should revalidate by copied
name/database that the slot is still a synced logical slot, then
acquire/drop by copied name, and use only copied values for
unlock/logging. I plan to prepare a refactoring patch for this. Does
that seem like the right direction to you?
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 27+ messages in thread
* RE: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
@ 2026-06-06 09:35 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-06-06 13:25 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
0 siblings, 2 replies; 27+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-06-06 09:35 UTC (permalink / raw)
To: Xuneng Zhou <[email protected]>; Fujii Masao <[email protected]>; +Cc: Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Friday, June 5, 2026 8:45 PM Xuneng Zhou <[email protected]> wrote:
> On Wed, Jun 3, 2026 at 8:03 PM Fujii Masao <[email protected]> wrote:
> >
> > On Tue, Jun 2, 2026 at 3:00 PM Xuneng Zhou <[email protected]>
> wrote:
> >
> > /* Drop the local slot if it is not required to be retained. */
> > if (!local_sync_slot_required(local_slot, remote_slot_list))
> > {
> > + bool dropped = false;
> > + NameData slot_name = {0};
> > + Oid slot_database = local_slot->data.database;
> > bool synced_slot;
> >
> > Is it really safe to read slot_database before acquiring the database lock?
>
> Reading slot_database before taking the database lock seems not
> inherently unsafe by itself. The comment suggests that the lock is
> primarily used to prevent conflicts with the startup process running
> ReplicationSlotsDropDBSlots() during db-drop replay; it does not
> protect replication slot array reuse.
>
> The unsafe part could be reading slot_database from local_slot after
> ReplicationSlotControlLock has been released. At this point, the slot
> array cell may already have been freed and reused, so the value read
> may no longer belong to the slot that get_local_synced_slots()
> originally collected. As a result, we could end up locking the wrong
> database.
>
> There seems to be two related issues:
>
> 1) Before drop: reading local_slot->data.database /
> local_slot->data.name after the slot-array lock was released, before
> verifying the cell still represents the same synced slot.
I recall condition (1) is considered acceptable, since the database lock is
released immediately after re-verifying that the slot is no longer the original
'synced' one anyway. Additionally, this race can only occur when replaying a
DROP DATABASE, which is rare in practice. Since we only take a shared lock, it
does not seem to cause real issues.
>
> 2) After drop: calling ReplicationSlotDropAcquired(false) and then
> reading local_slot->data.database / local_slot->data.name for
> unlocking/logging after the cell may have been reused by another
> backend.
Right. I missed this race condition during implementation and agree it's a real
issue. An even bigger problem is that we could release a lock on the wrong
database if the slot entry is reused after being dropped. I think we should fix
this by saving the database OID at the beginning, as your patch does.
>
> The prior patch targets to fix 2), but leaves 1) unfixed.
>
> > BTW, I'm also wondering whether it's safe for
> > local_sync_slot_required() to access local_slot without holding a lock.
>
> I share the same concern here. The issue smells similar to the one
> discussed above -- reading values from the array cell directly without
> the protection of array lock.
>
> To solve them altogether, it might be
> better to stop carrying raw ReplicationSlot * values across
> unprotected windows. We can get the snapshot values such as slot name
> & database oid from get_local_synced_slots() instead. Then
> local_sync_slot_required() works from the snapshot, and
> drop_local_obsolete_slots() uses the copied database OID to take the
> database lock. Before dropping, it should revalidate by copied
> name/database that the slot is still a synced logical slot, then
> acquire/drop by copied name, and use only copied values for
> unlock/logging. I plan to prepare a refactoring patch for this. Does
> that seem like the right direction to you?
Saving only the name and database OID would force us to search for the slot
again in the loop, which was one reason we didn't implement it that way.
Best Regards,
Hou zj
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
@ 2026-06-06 13:25 ` Xuneng Zhou <[email protected]>
1 sibling, 0 replies; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-06 13:25 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: Fujii Masao <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Jun 6, 2026 at 5:35 PM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> On Friday, June 5, 2026 8:45 PM Xuneng Zhou <[email protected]> wrote:
> > On Wed, Jun 3, 2026 at 8:03 PM Fujii Masao <[email protected]> wrote:
> > >
> > > On Tue, Jun 2, 2026 at 3:00 PM Xuneng Zhou <[email protected]>
> > wrote:
> > >
> > > /* Drop the local slot if it is not required to be retained. */
> > > if (!local_sync_slot_required(local_slot, remote_slot_list))
> > > {
> > > + bool dropped = false;
> > > + NameData slot_name = {0};
> > > + Oid slot_database = local_slot->data.database;
> > > bool synced_slot;
> > >
> > > Is it really safe to read slot_database before acquiring the database lock?
> >
> > Reading slot_database before taking the database lock seems not
> > inherently unsafe by itself. The comment suggests that the lock is
> > primarily used to prevent conflicts with the startup process running
> > ReplicationSlotsDropDBSlots() during db-drop replay; it does not
> > protect replication slot array reuse.
> >
> > The unsafe part could be reading slot_database from local_slot after
> > ReplicationSlotControlLock has been released. At this point, the slot
> > array cell may already have been freed and reused, so the value read
> > may no longer belong to the slot that get_local_synced_slots()
> > originally collected. As a result, we could end up locking the wrong
> > database.
> >
> > There seems to be two related issues:
> >
> > 1) Before drop: reading local_slot->data.database /
> > local_slot->data.name after the slot-array lock was released, before
> > verifying the cell still represents the same synced slot.
>
> I recall condition (1) is considered acceptable, since the database lock is
> released immediately after re-verifying that the slot is no longer the original
> 'synced' one anyway. Additionally, this race can only occur when replaying a
> DROP DATABASE, which is rare in practice. Since we only take a shared lock, it
> does not seem to cause real issues.
>
> >
> > 2) After drop: calling ReplicationSlotDropAcquired(false) and then
> > reading local_slot->data.database / local_slot->data.name for
> > unlocking/logging after the cell may have been reused by another
> > backend.
>
> Right. I missed this race condition during implementation and agree it's a real
> issue. An even bigger problem is that we could release a lock on the wrong
> database if the slot entry is reused after being dropped. I think we should fix
> this by saving the database OID at the beginning, as your patch does.
>
> >
> > The prior patch targets to fix 2), but leaves 1) unfixed.
> >
> > > BTW, I'm also wondering whether it's safe for
> > > local_sync_slot_required() to access local_slot without holding a lock.
> >
> > I share the same concern here. The issue smells similar to the one
> > discussed above -- reading values from the array cell directly without
> > the protection of array lock.
> >
> > To solve them altogether, it might be
> > better to stop carrying raw ReplicationSlot * values across
> > unprotected windows. We can get the snapshot values such as slot name
> > & database oid from get_local_synced_slots() instead. Then
> > local_sync_slot_required() works from the snapshot, and
> > drop_local_obsolete_slots() uses the copied database OID to take the
> > database lock. Before dropping, it should revalidate by copied
> > name/database that the slot is still a synced logical slot, then
> > acquire/drop by copied name, and use only copied values for
> > unlock/logging. I plan to prepare a refactoring patch for this. Does
> > that seem like the right direction to you?
>
> Saving only the name and database OID would force us to search for the slot
> again in the loop, which was one reason we didn't implement it that way.
The extra search may not be ideal, especially for the worker with a
large number of slots to sync. But the cost could be avoided with an
extra field like slotno. Were there other blocking issues discussed
before?
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
@ 2026-06-11 09:22 ` Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
1 sibling, 1 reply; 27+ messages in thread
From: Amit Kapila @ 2026-06-11 09:22 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: Xuneng Zhou <[email protected]>; Fujii Masao <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Jun 6, 2026 at 3:05 PM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> On Friday, June 5, 2026 8:45 PM Xuneng Zhou <[email protected]> wrote:
> > On Wed, Jun 3, 2026 at 8:03 PM Fujii Masao <[email protected]> wrote:
> > >
> > > On Tue, Jun 2, 2026 at 3:00 PM Xuneng Zhou <[email protected]>
> > wrote:
> > >
> > > /* Drop the local slot if it is not required to be retained. */
> > > if (!local_sync_slot_required(local_slot, remote_slot_list))
> > > {
> > > + bool dropped = false;
> > > + NameData slot_name = {0};
> > > + Oid slot_database = local_slot->data.database;
> > > bool synced_slot;
> > >
> > > Is it really safe to read slot_database before acquiring the database lock?
> >
> > Reading slot_database before taking the database lock seems not
> > inherently unsafe by itself. The comment suggests that the lock is
> > primarily used to prevent conflicts with the startup process running
> > ReplicationSlotsDropDBSlots() during db-drop replay; it does not
> > protect replication slot array reuse.
> >
> > The unsafe part could be reading slot_database from local_slot after
> > ReplicationSlotControlLock has been released. At this point, the slot
> > array cell may already have been freed and reused, so the value read
> > may no longer belong to the slot that get_local_synced_slots()
> > originally collected. As a result, we could end up locking the wrong
> > database.
> >
> > There seems to be two related issues:
> >
> > 1) Before drop: reading local_slot->data.database /
> > local_slot->data.name after the slot-array lock was released, before
> > verifying the cell still represents the same synced slot.
>
> I recall condition (1) is considered acceptable, since the database lock is
> released immediately after re-verifying that the slot is no longer the original
> 'synced' one anyway. Additionally, this race can only occur when replaying a
> DROP DATABASE, which is rare in practice. Since we only take a shared lock, it
> does not seem to cause real issues.
>
It seems that (1) is talking about the access to local_slot->data.name
before we acquire database lock in local_sync_slot_required() whereas
your response doesn't seem to address that concern. If not, then how
exactly does the database lock protect what we are doing in
local_sync_slot_required()?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
@ 2026-06-11 11:18 ` Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Amit Kapila @ 2026-06-11 11:18 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: Xuneng Zhou <[email protected]>; Fujii Masao <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Jun 11, 2026 at 2:52 PM Amit Kapila <[email protected]> wrote:
>
> On Sat, Jun 6, 2026 at 3:05 PM Zhijie Hou (Fujitsu)
> <[email protected]> wrote:
> >
> > On Friday, June 5, 2026 8:45 PM Xuneng Zhou <[email protected]> wrote:
> > > On Wed, Jun 3, 2026 at 8:03 PM Fujii Masao <[email protected]> wrote:
> > > >
> > > > On Tue, Jun 2, 2026 at 3:00 PM Xuneng Zhou <[email protected]>
> > > wrote:
> > > >
> > > > /* Drop the local slot if it is not required to be retained. */
> > > > if (!local_sync_slot_required(local_slot, remote_slot_list))
> > > > {
> > > > + bool dropped = false;
> > > > + NameData slot_name = {0};
> > > > + Oid slot_database = local_slot->data.database;
> > > > bool synced_slot;
> > > >
> > > > Is it really safe to read slot_database before acquiring the database lock?
> > >
> > > Reading slot_database before taking the database lock seems not
> > > inherently unsafe by itself. The comment suggests that the lock is
> > > primarily used to prevent conflicts with the startup process running
> > > ReplicationSlotsDropDBSlots() during db-drop replay; it does not
> > > protect replication slot array reuse.
> > >
> > > The unsafe part could be reading slot_database from local_slot after
> > > ReplicationSlotControlLock has been released. At this point, the slot
> > > array cell may already have been freed and reused, so the value read
> > > may no longer belong to the slot that get_local_synced_slots()
> > > originally collected. As a result, we could end up locking the wrong
> > > database.
> > >
> > > There seems to be two related issues:
> > >
> > > 1) Before drop: reading local_slot->data.database /
> > > local_slot->data.name after the slot-array lock was released, before
> > > verifying the cell still represents the same synced slot.
> >
> > I recall condition (1) is considered acceptable, since the database lock is
> > released immediately after re-verifying that the slot is no longer the original
> > 'synced' one anyway. Additionally, this race can only occur when replaying a
> > DROP DATABASE, which is rare in practice. Since we only take a shared lock, it
> > does not seem to cause real issues.
> >
>
> It seems that (1) is talking about the access to local_slot->data.name
> before we acquire database lock in local_sync_slot_required() whereas
> your response doesn't seem to address that concern. If not, then how
> exactly does the database lock protect what we are doing in
> local_sync_slot_required()?
>
I re-analyzed this case and found the 'Before-drop' case is safe. In
the gap between get_local_synced_slots() releasing
ReplicationSlotControlLock and LockSharedObject,
ReplicationSlotsDropDBSlots() can run and free a synced slot cell
because the slotsync worker holds no database lock yet. The cell can
then be reused by any user-created (non-synced) slot. It could lead to
following risks which I think are already addressed due to recheck of
sync flag.
1. Stale name read in local_sync_slot_required(): The reused cell
holds a different name. local_sync_slot_required() might return false
(drop needed). But then the in_use && synced spinlock check sees
synced = false and skips the actual drop. The wrong decision is
caught.
2. Wrong database OID read at line 551: The reused cell holds OID_B
from the new slot. We lock OID_B, then at lines 563–565 we see synced
= false, skip the drop, and unlock OID_B at line 579. Since no drop
occurred, the cell is still the same non-synced slot, so the lock and
unlock see the same OID_B. Symmetric — no lock leak.
3. Acquiring the wrong slot at line 575: Once the shared database lock
is held at line 551, ReplicationSlotsDropDBSlots() is blocked from
freeing the cell. The slotsync worker itself won't free a synced slot
from any other code path while inside this function. So, this should
not happen.
Does this match your analysis? If so, After-drop case is still a risk,
and for that, the patch proposed in email [1] seems to address it.
[1] - https://www.postgresql.org/message-id/CABPTF7VyH1-W2xnDspECDEzFGQj%3DWTFpZBCqKfM11OAZa6gQHQ%40mail.g...
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
@ 2026-06-11 13:19 ` Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Fujii Masao @ 2026-06-11 13:19 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Zhijie Hou (Fujitsu) <[email protected]>; Xuneng Zhou <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Jun 11, 2026 at 8:18 PM Amit Kapila <[email protected]> wrote:
> 1. Stale name read in local_sync_slot_required(): The reused cell
> holds a different name. local_sync_slot_required() might return false
> (drop needed). But then the in_use && synced spinlock check sees
> synced = false and skips the actual drop. The wrong decision is
> caught.
Yes, we could skip the actual drop. But then wouldn't we still emit
the log message "dropped replication slot ..." even though no slot was
actually dropped?
> 2. Wrong database OID read at line 551: The reused cell holds OID_B
> from the new slot. We lock OID_B, then at lines 563–565 we see synced
> = false, skip the drop, and unlock OID_B at line 579. Since no drop
> occurred, the cell is still the same non-synced slot, so the lock and
> unlock see the same OID_B. Symmetric — no lock leak.
What happens if the slot for OID_B is dropped after we lock
OID_B, and then a new slot for OID_C reuses the same array entry? In
that case, wouldn't the later unlock read OID_C from
local_slot->data.database even though the lock was originally taken on
OID_B?
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
@ 2026-06-12 02:52 ` Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-12 02:52 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; Amit Kapila <[email protected]>; +Cc: Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi Fujii-san, Amit,
The issues look real to me and could be dealt with patch v1 partially.
On Thu, Jun 11, 2026 at 9:19 PM Fujii Masao <[email protected]> wrote:
>
> On Thu, Jun 11, 2026 at 8:18 PM Amit Kapila <[email protected]> wrote:
> > 1. Stale name read in local_sync_slot_required(): The reused cell
> > holds a different name. local_sync_slot_required() might return false
> > (drop needed). But then the in_use && synced spinlock check sees
> > synced = false and skips the actual drop. The wrong decision is
> > caught.
>
> Yes, we could skip the actual drop. But then wouldn't we still emit
> the log message "dropped replication slot ..." even though no slot was
> actually dropped?
With v1, we won't emit the log message unless the log is factually
dropped. However it did not prevent the stale read in
local_sync_slot_required().
> > 2. Wrong database OID read at line 551: The reused cell holds OID_B
> > from the new slot. We lock OID_B, then at lines 563–565 we see synced
> > = false, skip the drop, and unlock OID_B at line 579. Since no drop
> > occurred, the cell is still the same non-synced slot, so the lock and
> > unlock see the same OID_B. Symmetric — no lock leak.
>
> What happens if the slot for OID_B is dropped after we lock
> OID_B, and then a new slot for OID_C reuses the same array entry? In
> that case, wouldn't the later unlock read OID_C from
> local_slot->data.database even though the lock was originally taken on
> OID_B?
V1 stops doing the venerable second read of local_slot->data.database.
So if the copied value was already stale and points to OID_B, v1 is at
least symmetric:
read OID_B once
lock OID_B
cell reused as OID_C
unlock OID_B
But v1 seems not to fully solve issue 1.
It can still do this:
cell already reused before slot_database is copied
v1 copies OID_B from replacement slot
locks OID_B
recheck sees synced=false
skips drop
unlocks OID_B
That is still a stale read and possibly a wasted/wrong database lock,
but it doesn't leak the lock, unlocks the wrong object, logs a false
drop, or drops the replacement slot.
In an off-list chat with Zhijie, we kinda thought that holding the
lock of a wrong db for a brief time doesn't seem to harm a lot. The
concurrent dropping-db operation leads to this issue seems rare in
practice. He stated that the deletion of the slot seems unavoidable
because we have to acquire the database lock after releasing the
replication slot lock to avoid the deadlock with the startup/drop db
operation. Therefore, he prefered keeping the design simple and
avoiding the fatal issue over doing a broader refactoring work. I
don't have a strong opinion on this. Still attaching the refactoring
patch to do some clean-up in case someone thinks it's worthwhile.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
Attachments:
[application/octet-stream] v1-0001-Avoid-stale-slot-access-after-dropping-obsolete-s.patch (3.0K, 2-v1-0001-Avoid-stale-slot-access-after-dropping-obsolete-s.patch)
download | inline diff:
From e920adcb5c01510f94ee75f26133e0efa6089038 Mon Sep 17 00:00:00 2001
From: alterego655 <[email protected]>
Date: Tue, 2 Jun 2026 13:14:54 +0800
Subject: [PATCH v1] Avoid stale slot access after dropping obsolete synced
slots
drop_local_obsolete_slots() kept using local_slot after calling
ReplicationSlotDropAcquired(). Once the drop completes, the slot array entry can
be reused by another backend, so later reads of local_slot->data could refer to a
different slot.
Copy the slot name and database OID before dropping the slot, and use those
saved values for unlocking and logging after the drop.
---
src/backend/replication/logical/slotsync.c | 25 ++++++++++++++--------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 93f41be32af..ea73f0aa262 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -541,6 +541,9 @@ drop_local_obsolete_slots(List *remote_slot_list)
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ bool dropped = false;
+ NameData slot_name = {0};
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
/*
@@ -548,8 +551,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ LockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
/*
* In the small window between getting the slot to drop and
@@ -562,6 +565,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
*/
SpinLockAcquire(&local_slot->mutex);
synced_slot = local_slot->in_use && local_slot->data.synced;
+ if (synced_slot)
+ slot_name = local_slot->data.name;
SpinLockRelease(&local_slot->mutex);
if (synced_slot)
@@ -572,17 +577,19 @@ drop_local_obsolete_slots(List *remote_slot_list)
* a standby, which derives its logical decoding state from
* the primary, it would be wrong to do so.
*/
- ReplicationSlotAcquire(NameStr(local_slot->data.name), true, false);
+ ReplicationSlotAcquire(NameStr(slot_name), true, false);
ReplicationSlotDropAcquired(false);
+ dropped = true;
}
- UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
- ereport(LOG,
- errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(local_slot->data.name),
- local_slot->data.database));
+ if (dropped)
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
}
}
}
--
2.51.0
[application/octet-stream] v1-0002-Avoid-stale-slot-pointers-in-slotsync-cleanup.patch (10.5K, 3-v1-0002-Avoid-stale-slot-pointers-in-slotsync-cleanup.patch)
download | inline diff:
From c741e5c1205efedcc6841c85ab0539832c658578 Mon Sep 17 00:00:00 2001
From: alterego655 <[email protected]>
Date: Fri, 12 Jun 2026 10:27:04 +0800
Subject: [PATCH v1] Avoid stale slot pointers in slotsync cleanup
drop_local_obsolete_slots() kept raw ReplicationSlot * values after
scanning the shared slot array. Once ReplicationSlotControlLock was
released, those array entries could be dropped and reused before the
later retention check or database-lock/drop path.
Copy the local synced slot identity while scanning the array, and carry
those copied values instead of raw slot pointers. Revalidate the copied
identity before acquiring and dropping the slot, and use the copied
name and database OID for acquire, unlock, and logging.
This makes the cleanup path avoid depending on slot array entries
remaining stable across unlocked windows.
---
src/backend/replication/logical/slotsync.c | 205 +++++++++++++++++----
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 167 insertions(+), 39 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index ea73f0aa262..a323df452fd 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -178,6 +178,20 @@ typedef struct RemoteSlot
ReplicationSlotInvalidationCause invalidated;
} RemoteSlot;
+/*
+ * Copied identity of a local synchronized slot.
+ *
+ * The slot number is a cached array position used to avoid a later name scan.
+ * It is not sufficient as identity by itself because slot array entries can be
+ * reused after a slot is dropped.
+ */
+typedef struct LocalSyncedSlot
+{
+ NameData name;
+ Oid database;
+ int slotno;
+} LocalSyncedSlot;
+
static void slotsync_failure_callback(int code, Datum arg);
static void update_synced_slots_inactive_since(void);
@@ -444,7 +458,7 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
}
/*
- * Get the list of local logical slots that are synchronized from the
+ * Get copied identities of local logical slots that are synchronized from the
* primary server.
*/
static List *
@@ -459,10 +473,29 @@ get_local_synced_slots(void)
ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];
/* Check if it is a synchronized slot */
- if (s->in_use && s->data.synced)
+ if (s->in_use)
{
- Assert(SlotIsLogical(s));
- local_slots = lappend(local_slots, s);
+ LocalSyncedSlot local_slot;
+ bool synced;
+
+ SpinLockAcquire(&s->mutex);
+ synced = s->data.synced;
+ if (synced)
+ {
+ local_slot.name = s->data.name;
+ local_slot.database = s->data.database;
+ local_slot.slotno = i;
+ }
+ SpinLockRelease(&s->mutex);
+
+ if (synced)
+ {
+ LocalSyncedSlot *slot = palloc(sizeof(LocalSyncedSlot));
+
+ Assert(local_slot.database != InvalidOid);
+ *slot = local_slot;
+ local_slots = lappend(local_slots, slot);
+ }
}
}
@@ -471,34 +504,125 @@ get_local_synced_slots(void)
return local_slots;
}
+/*
+ * Check whether the previously observed slot array cell still contains a
+ * synchronized logical slot matching the copied identity. If requested, copy
+ * the current invalidation cause.
+ */
+static bool
+local_synced_slot_matches(const LocalSyncedSlot *local_slot,
+ ReplicationSlotInvalidationCause *invalidated)
+{
+ ReplicationSlot *slot;
+ bool matches = false;
+
+ LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+
+ Assert(local_slot->slotno >= 0);
+ Assert(local_slot->slotno <
+ max_replication_slots + max_repack_replication_slots);
+
+ slot = &ReplicationSlotCtl->replication_slots[local_slot->slotno];
+ if (slot->in_use)
+ {
+ NameData slot_name;
+ Oid slot_database;
+ bool slot_synced;
+ ReplicationSlotInvalidationCause slot_invalidated;
+
+ SpinLockAcquire(&slot->mutex);
+ slot_name = slot->data.name;
+ slot_database = slot->data.database;
+ slot_synced = slot->data.synced;
+ slot_invalidated = slot->data.invalidated;
+ SpinLockRelease(&slot->mutex);
+
+ /*
+ * A synced slot's database can be updated by slot synchronization, so
+ * database is not globally immutable. It is still part of the copied
+ * identity here because slot synchronization is serialized by
+ * SlotSyncCtx->syncing.
+ */
+ matches = slot_database != InvalidOid &&
+ slot_synced &&
+ strcmp(NameStr(slot_name), NameStr(local_slot->name)) == 0 &&
+ slot_database == local_slot->database;
+
+ if (matches && invalidated)
+ *invalidated = slot_invalidated;
+ }
+
+ LWLockRelease(ReplicationSlotControlLock);
+
+ return matches;
+}
+
+/*
+ * Check whether the acquired slot still matches a previously copied local slot
+ * identity.
+ */
+static bool
+acquired_slot_matches(const LocalSyncedSlot *local_slot)
+{
+ ReplicationSlot *slot = MyReplicationSlot;
+ NameData slot_name;
+ Oid slot_database;
+ bool slot_synced;
+ int slotno;
+
+ Assert(slot != NULL);
+
+ /*
+ * We own MyReplicationSlot, so slot.h allows reading its fields without
+ * taking the slot mutex.
+ */
+ slot_name = slot->data.name;
+ slot_database = slot->data.database;
+ slot_synced = slot->data.synced;
+ slotno = ReplicationSlotIndex(slot);
+
+ return slotno == local_slot->slotno &&
+ slot_database != InvalidOid &&
+ slot_synced &&
+ strcmp(NameStr(slot_name), NameStr(local_slot->name)) == 0 &&
+ slot_database == local_slot->database;
+}
+
/*
* Helper function to check if local_slot is required to be retained.
*
* Return false either if local_slot does not exist in the remote_slots list
- * or is invalidated while the corresponding remote slot is still valid,
- * otherwise true.
+ * or is invalidated while the corresponding remote slot is still valid, or if
+ * the copied local slot no longer matches the current slot array entry.
+ * Otherwise, return true.
*/
static bool
-local_sync_slot_required(ReplicationSlot *local_slot, List *remote_slots)
+local_sync_slot_required(const LocalSyncedSlot *local_slot, List *remote_slots)
{
bool remote_exists = false;
bool locally_invalidated = false;
foreach_ptr(RemoteSlot, remote_slot, remote_slots)
{
- if (strcmp(remote_slot->name, NameStr(local_slot->data.name)) == 0)
+ if (strcmp(remote_slot->name, NameStr(local_slot->name)) == 0)
{
remote_exists = true;
/*
* If remote slot is not invalidated but local slot is marked as
- * invalidated, then set locally_invalidated flag.
+ * invalidated, then set locally_invalidated flag. When the remote
+ * slot is already invalidated, keep the local slot as before and
+ * avoid an unnecessary revalidation.
*/
- SpinLockAcquire(&local_slot->mutex);
- locally_invalidated =
- (remote_slot->invalidated == RS_INVAL_NONE) &&
- (local_slot->data.invalidated != RS_INVAL_NONE);
- SpinLockRelease(&local_slot->mutex);
+ if (remote_slot->invalidated == RS_INVAL_NONE)
+ {
+ ReplicationSlotInvalidationCause invalidated;
+
+ if (!local_synced_slot_matches(local_slot, &invalidated))
+ return false;
+
+ locally_invalidated = invalidated != RS_INVAL_NONE;
+ }
break;
}
@@ -536,40 +660,31 @@ drop_local_obsolete_slots(List *remote_slot_list)
{
List *local_slots = get_local_synced_slots();
- foreach_ptr(ReplicationSlot, local_slot, local_slots)
+ foreach_ptr(LocalSyncedSlot, local_slot, local_slots)
{
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
bool dropped = false;
- NameData slot_name = {0};
- Oid slot_database = local_slot->data.database;
- bool synced_slot;
/*
* Use shared lock to prevent a conflict with
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, slot_database, 0,
+ LockSharedObject(DatabaseRelationId, local_slot->database, 0,
AccessShareLock);
/*
- * In the small window between getting the slot to drop and
- * locking the database, there is a possibility of a parallel
- * database drop by the startup process and the creation of a new
- * slot by the user. This new user-created slot may end up using
- * the same shared memory as that of 'local_slot'. Thus check if
- * local_slot is still the synced one before performing the actual
- * drop.
+ * In the window between copying the slot identity and locking the
+ * database, there is a possibility of a parallel database drop by
+ * the startup process and the creation of a new slot by the user.
+ * This new user-created slot may end up using the same shared
+ * memory cell as the copied slot. Thus check whether the cached
+ * slot position still contains the same synced slot before
+ * performing the actual drop.
*/
- SpinLockAcquire(&local_slot->mutex);
- synced_slot = local_slot->in_use && local_slot->data.synced;
- if (synced_slot)
- slot_name = local_slot->data.name;
- SpinLockRelease(&local_slot->mutex);
-
- if (synced_slot)
+ if (local_synced_slot_matches(local_slot, NULL))
{
/*
* Now acquire and drop the slot. Note we purposely don't
@@ -577,21 +692,33 @@ drop_local_obsolete_slots(List *remote_slot_list)
* a standby, which derives its logical decoding state from
* the primary, it would be wrong to do so.
*/
- ReplicationSlotAcquire(NameStr(slot_name), true, false);
- ReplicationSlotDropAcquired(false);
- dropped = true;
+ ReplicationSlotAcquire(NameStr(local_slot->name), true, false);
+
+ /*
+ * Recheck the acquired slot defensively in case the slot
+ * changed between revalidation and acquisition.
+ */
+ if (acquired_slot_matches(local_slot))
+ {
+ ReplicationSlotDropAcquired(false);
+ dropped = true;
+ }
+ else
+ ReplicationSlotRelease();
}
- UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ UnlockSharedObject(DatabaseRelationId, local_slot->database, 0,
AccessShareLock);
if (dropped)
ereport(LOG,
errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(slot_name),
- slot_database));
+ NameStr(local_slot->name),
+ local_slot->database));
}
}
+
+ list_free_deep(local_slots);
}
/*
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 8cf40c87043..663383b0531 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1646,6 +1646,7 @@ LoInfo
LoadStmt
LocalBufferLookupEnt
LocalPgBackendStatus
+LocalSyncedSlot
LocalTransactionId
Location
LocationIndex
--
2.51.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
@ 2026-06-12 10:54 ` Amit Kapila <[email protected]>
2026-06-16 05:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-16 12:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
0 siblings, 2 replies; 27+ messages in thread
From: Amit Kapila @ 2026-06-12 10:54 UTC (permalink / raw)
To: Xuneng Zhou <[email protected]>; +Cc: Fujii Masao <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Jun 12, 2026 at 8:22 AM Xuneng Zhou <[email protected]> wrote:
>
> The issues look real to me and could be dealt with patch v1 partially.
>
> On Thu, Jun 11, 2026 at 9:19 PM Fujii Masao <[email protected]> wrote:
> >
> > On Thu, Jun 11, 2026 at 8:18 PM Amit Kapila <[email protected]> wrote:
> > > 1. Stale name read in local_sync_slot_required(): The reused cell
> > > holds a different name. local_sync_slot_required() might return false
> > > (drop needed). But then the in_use && synced spinlock check sees
> > > synced = false and skips the actual drop. The wrong decision is
> > > caught.
> >
> > Yes, we could skip the actual drop. But then wouldn't we still emit
> > the log message "dropped replication slot ..." even though no slot was
> > actually dropped?
>
> With v1, we won't emit the log message unless the log is factually
> dropped. However it did not prevent the stale read in
> local_sync_slot_required().
>
> > > 2. Wrong database OID read at line 551: The reused cell holds OID_B
> > > from the new slot. We lock OID_B, then at lines 563–565 we see synced
> > > = false, skip the drop, and unlock OID_B at line 579. Since no drop
> > > occurred, the cell is still the same non-synced slot, so the lock and
> > > unlock see the same OID_B. Symmetric — no lock leak.
> >
> > What happens if the slot for OID_B is dropped after we lock
> > OID_B, and then a new slot for OID_C reuses the same array entry? In
> > that case, wouldn't the later unlock read OID_C from
> > local_slot->data.database even though the lock was originally taken on
> > OID_B?
>
> V1 stops doing the venerable second read of local_slot->data.database.
> So if the copied value was already stale and points to OID_B, v1 is at
> least symmetric:
>
> read OID_B once
> lock OID_B
> cell reused as OID_C
> unlock OID_B
>
> But v1 seems not to fully solve issue 1.
>
> It can still do this:
>
> cell already reused before slot_database is copied
> v1 copies OID_B from replacement slot
> locks OID_B
> recheck sees synced=false
> skips drop
> unlocks OID_B
>
> That is still a stale read and possibly a wasted/wrong database lock,
> but it doesn't leak the lock, unlocks the wrong object, logs a false
> drop, or drops the replacement slot.
>
> In an off-list chat with Zhijie, we kinda thought that holding the
> lock of a wrong db for a brief time doesn't seem to harm a lot. The
> concurrent dropping-db operation leads to this issue seems rare in
> practice. He stated that the deletion of the slot seems unavoidable
> because we have to acquire the database lock after releasing the
> replication slot lock to avoid the deadlock with the startup/drop db
> operation. Therefore, he prefered keeping the design simple and
> avoiding the fatal issue over doing a broader refactoring work.
>
+1. I also think this change is not worth it.
>
I
> don't have a strong opinion on this. Still attaching the refactoring
> patch to do some clean-up in case someone thinks it's worthwhile.
>
I feel even if there is an argument to do such a refactoring, it can
be done separately. We can push forward with 0001 and then do more
discussion for 0002, if required. I can take care of 0001 unless
Fujii-San wishes to take care of it?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
@ 2026-06-16 05:29 ` Xuneng Zhou <[email protected]>
2026-06-16 08:54 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
1 sibling, 1 reply; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-16 05:29 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Fujii Masao <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Jun 12, 2026 at 6:54 PM Amit Kapila <[email protected]> wrote:
>
> On Fri, Jun 12, 2026 at 8:22 AM Xuneng Zhou <[email protected]> wrote:
> >
> > The issues look real to me and could be dealt with patch v1 partially.
> >
> > On Thu, Jun 11, 2026 at 9:19 PM Fujii Masao <[email protected]> wrote:
> > >
> > > On Thu, Jun 11, 2026 at 8:18 PM Amit Kapila <[email protected]> wrote:
> > > > 1. Stale name read in local_sync_slot_required(): The reused cell
> > > > holds a different name. local_sync_slot_required() might return false
> > > > (drop needed). But then the in_use && synced spinlock check sees
> > > > synced = false and skips the actual drop. The wrong decision is
> > > > caught.
> > >
> > > Yes, we could skip the actual drop. But then wouldn't we still emit
> > > the log message "dropped replication slot ..." even though no slot was
> > > actually dropped?
> >
> > With v1, we won't emit the log message unless the log is factually
> > dropped. However it did not prevent the stale read in
> > local_sync_slot_required().
> >
> > > > 2. Wrong database OID read at line 551: The reused cell holds OID_B
> > > > from the new slot. We lock OID_B, then at lines 563–565 we see synced
> > > > = false, skip the drop, and unlock OID_B at line 579. Since no drop
> > > > occurred, the cell is still the same non-synced slot, so the lock and
> > > > unlock see the same OID_B. Symmetric — no lock leak.
> > >
> > > What happens if the slot for OID_B is dropped after we lock
> > > OID_B, and then a new slot for OID_C reuses the same array entry? In
> > > that case, wouldn't the later unlock read OID_C from
> > > local_slot->data.database even though the lock was originally taken on
> > > OID_B?
> >
> > V1 stops doing the venerable second read of local_slot->data.database.
> > So if the copied value was already stale and points to OID_B, v1 is at
> > least symmetric:
> >
> > read OID_B once
> > lock OID_B
> > cell reused as OID_C
> > unlock OID_B
> >
> > But v1 seems not to fully solve issue 1.
> >
> > It can still do this:
> >
> > cell already reused before slot_database is copied
> > v1 copies OID_B from replacement slot
> > locks OID_B
> > recheck sees synced=false
> > skips drop
> > unlocks OID_B
> >
> > That is still a stale read and possibly a wasted/wrong database lock,
> > but it doesn't leak the lock, unlocks the wrong object, logs a false
> > drop, or drops the replacement slot.
> >
> > In an off-list chat with Zhijie, we kinda thought that holding the
> > lock of a wrong db for a brief time doesn't seem to harm a lot. The
> > concurrent dropping-db operation leads to this issue seems rare in
> > practice. He stated that the deletion of the slot seems unavoidable
> > because we have to acquire the database lock after releasing the
> > replication slot lock to avoid the deadlock with the startup/drop db
> > operation. Therefore, he prefered keeping the design simple and
> > avoiding the fatal issue over doing a broader refactoring work.
> >
>
> +1. I also think this change is not worth it.
I am also OK with the scope of change made by patch 1.
> > don't have a strong opinion on this. Still attaching the refactoring
> > patch to do some clean-up in case someone thinks it's worthwhile.
> >
>
> I feel even if there is an argument to do such a refactoring, it can
> be done separately.
Makes sense to me.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 27+ messages in thread
* RE: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 05:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
@ 2026-06-16 08:54 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-06-16 09:35 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-06-16 08:54 UTC (permalink / raw)
To: Xuneng Zhou <[email protected]>; +Cc: Fujii Masao <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>; Amit Kapila <[email protected]>
On Tuesday, June 16, 2026 1:30 PM Xuneng Zhou <[email protected]> wrote:
> On Fri, Jun 12, 2026 at 6:54 PM Amit Kapila <[email protected]>
> wrote:
> >
> > On Fri, Jun 12, 2026 at 8:22 AM Xuneng Zhou <[email protected]>
> wrote:
> > >
> > > On Thu, Jun 11, 2026 at 9:19 PM Fujii Masao <[email protected]>
> > > In an off-list chat with Zhijie, we kinda thought that holding the
> > > lock of a wrong db for a brief time doesn't seem to harm a lot. The
> > > concurrent dropping-db operation leads to this issue seems rare in
> > > practice. He stated that the deletion of the slot seems unavoidable
> > > because we have to acquire the database lock after releasing the
> > > replication slot lock to avoid the deadlock with the startup/drop db
> > > operation. Therefore, he prefered keeping the design simple and
> > > avoiding the fatal issue over doing a broader refactoring work.
> > >
> >
> > +1. I also think this change is not worth it.
>
> I am also OK with the scope of change made by patch 1.
I have one minor comment for the 0001 patch.
+ NameData slot_name = {0};
...
SpinLockAcquire(&local_slot->mutex);
synced_slot = local_slot->in_use && local_slot->data.synced;
+ if (synced_slot)
+ slot_name = local_slot->data.name;
SpinLockRelease(&local_slot->mutex);
We can defer assigning slot_name until after we pass the existing (synced_slot)
check. Since it's a synced slot, no other process can change it at that point,
and we can also skip initializing slot_name. (Please refer to the
attached patch for suggested changes)
Best Regards,
Hou zj
Attachments:
[application/octet-stream] 0001-comments_patch (1.3K, 2-0001-comments_patch)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 05:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-16 08:54 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
@ 2026-06-16 09:35 ` Amit Kapila <[email protected]>
2026-06-16 10:32 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Amit Kapila @ 2026-06-16 09:35 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: Xuneng Zhou <[email protected]>; Fujii Masao <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Jun 16, 2026 at 2:24 PM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> On Tuesday, June 16, 2026 1:30 PM Xuneng Zhou <[email protected]> wrote:
> > On Fri, Jun 12, 2026 at 6:54 PM Amit Kapila <[email protected]>
> > wrote:
> > >
> > > On Fri, Jun 12, 2026 at 8:22 AM Xuneng Zhou <[email protected]>
> > wrote:
> > > >
> > > > On Thu, Jun 11, 2026 at 9:19 PM Fujii Masao <[email protected]>
> > > > In an off-list chat with Zhijie, we kinda thought that holding the
> > > > lock of a wrong db for a brief time doesn't seem to harm a lot. The
> > > > concurrent dropping-db operation leads to this issue seems rare in
> > > > practice. He stated that the deletion of the slot seems unavoidable
> > > > because we have to acquire the database lock after releasing the
> > > > replication slot lock to avoid the deadlock with the startup/drop db
> > > > operation. Therefore, he prefered keeping the design simple and
> > > > avoiding the fatal issue over doing a broader refactoring work.
> > > >
> > >
> > > +1. I also think this change is not worth it.
> >
> > I am also OK with the scope of change made by patch 1.
>
> I have one minor comment for the 0001 patch.
>
> + NameData slot_name = {0};
> ...
> SpinLockAcquire(&local_slot->mutex);
> synced_slot = local_slot->in_use && local_slot->data.synced;
> + if (synced_slot)
> + slot_name = local_slot->data.name;
> SpinLockRelease(&local_slot->mutex);
>
> We can defer assigning slot_name until after we pass the existing (synced_slot)
> check. Since it's a synced slot, no other process can change it at that point,
> and we can also skip initializing slot_name. (Please refer to the
> attached patch for suggested changes)
>
+ if (dropped)
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
Can we avoid the if (dropped) check by placing this LOG message
immediately after dropping the slot under synced slot check?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* RE: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 05:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-16 08:54 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-16 09:35 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
@ 2026-06-16 10:32 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-06-17 07:08 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-06-16 10:32 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Xuneng Zhou <[email protected]>; Fujii Masao <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tuesday, June 16, 2026 5:36 PM Amit Kapila <[email protected]> wrote:
>
> On Tue, Jun 16, 2026 at 2:24 PM Zhijie Hou (Fujitsu) <[email protected]>
> wrote:
> >
> >
> > I have one minor comment for the 0001 patch.
> >
> > + NameData slot_name = {0};
> > ...
> > SpinLockAcquire(&local_slot->mutex);
> > synced_slot = local_slot->in_use &&
> > local_slot->data.synced;
> > + if (synced_slot)
> > + slot_name = local_slot->data.name;
> > SpinLockRelease(&local_slot->mutex);
> >
> > We can defer assigning slot_name until after we pass the existing
> > (synced_slot) check. Since it's a synced slot, no other process can
> > change it at that point, and we can also skip initializing slot_name.
> > (Please refer to the attached patch for suggested changes)
> >
>
> + if (dropped)
> + ereport(LOG,
> + errmsg("dropped replication slot \"%s\" of database with OID %u",
> + NameStr(slot_name),
> + slot_database));
>
> Can we avoid the if (dropped) check by placing this LOG message immediately
> after dropping the slot under synced slot check?
I think we can do that. I'm attaching the new patches for all supported
branches, incorporating both my and Amit's comments. I hope this helps move the
fix forward.
I also confirmed that the fix works on all supported branches.
Best Regards,
Hou zj
Attachments:
[application/octet-stream] v2_PG17-0001-Avoid-stale-slot-access-after-dropping-obsol.patch (2.7K, 2-v2_PG17-0001-Avoid-stale-slot-access-after-dropping-obsol.patch)
download | inline diff:
From 41350e61f9d0a5060a7a76fc1de91ba944f4328c Mon Sep 17 00:00:00 2001
From: Zhijie Hou <[email protected]>
Date: Tue, 16 Jun 2026 18:15:35 +0800
Subject: [PATCH v2_PG18] Avoid stale slot access after dropping obsolete
synced slots
drop_local_obsolete_slots() kept using local_slot after calling
ReplicationSlotDropAcquired(). Once the drop completes, the slot array entry can
be reused by another backend, so later reads of local_slot->data could refer to a
different slot.
Copy the slot name and database OID before dropping the slot, and use those
saved values for unlocking and logging after the drop.
Author: Xuneng Zhou <[email protected]>
Reviewed-by: Zhijie Hou <[email protected]>
Reviewed-by: Amit Kapila <[email protected]>
---
src/backend/replication/logical/slotsync.c | 23 ++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index bc42d74fec2..c4dda8aa5f1 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -463,6 +463,7 @@ drop_local_obsolete_slots(List *remote_slot_list)
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
/*
@@ -470,8 +471,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ LockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
/*
* In the small window between getting the slot to drop and
@@ -488,17 +489,19 @@ drop_local_obsolete_slots(List *remote_slot_list)
if (synced_slot)
{
- ReplicationSlotAcquire(NameStr(local_slot->data.name), true);
+ NameData slot_name = local_slot->data.name;
+
+ ReplicationSlotAcquire(NameStr(slot_name), true);
ReplicationSlotDropAcquired();
- }
- UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
+ }
- ereport(LOG,
- errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(local_slot->data.name),
- local_slot->data.database));
+ UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
}
}
}
--
2.43.0
[application/octet-stream] v2-0001-Avoid-stale-slot-access-after-dropping-obsolete-s.patch (2.9K, 3-v2-0001-Avoid-stale-slot-access-after-dropping-obsolete-s.patch)
download | inline diff:
From 898dfaeab0a721b8d2902e6d7b4b799193b18aee Mon Sep 17 00:00:00 2001
From: alterego655 <[email protected]>
Date: Tue, 2 Jun 2026 13:14:54 +0800
Subject: [PATCH v2] Avoid stale slot access after dropping obsolete synced slots
drop_local_obsolete_slots() kept using local_slot after calling
ReplicationSlotDropAcquired(). Once the drop completes, the slot array entry can
be reused by another backend, so later reads of local_slot->data could refer to a
different slot.
Copy the slot name and database OID before dropping the slot, and use those
saved values for unlocking and logging after the drop.
Author: Xuneng Zhou <[email protected]>
Reviewed-by: Zhijie Hou <[email protected]>
Reviewed-by: Amit Kapila <[email protected]>
---
src/backend/replication/logical/slotsync.c | 23 ++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 96107c9475d..05637344363 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -541,6 +541,7 @@ drop_local_obsolete_slots(List *remote_slot_list)
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
/*
@@ -548,8 +549,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ LockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
/*
* In the small window between getting the slot to drop and
@@ -566,23 +567,25 @@ drop_local_obsolete_slots(List *remote_slot_list)
if (synced_slot)
{
+ NameData slot_name = local_slot->data.name;
+
/*
* Now acquire and drop the slot. Note we purposely don't
* request logical decoding to be disabled here: since this is
* a standby, which derives its logical decoding state from
* the primary, it would be wrong to do so.
*/
- ReplicationSlotAcquire(NameStr(local_slot->data.name), true, false);
+ ReplicationSlotAcquire(NameStr(slot_name), true, false);
ReplicationSlotDropAcquired(false);
- }
- UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
+ }
- ereport(LOG,
- errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(local_slot->data.name),
- local_slot->data.database));
+ UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
}
}
}
--
2.43.0
[application/octet-stream] v2_PG18-0001-Avoid-stale-slot-access-after-dropping-obsol.patch (2.7K, 4-v2_PG18-0001-Avoid-stale-slot-access-after-dropping-obsol.patch)
download | inline diff:
From 41350e61f9d0a5060a7a76fc1de91ba944f4328c Mon Sep 17 00:00:00 2001
From: Zhijie Hou <[email protected]>
Date: Tue, 16 Jun 2026 18:15:35 +0800
Subject: [PATCH v2_PG18] Avoid stale slot access after dropping obsolete
synced slots
drop_local_obsolete_slots() kept using local_slot after calling
ReplicationSlotDropAcquired(). Once the drop completes, the slot array entry can
be reused by another backend, so later reads of local_slot->data could refer to a
different slot.
Copy the slot name and database OID before dropping the slot, and use those
saved values for unlocking and logging after the drop.
Author: Xuneng Zhou <[email protected]>
Reviewed-by: Zhijie Hou <[email protected]>
Reviewed-by: Amit Kapila <[email protected]>
---
src/backend/replication/logical/slotsync.c | 23 ++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index bc42d74fec2..c4dda8aa5f1 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -463,6 +463,7 @@ drop_local_obsolete_slots(List *remote_slot_list)
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
/*
@@ -470,8 +471,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ LockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
/*
* In the small window between getting the slot to drop and
@@ -488,17 +489,19 @@ drop_local_obsolete_slots(List *remote_slot_list)
if (synced_slot)
{
- ReplicationSlotAcquire(NameStr(local_slot->data.name), true, false);
+ NameData slot_name = local_slot->data.name;
+
+ ReplicationSlotAcquire(NameStr(slot_name), true, false);
ReplicationSlotDropAcquired();
- }
- UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
+ }
- ereport(LOG,
- errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(local_slot->data.name),
- local_slot->data.database));
+ UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
}
}
}
--
2.43.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 05:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-16 08:54 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-16 09:35 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 10:32 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
@ 2026-06-17 07:08 ` Xuneng Zhou <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-17 07:08 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: Amit Kapila <[email protected]>; Fujii Masao <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Jun 16, 2026 at 6:32 PM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> On Tuesday, June 16, 2026 5:36 PM Amit Kapila <[email protected]> wrote:
> >
> > On Tue, Jun 16, 2026 at 2:24 PM Zhijie Hou (Fujitsu) <[email protected]>
> > wrote:
> > >
> > >
> > > I have one minor comment for the 0001 patch.
> > >
> > > + NameData slot_name = {0};
> > > ...
> > > SpinLockAcquire(&local_slot->mutex);
> > > synced_slot = local_slot->in_use &&
> > > local_slot->data.synced;
> > > + if (synced_slot)
> > > + slot_name = local_slot->data.name;
> > > SpinLockRelease(&local_slot->mutex);
> > >
> > > We can defer assigning slot_name until after we pass the existing
> > > (synced_slot) check. Since it's a synced slot, no other process can
> > > change it at that point, and we can also skip initializing slot_name.
> > > (Please refer to the attached patch for suggested changes)
> > >
This seems ok to me. The only risk is that people who don't know the
assumption might think this getter is unsafe.
> > + if (dropped)
> > + ereport(LOG,
> > + errmsg("dropped replication slot \"%s\" of database with OID %u",
> > + NameStr(slot_name),
> > + slot_database));
> >
> > Can we avoid the if (dropped) check by placing this LOG message immediately
> > after dropping the slot under synced slot check?
>
> I think we can do that.
+1. Placing the log message within the lock window looks OK to me
since the cost is small.
> I'm attaching the new patches for all supported
> branches, incorporating both my and Amit's comments. I hope this helps move the
> fix forward.
>
> I also confirmed that the fix works on all supported branches.
Thanks for your preparation for patches and confirmation of them!
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
@ 2026-06-16 12:45 ` Fujii Masao <[email protected]>
2026-06-17 07:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
1 sibling, 1 reply; 27+ messages in thread
From: Fujii Masao @ 2026-06-16 12:45 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Xuneng Zhou <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Jun 12, 2026 at 7:54 PM Amit Kapila <[email protected]> wrote:
> I feel even if there is an argument to do such a refactoring, it can
> be done separately. We can push forward with 0001 and then do more
> discussion for 0002, if required. I can take care of 0001 unless
> Fujii-San wishes to take care of it?
Yeah, please feel free to work on 0001.
Regarding 0002, since the race is very rare and non-fatal, I'm okay
with accepting the risk rather than adding more refactoring just to
avoid it.
I'm a bit tempted to add a source comment explaining the risk and
why we accept it, though, so other developers can understand
the tradeoff. For example:
diff --git a/src/backend/replication/logical/slotsync.c
b/src/backend/replication/logical/slotsync.c
index 05637344363..ca49f20e7d9 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -560,6 +560,12 @@ drop_local_obsolete_slots(List *remote_slot_list)
* the same shared memory as that of
'local_slot'. Thus check if
* local_slot is still the synced one before
performing the actual
* drop.
+ *
+ * Because local_slot still points to a
reusable slot-array entry,
+ * fields such as name or database OID could
already be stale here.
+ * That could cause an incorrect cleanup
decision for this cycle or
+ * briefly lock an unrelated database. We
accept that risk because
+ * this race is rare and non-fatal.
*/
SpinLockAcquire(&local_slot->mutex);
synced_slot = local_slot->in_use &&
local_slot->data.synced;
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 12:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
@ 2026-06-17 07:29 ` Xuneng Zhou <[email protected]>
2026-06-18 03:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-17 07:29 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Amit Kapila <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Jun 16, 2026 at 8:46 PM Fujii Masao <[email protected]> wrote:
>
> On Fri, Jun 12, 2026 at 7:54 PM Amit Kapila <[email protected]> wrote:
> > I feel even if there is an argument to do such a refactoring, it can
> > be done separately. We can push forward with 0001 and then do more
> > discussion for 0002, if required. I can take care of 0001 unless
> > Fujii-San wishes to take care of it?
>
> Yeah, please feel free to work on 0001.
>
> Regarding 0002, since the race is very rare and non-fatal, I'm okay
> with accepting the risk rather than adding more refactoring just to
> avoid it.
>
> I'm a bit tempted to add a source comment explaining the risk and
> why we accept it, though, so other developers can understand
> the tradeoff. For example:
>
> diff --git a/src/backend/replication/logical/slotsync.c
> b/src/backend/replication/logical/slotsync.c
> index 05637344363..ca49f20e7d9 100644
> --- a/src/backend/replication/logical/slotsync.c
> +++ b/src/backend/replication/logical/slotsync.c
> @@ -560,6 +560,12 @@ drop_local_obsolete_slots(List *remote_slot_list)
> * the same shared memory as that of
> 'local_slot'. Thus check if
> * local_slot is still the synced one before
> performing the actual
> * drop.
> + *
> + * Because local_slot still points to a
> reusable slot-array entry,
> + * fields such as name or database OID could
> already be stale here.
> + * That could cause an incorrect cleanup
> decision for this cycle or
> + * briefly lock an unrelated database. We
> accept that risk because
> + * this race is rare and non-fatal.
> */
> SpinLockAcquire(&local_slot->mutex);
> synced_slot = local_slot->in_use &&
> local_slot->data.synced;
Thanks for suggesting the comment! It helps to clarify the situation
and the trade-off we made here. I tweaked it a bit and added it to the
patches prepared by Zhijie.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
Attachments:
[application/octet-stream] v3_PG17-0001-Avoid-stale-slot-access-after-dropping-obsol.patch (3.6K, 2-v3_PG17-0001-Avoid-stale-slot-access-after-dropping-obsol.patch)
download | inline diff:
From b279fa1de26db4af7218bc6a041e6e9e918a1802 Mon Sep 17 00:00:00 2001
From: alterego655 <[email protected]>
Date: Wed, 17 Jun 2026 14:53:28 +0800
Subject: [PATCH v3_PG17] Avoid stale slot access after dropping obsolete
synced slots
drop_local_obsolete_slots() kept using local_slot after calling
ReplicationSlotDropAcquired(). Once the drop completes, the slot array entry can
be reused by another backend, so later reads of local_slot->data could refer to a
different slot.
Copy the slot name and database OID before dropping the slot, and use those
saved values for unlocking and logging after the drop.
Author: Xuneng Zhou <[email protected]>
Reviewed-by: Zhijie Hou <[email protected]>
Reviewed-by: Amit Kapila <[email protected]>
Reviewed-by: Fujii Masao <[email protected]>
---
src/backend/replication/logical/slotsync.c | 33 +++++++++++++++-------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 051b1c866b5..19e0be20b72 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -423,6 +423,7 @@ drop_local_obsolete_slots(List *remote_slot_list)
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
/*
@@ -430,8 +431,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ LockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
/*
* In the small window between getting the slot to drop and
@@ -441,6 +442,16 @@ drop_local_obsolete_slots(List *remote_slot_list)
* the same shared memory as that of 'local_slot'. Thus check if
* local_slot is still the synced one before performing actual
* drop.
+ *
+ * We cannot close this window by holding
+ * ReplicationSlotControlLock while taking the database lock,
+ * because the database-drop path holds the database lock and then
+ * scans replication slots. Therefore, local_slot may already
+ * refer to a reused slot-array entry here, and fields such as
+ * name or database OID could already be stale. That could cause
+ * an incorrect cleanup decision for this cycle or briefly lock an
+ * unrelated database. We accept that risk because this race is
+ * rare and non-fatal.
*/
SpinLockAcquire(&local_slot->mutex);
synced_slot = local_slot->in_use && local_slot->data.synced;
@@ -448,17 +459,19 @@ drop_local_obsolete_slots(List *remote_slot_list)
if (synced_slot)
{
- ReplicationSlotAcquire(NameStr(local_slot->data.name), true);
+ NameData slot_name = local_slot->data.name;
+
+ ReplicationSlotAcquire(NameStr(slot_name), true);
ReplicationSlotDropAcquired();
- }
- UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
+ }
- ereport(LOG,
- errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(local_slot->data.name),
- local_slot->data.database));
+ UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
}
}
}
--
2.51.0
[application/octet-stream] v3-0001-Avoid-stale-slot-access-after-dropping-obsolete-s.patch (3.8K, 3-v3-0001-Avoid-stale-slot-access-after-dropping-obsolete-s.patch)
download | inline diff:
From 878988979952d0483d9b91626187537b1bf4f044 Mon Sep 17 00:00:00 2001
From: alterego655 <[email protected]>
Date: Wed, 17 Jun 2026 14:43:14 +0800
Subject: [PATCH v3] Avoid stale slot access after dropping obsolete synced
slots
drop_local_obsolete_slots() kept using local_slot after calling
ReplicationSlotDropAcquired(). Once the drop completes, the slot array entry can
be reused by another backend, so later reads of local_slot->data could refer to a
different slot.
Copy the slot name and database OID before dropping the slot, and use those
saved values for unlocking and logging after the drop.
Author: Xuneng Zhou <[email protected]>
Reviewed-by: Zhijie Hou <[email protected]>
Reviewed-by: Amit Kapila <[email protected]>
Reviewed-by: Fujii Masao <[email protected]>
---
src/backend/replication/logical/slotsync.c | 33 +++++++++++++++-------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 96107c9475d..a22d0515d48 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -541,6 +541,7 @@ drop_local_obsolete_slots(List *remote_slot_list)
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
/*
@@ -548,8 +549,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ LockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
/*
* In the small window between getting the slot to drop and
@@ -559,6 +560,16 @@ drop_local_obsolete_slots(List *remote_slot_list)
* the same shared memory as that of 'local_slot'. Thus check if
* local_slot is still the synced one before performing the actual
* drop.
+ *
+ * We cannot close this window by holding
+ * ReplicationSlotControlLock while taking the database lock,
+ * because the database-drop path holds the database lock and then
+ * scans replication slots. Therefore, local_slot may already
+ * refer to a reused slot-array entry here, and fields such as
+ * name or database OID could already be stale. That could cause
+ * an incorrect cleanup decision for this cycle or briefly lock an
+ * unrelated database. We accept that risk because this race is
+ * rare and non-fatal.
*/
SpinLockAcquire(&local_slot->mutex);
synced_slot = local_slot->in_use && local_slot->data.synced;
@@ -566,23 +577,25 @@ drop_local_obsolete_slots(List *remote_slot_list)
if (synced_slot)
{
+ NameData slot_name = local_slot->data.name;
+
/*
* Now acquire and drop the slot. Note we purposely don't
* request logical decoding to be disabled here: since this is
* a standby, which derives its logical decoding state from
* the primary, it would be wrong to do so.
*/
- ReplicationSlotAcquire(NameStr(local_slot->data.name), true, false);
+ ReplicationSlotAcquire(NameStr(slot_name), true, false);
ReplicationSlotDropAcquired(false);
- }
- UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
+ }
- ereport(LOG,
- errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(local_slot->data.name),
- local_slot->data.database));
+ UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
}
}
}
--
2.51.0
[application/octet-stream] v3_PG18-0001-Avoid-stale-slot-access-after-dropping-obsol.patch (3.6K, 4-v3_PG18-0001-Avoid-stale-slot-access-after-dropping-obsol.patch)
download | inline diff:
From 33c3160502cbf3f5347f86c6c5bfaac2b1b51547 Mon Sep 17 00:00:00 2001
From: alterego655 <[email protected]>
Date: Wed, 17 Jun 2026 15:26:20 +0800
Subject: [PATCH v3_PG18] Avoid stale slot access after dropping obsolete
synced slots
drop_local_obsolete_slots() kept using local_slot after calling
ReplicationSlotDropAcquired(). Once the drop completes, the slot array entry can
be reused by another backend, so later reads of local_slot->data could refer to a
different slot.
Copy the slot name and database OID before dropping the slot, and use those
saved values for unlocking and logging after the drop.
Author: Xuneng Zhou <[email protected]>
Reviewed-by: Zhijie Hou <[email protected]>
Reviewed-by: Amit Kapila <[email protected]>
Reviewed-by: Fujii Masao <[email protected]>
---
src/backend/replication/logical/slotsync.c | 33 +++++++++++++++-------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 61b2e9396aa..9976fdccf41 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -439,6 +439,7 @@ drop_local_obsolete_slots(List *remote_slot_list)
/* Drop the local slot if it is not required to be retained. */
if (!local_sync_slot_required(local_slot, remote_slot_list))
{
+ Oid slot_database = local_slot->data.database;
bool synced_slot;
/*
@@ -446,8 +447,8 @@ drop_local_obsolete_slots(List *remote_slot_list)
* ReplicationSlotsDropDBSlots(), trying to drop the same slot
* during a drop-database operation.
*/
- LockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ LockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
/*
* In the small window between getting the slot to drop and
@@ -457,6 +458,16 @@ drop_local_obsolete_slots(List *remote_slot_list)
* the same shared memory as that of 'local_slot'. Thus check if
* local_slot is still the synced one before performing actual
* drop.
+ *
+ * We cannot close this window by holding
+ * ReplicationSlotControlLock while taking the database lock,
+ * because the database-drop path holds the database lock and then
+ * scans replication slots. Therefore, local_slot may already
+ * refer to a reused slot-array entry here, and fields such as
+ * name or database OID could already be stale. That could cause
+ * an incorrect cleanup decision for this cycle or briefly lock an
+ * unrelated database. We accept that risk because this race is
+ * rare and non-fatal.
*/
SpinLockAcquire(&local_slot->mutex);
synced_slot = local_slot->in_use && local_slot->data.synced;
@@ -464,17 +475,19 @@ drop_local_obsolete_slots(List *remote_slot_list)
if (synced_slot)
{
- ReplicationSlotAcquire(NameStr(local_slot->data.name), true, false);
+ NameData slot_name = local_slot->data.name;
+
+ ReplicationSlotAcquire(NameStr(slot_name), true, false);
ReplicationSlotDropAcquired();
- }
- UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
- 0, AccessShareLock);
+ ereport(LOG,
+ errmsg("dropped replication slot \"%s\" of database with OID %u",
+ NameStr(slot_name),
+ slot_database));
+ }
- ereport(LOG,
- errmsg("dropped replication slot \"%s\" of database with OID %u",
- NameStr(local_slot->data.name),
- local_slot->data.database));
+ UnlockSharedObject(DatabaseRelationId, slot_database, 0,
+ AccessShareLock);
}
}
}
--
2.51.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 12:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-17 07:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
@ 2026-06-18 03:19 ` Amit Kapila <[email protected]>
2026-06-18 08:36 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Amit Kapila @ 2026-06-18 03:19 UTC (permalink / raw)
To: Xuneng Zhou <[email protected]>; +Cc: Fujii Masao <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Jun 17, 2026 at 12:59 PM Xuneng Zhou <[email protected]> wrote:
>
> On Tue, Jun 16, 2026 at 8:46 PM Fujii Masao <[email protected]> wrote:
> >
> > On Fri, Jun 12, 2026 at 7:54 PM Amit Kapila <[email protected]> wrote:
> > > I feel even if there is an argument to do such a refactoring, it can
> > > be done separately. We can push forward with 0001 and then do more
> > > discussion for 0002, if required. I can take care of 0001 unless
> > > Fujii-San wishes to take care of it?
> >
> > Yeah, please feel free to work on 0001.
> >
> > Regarding 0002, since the race is very rare and non-fatal, I'm okay
> > with accepting the risk rather than adding more refactoring just to
> > avoid it.
> >
> > I'm a bit tempted to add a source comment explaining the risk and
> > why we accept it, though, so other developers can understand
> > the tradeoff. For example:
> >
> > diff --git a/src/backend/replication/logical/slotsync.c
> > b/src/backend/replication/logical/slotsync.c
> > index 05637344363..ca49f20e7d9 100644
> > --- a/src/backend/replication/logical/slotsync.c
> > +++ b/src/backend/replication/logical/slotsync.c
> > @@ -560,6 +560,12 @@ drop_local_obsolete_slots(List *remote_slot_list)
> > * the same shared memory as that of
> > 'local_slot'. Thus check if
> > * local_slot is still the synced one before
> > performing the actual
> > * drop.
> > + *
> > + * Because local_slot still points to a
> > reusable slot-array entry,
> > + * fields such as name or database OID could
> > already be stale here.
> > + * That could cause an incorrect cleanup
> > decision for this cycle or
> > + * briefly lock an unrelated database. We
> > accept that risk because
> > + * this race is rare and non-fatal.
> > */
> > SpinLockAcquire(&local_slot->mutex);
> > synced_slot = local_slot->in_use &&
> > local_slot->data.synced;
>
> Thanks for suggesting the comment! It helps to clarify the situation
> and the trade-off we made here. I tweaked it a bit and added it to the
> patches prepared by Zhijie.
>
+ *
+ * We cannot close this window by holding
+ * ReplicationSlotControlLock while taking the database lock,
+ * because the database-drop path holds the database lock and then
+ * scans replication slots.
The database-drop path acquires ReplicationSlotControlLock in shared
mode, so not sure if the above is completely correct, here you are
going in the direction of trying to defend that no easy solution
exists which needs more thought. Fujii-San's proposal was better but
there also we may need to be a bit more specific about "That could
cause an incorrect cleanup decision ...", otherwise, it makes the
comment unclear.
I am planning to commit and backpatch the fix for the first problem
based on what Hou-San has shared (v2-*), then we can discuss how to
improve the existing comments and if we agree on something, that can
be a HEAD-only patch.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 12:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-17 07:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-18 03:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
@ 2026-06-18 08:36 ` Xuneng Zhou <[email protected]>
2026-06-19 12:08 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-18 08:36 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Fujii Masao <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Jun 18, 2026 at 11:20 AM Amit Kapila <[email protected]> wrote:
>
> On Wed, Jun 17, 2026 at 12:59 PM Xuneng Zhou <[email protected]> wrote:
> >
> > On Tue, Jun 16, 2026 at 8:46 PM Fujii Masao <[email protected]> wrote:
> > >
> > > On Fri, Jun 12, 2026 at 7:54 PM Amit Kapila <[email protected]> wrote:
> > > > I feel even if there is an argument to do such a refactoring, it can
> > > > be done separately. We can push forward with 0001 and then do more
> > > > discussion for 0002, if required. I can take care of 0001 unless
> > > > Fujii-San wishes to take care of it?
> > >
> > > Yeah, please feel free to work on 0001.
> > >
> > > Regarding 0002, since the race is very rare and non-fatal, I'm okay
> > > with accepting the risk rather than adding more refactoring just to
> > > avoid it.
> > >
> > > I'm a bit tempted to add a source comment explaining the risk and
> > > why we accept it, though, so other developers can understand
> > > the tradeoff. For example:
> > >
> > > diff --git a/src/backend/replication/logical/slotsync.c
> > > b/src/backend/replication/logical/slotsync.c
> > > index 05637344363..ca49f20e7d9 100644
> > > --- a/src/backend/replication/logical/slotsync.c
> > > +++ b/src/backend/replication/logical/slotsync.c
> > > @@ -560,6 +560,12 @@ drop_local_obsolete_slots(List *remote_slot_list)
> > > * the same shared memory as that of
> > > 'local_slot'. Thus check if
> > > * local_slot is still the synced one before
> > > performing the actual
> > > * drop.
> > > + *
> > > + * Because local_slot still points to a
> > > reusable slot-array entry,
> > > + * fields such as name or database OID could
> > > already be stale here.
> > > + * That could cause an incorrect cleanup
> > > decision for this cycle or
> > > + * briefly lock an unrelated database. We
> > > accept that risk because
> > > + * this race is rare and non-fatal.
> > > */
> > > SpinLockAcquire(&local_slot->mutex);
> > > synced_slot = local_slot->in_use &&
> > > local_slot->data.synced;
> >
> > Thanks for suggesting the comment! It helps to clarify the situation
> > and the trade-off we made here. I tweaked it a bit and added it to the
> > patches prepared by Zhijie.
> >
>
> + *
> + * We cannot close this window by holding
> + * ReplicationSlotControlLock while taking the database lock,
> + * because the database-drop path holds the database lock and then
> + * scans replication slots.
>
> The database-drop path acquires ReplicationSlotControlLock in shared
> mode, so not sure if the above is completely correct, here you are
> going in the direction of trying to defend that no easy solution
> exists which needs more thought.
I agree that this phrasing could be misleading. The dangerous part is
in the later actual slot drop. Eventually the database-drop path needs
ReplicationSlotControlLock in exclusive mode to set slot->in_use =
false. If the slotsync holds ReplicationSlotControlLock in shared mode
while it tries to take DB AccessShareLock. Then this would lead to a
deadlock. On second thought, I’m inclined not to add a comment
explaining the constraint here, since it would be difficult to do so
concisely and it's not directly related to the issue.
> Fujii-San's proposal was better but
> there also we may need to be a bit more specific about "That could
> cause an incorrect cleanup decision ...", otherwise, it makes the
> comment unclear.
OK, how about elaborate it a bit like this:
/*
* In the small window between getting the slot to drop and
* locking the database, there is a possibility of a parallel
* database drop by the startup process and the creation of a new
* slot by the user. This new user-created slot may end up using
* the same shared memory as that of 'local_slot'.
*
* If that happens, local_slot now describes the replacement slot:
* local_sync_slot_required() may have made its drop decision using
* the replacement slot's name or invalidation state, and slot_database
* may refer to the replacement slot's database. Thus check if
* local_slot is still a synced slot before performing the actual drop.
* This does not prove it is the original slot, but it prevents dropping
* an ordinary user-created replacement slot, and the copied database OID
* keeps lock/unlock symmetric. The remaining risk is limited to this
* cleanup cycle, such as briefly holding an unrelated database lock, and
* is acceptable here because this race is rare.
*/
> I am planning to commit and backpatch the fix for the first problem
> based on what Hou-San has shared (v2-*), then we can discuss how to
> improve the existing comments and if we agree on something, that can
> be a HEAD-only patch.
Thanks!
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 12:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-17 07:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-18 03:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-18 08:36 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
@ 2026-06-19 12:08 ` Amit Kapila <[email protected]>
2026-06-20 06:41 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Amit Kapila @ 2026-06-19 12:08 UTC (permalink / raw)
To: Xuneng Zhou <[email protected]>; +Cc: Fujii Masao <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Jun 18, 2026 at 2:06 PM Xuneng Zhou <[email protected]> wrote:
>
> OK, how about elaborate it a bit like this:
>
> /*
> * In the small window between getting the slot to drop and
> * locking the database, there is a possibility of a parallel
> * database drop by the startup process and the creation of a new
> * slot by the user. This new user-created slot may end up using
> * the same shared memory as that of 'local_slot'.
> *
> * If that happens, local_slot now describes the replacement slot:
> * local_sync_slot_required() may have made its drop decision using
> * the replacement slot's name or invalidation state, and slot_database
> * may refer to the replacement slot's database. Thus check if
> * local_slot is still a synced slot before performing the actual drop.
> * This does not prove it is the original slot, but it prevents dropping
> * an ordinary user-created replacement slot, and the copied database OID
> * keeps lock/unlock symmetric. The remaining risk is limited to this
> * cleanup cycle, such as briefly holding an unrelated database lock, and
> * is acceptable here because this race is rare.
> */
>
Okay inspired from your and Fujii-san's version, here is a third version:
/*
* In the small window between getting the slot to drop and
* locking the database, there is a possibility of a parallel
* database drop by the startup process and the creation of a new
* slot by the user. This new user-created slot may end up using
* the same shared memory as that of 'local_slot'.
*
* Because local_slot still points to a reusable slot-array entry,
* its fields (name, database OID, invalidation state) may already
* describe such a replacement slot by the time we reach here. That
* means the drop decision made by local_sync_slot_required() above
* could have been based on the replacement slot's data, and
* slot_database could refer to an unrelated database. The recheck
* below keeps us from actually dropping a user-created replacement
* slot; the residual risk is confined to this cycle (for example,
* briefly locking an unrelated database) and is acceptable because
* the race is rare and non-fatal.
*/
Thoughts?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 12:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-17 07:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-18 03:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-18 08:36 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-19 12:08 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
@ 2026-06-20 06:41 ` Xuneng Zhou <[email protected]>
2026-06-20 09:42 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Xuneng Zhou @ 2026-06-20 06:41 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Fujii Masao <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Jun 19, 2026 at 8:08 PM Amit Kapila <[email protected]> wrote:
>
> On Thu, Jun 18, 2026 at 2:06 PM Xuneng Zhou <[email protected]> wrote:
> >
> > OK, how about elaborate it a bit like this:
> >
> > /*
> > * In the small window between getting the slot to drop and
> > * locking the database, there is a possibility of a parallel
> > * database drop by the startup process and the creation of a new
> > * slot by the user. This new user-created slot may end up using
> > * the same shared memory as that of 'local_slot'.
> > *
> > * If that happens, local_slot now describes the replacement slot:
> > * local_sync_slot_required() may have made its drop decision using
> > * the replacement slot's name or invalidation state, and slot_database
> > * may refer to the replacement slot's database. Thus check if
> > * local_slot is still a synced slot before performing the actual drop.
> > * This does not prove it is the original slot, but it prevents dropping
> > * an ordinary user-created replacement slot, and the copied database OID
> > * keeps lock/unlock symmetric. The remaining risk is limited to this
> > * cleanup cycle, such as briefly holding an unrelated database lock, and
> > * is acceptable here because this race is rare.
> > */
> >
>
> Okay inspired from your and Fujii-san's version, here is a third version:
> /*
> * In the small window between getting the slot to drop and
> * locking the database, there is a possibility of a parallel
> * database drop by the startup process and the creation of a new
> * slot by the user. This new user-created slot may end up using
> * the same shared memory as that of 'local_slot'.
> *
> * Because local_slot still points to a reusable slot-array entry,
> * its fields (name, database OID, invalidation state) may already
> * describe such a replacement slot by the time we reach here. That
> * means the drop decision made by local_sync_slot_required() above
> * could have been based on the replacement slot's data, and
> * slot_database could refer to an unrelated database. The recheck
> * below keeps us from actually dropping a user-created replacement
> * slot; the residual risk is confined to this cycle (for example,
> * briefly locking an unrelated database) and is acceptable because
> * the race is rare and non-fatal.
> */
>
> Thoughts?
LGTM. It looks well-articulated.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Fix race in ReplicationSlotRelease for ephemeral slots
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 16:44 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-05 11:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` RE: Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-06-11 09:22 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 11:18 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-11 13:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-12 02:52 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-16 12:45 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Fujii Masao <[email protected]>
2026-06-17 07:29 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-18 03:19 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-18 08:36 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
2026-06-19 12:08 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Amit Kapila <[email protected]>
2026-06-20 06:41 ` Re: Fix race in ReplicationSlotRelease for ephemeral slots Xuneng Zhou <[email protected]>
@ 2026-06-20 09:42 ` Amit Kapila <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Amit Kapila @ 2026-06-20 09:42 UTC (permalink / raw)
To: Xuneng Zhou <[email protected]>; +Cc: Fujii Masao <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Jun 20, 2026 at 12:11 PM Xuneng Zhou <[email protected]> wrote:
>
> On Fri, Jun 19, 2026 at 8:08 PM Amit Kapila <[email protected]> wrote:
> >
> > On Thu, Jun 18, 2026 at 2:06 PM Xuneng Zhou <[email protected]> wrote:
> > >
> > > OK, how about elaborate it a bit like this:
> > >
> > > /*
> > > * In the small window between getting the slot to drop and
> > > * locking the database, there is a possibility of a parallel
> > > * database drop by the startup process and the creation of a new
> > > * slot by the user. This new user-created slot may end up using
> > > * the same shared memory as that of 'local_slot'.
> > > *
> > > * If that happens, local_slot now describes the replacement slot:
> > > * local_sync_slot_required() may have made its drop decision using
> > > * the replacement slot's name or invalidation state, and slot_database
> > > * may refer to the replacement slot's database. Thus check if
> > > * local_slot is still a synced slot before performing the actual drop.
> > > * This does not prove it is the original slot, but it prevents dropping
> > > * an ordinary user-created replacement slot, and the copied database OID
> > > * keeps lock/unlock symmetric. The remaining risk is limited to this
> > > * cleanup cycle, such as briefly holding an unrelated database lock, and
> > > * is acceptable here because this race is rare.
> > > */
> > >
> >
> > Okay inspired from your and Fujii-san's version, here is a third version:
> > /*
> > * In the small window between getting the slot to drop and
> > * locking the database, there is a possibility of a parallel
> > * database drop by the startup process and the creation of a new
> > * slot by the user. This new user-created slot may end up using
> > * the same shared memory as that of 'local_slot'.
> > *
> > * Because local_slot still points to a reusable slot-array entry,
> > * its fields (name, database OID, invalidation state) may already
> > * describe such a replacement slot by the time we reach here. That
> > * means the drop decision made by local_sync_slot_required() above
> > * could have been based on the replacement slot's data, and
> > * slot_database could refer to an unrelated database. The recheck
> > * below keeps us from actually dropping a user-created replacement
> > * slot; the residual risk is confined to this cycle (for example,
> > * briefly locking an unrelated database) and is acceptable because
> > * the race is rare and non-fatal.
> > */
> >
> > Thoughts?
>
> LGTM. It looks well-articulated.
>
Thanks, I'll push this as soon as the PG20 branch opens.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
end of thread, other threads:[~2026-06-20 09:42 UTC | newest]
Thread overview: 27+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-27 11:50 Fix race in ReplicationSlotRelease for ephemeral slots Zhijie Hou (Fujitsu) <[email protected]>
2026-05-29 08:09 ` Fujii Masao <[email protected]>
2026-05-29 13:31 ` Fujii Masao <[email protected]>
2026-05-29 16:44 ` Srinath Reddy Sadipiralla <[email protected]>
2026-05-30 08:14 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-06-02 06:00 ` Xuneng Zhou <[email protected]>
2026-06-03 12:03 ` Fujii Masao <[email protected]>
2026-06-05 11:45 ` Xuneng Zhou <[email protected]>
2026-06-06 09:35 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-06-06 13:25 ` Xuneng Zhou <[email protected]>
2026-06-11 09:22 ` Amit Kapila <[email protected]>
2026-06-11 11:18 ` Amit Kapila <[email protected]>
2026-06-11 13:19 ` Fujii Masao <[email protected]>
2026-06-12 02:52 ` Xuneng Zhou <[email protected]>
2026-06-12 10:54 ` Amit Kapila <[email protected]>
2026-06-16 05:29 ` Xuneng Zhou <[email protected]>
2026-06-16 08:54 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-06-16 09:35 ` Amit Kapila <[email protected]>
2026-06-16 10:32 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-06-17 07:08 ` Xuneng Zhou <[email protected]>
2026-06-16 12:45 ` Fujii Masao <[email protected]>
2026-06-17 07:29 ` Xuneng Zhou <[email protected]>
2026-06-18 03:19 ` Amit Kapila <[email protected]>
2026-06-18 08:36 ` Xuneng Zhou <[email protected]>
2026-06-19 12:08 ` Amit Kapila <[email protected]>
2026-06-20 06:41 ` Xuneng Zhou <[email protected]>
2026-06-20 09:42 ` Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox