Subject: Fix race in ReplicationSlotRelease for ephemeral slots

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 27 May 2026 11:50:16 +0000

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

---

From: Fujii Masao <[email-masked]>
Date: Fri, 29 May 2026 17:09:44 +0900

> writes could inadvertently affect a different, unrelated slot.

Good catch!

> 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.

> cycles for it.

+1

Regards,

-- 
Fujii Masao

---

From: Fujii Masao <[email-masked]>
Date: Fri, 29 May 2026 22:31:31 +0900

> 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-masked]>
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.

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-masked]>
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.

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-masked]>
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.

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

---

From: Srinath Reddy Sadipiralla <[email-masked]>
Date: Fri, 29 May 2026 22:14:10 +0530

Hi,

>    'test_slot_created' ? this should not be possible under normal

patch LGTM.

> 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/

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Sat, 30 May 2026 08:14:08 +0000

On Saturday, May 30, 2026 1:44 AM Srinath Reddy Sadipiralla <[email-masked]>  wrote:

> > On Wed, May 27, 2026 at 5:20 PM Zhijie Hou (Fujitsu) <mailto:[email-masked]> wrote:
> > Likelihood of encountering this bug is low, so it may not be worth adding test
> +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

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

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 2 Jun 2026 14:00:29 +0800

Hi,

> 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.

---

From: Fujii Masao <[email-masked]>
Date: Wed, 3 Jun 2026 21:03:39 +0900

> > 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.

> 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

---

From: Xuneng Zhou <[email-masked]>
Date: Fri, 5 Jun 2026 19:45:20 +0800

Hi Fujii-san,

> 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.

> 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?

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Sat, 6 Jun 2026 09:35:01 +0000

> 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.

> unlocking/logging after the cell may have been reused by another

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 protection of array lock.
>
> To solve them altogether, it might be

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

---

From: Xuneng Zhou <[email-masked]>
Date: Sat, 6 Jun 2026 21:25:39 +0800

> 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?

---

From: Amit Kapila <[email-masked]>
Date: Thu, 11 Jun 2026 14:52:24 +0530

> 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.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 11 Jun 2026 16:48:08 +0530

> 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.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Fujii Masao <[email-masked]>
Date: Thu, 11 Jun 2026 22:19:33 +0900

> synced = false and skips the actual drop. The wrong decision is

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?

> 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

---

From: Xuneng Zhou <[email-masked]>
Date: Fri, 12 Jun 2026 10:52:01 +0800

Hi Fujii-san, Amit,

The issues look real to me and could be dealt with patch v1 partially.

> the log message "dropped replication slot ..." even though no slot was

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().

> local_slot->data.database even though the lock was originally taken on

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.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 12 Jun 2026 16:24:11 +0530

On Fri, Jun 12, 2026 at 8:22 AM Xuneng Zhou <[email-masked]> wrote:
> avoiding the fatal issue over doing a broader refactoring work.
>

+1. I also think this change is not worth it.

 I
> 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.

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 16 Jun 2026 13:29:35 +0800

> +1. I also think this change is not worth it.

I am also OK with the scope of change made by patch 1.

> be done separately.

Makes sense to me.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 16 Jun 2026 08:54:11 +0000

On Tuesday, June 16, 2026 1:30 PM Xuneng Zhou <[email-masked]> wrote:
> > > On Thu, Jun 11, 2026 at 9:19 PM Fujii Masao <[email-masked]>

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 16 Jun 2026 15:05:39 +0530

> 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.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 16 Jun 2026 10:32:14 +0000

On Tuesday, June 16, 2026 5:36 PM Amit Kapila <[email-masked]> wrote:
> 
> On Tue, Jun 16, 2026 at 2:24 PM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >
> >
> > 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,
> 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

---

From: Fujii Masao <[email-masked]>
Date: Tue, 16 Jun 2026 21:45:55 +0900

> 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

---

From: Xuneng Zhou <[email-masked]>
Date: Wed, 17 Jun 2026 15:08:40 +0800

> > > (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.

> 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

Thanks for your preparation for patches and confirmation of them!

---

From: Xuneng Zhou <[email-masked]>
Date: Wed, 17 Jun 2026 15:29:09 +0800

>                         synced_slot = local_slot->in_use &&

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.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 18 Jun 2026 08:49:53 +0530

> 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.

---

From: Xuneng Zhou <[email-masked]>
Date: Thu, 18 Jun 2026 16:36:32 +0800

On Thu, Jun 18, 2026 at 11:20 AM Amit Kapila <[email-masked]> wrote:
> 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

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.
 */

> improve the existing comments and if we agree on something, that can

Thanks!

---

From: Amit Kapila <[email-masked]>
Date: Fri, 19 Jun 2026 17:38:37 +0530

>  * 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.

---

From: Xuneng Zhou <[email-masked]>
Date: Sat, 20 Jun 2026 14:41:35 +0800

>  * the race is rare and non-fatal.

LGTM. It looks well-articulated.

---

From: Amit Kapila <[email-masked]>
Date: Sat, 20 Jun 2026 15:12:42 +0530

> LGTM. It looks well-articulated.

Thanks, I'll push this as soon as the PG20 branch opens.

-- 
With Regards,
Amit Kapila.