public inbox for [email protected]
help / color / mirror / Atom feedFrom: Xuneng Zhou <[email protected]>
To: Fujii Masao <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Srinath Reddy Sadipiralla <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Fix race in ReplicationSlotRelease for ephemeral slots
Date: Wed, 17 Jun 2026 15:29:09 +0800
Message-ID: <CABPTF7UCqndPh8jucFtWBpFMoA2oQkSObQGXVVQNVGMZ1q-DCg@mail.gmail.com> (raw)
In-Reply-To: <CAHGQGwGGyEDL3dh7uJ6qPsGvnq4QK_R8+U=12CaprnzwrwaLGA@mail.gmail.com>
References: <TY4PR01MB177184FF9EE916F577E1F554194082@TY4PR01MB17718.jpnprd01.prod.outlook.com>
<CAFC+b6o-hD5VxVLZQovmHSYykF8Qzq3eiuBU-U1F_yR9-y6P_w@mail.gmail.com>
<TY4PR01MB177180A7CE60BCDF286B1C6F594172@TY4PR01MB17718.jpnprd01.prod.outlook.com>
<CABPTF7VyH1-W2xnDspECDEzFGQj=WTFpZBCqKfM11OAZa6gQHQ@mail.gmail.com>
<CAHGQGwE+2WSqiAYgNJRkf_twdB+uRGozjjGhUn76vUKZ8dzbSA@mail.gmail.com>
<CABPTF7VeA8szPv7LYDVY9_7LftV-HM8NFVQR2natPKmr73JW+A@mail.gmail.com>
<TY4PR01MB1771887D33612C5A45F7E9CDF941E2@TY4PR01MB17718.jpnprd01.prod.outlook.com>
<CAA4eK1LqFBKCkX2eoX3iQPxJJnzWTaCpdh9zNotxuoG8BgjdtA@mail.gmail.com>
<CAA4eK1LkRdbm5XA=qa82Rp_y4rnyJh8pypMWVqOezOZpzy=Oaw@mail.gmail.com>
<CAHGQGwG_3ff4HciHtTZ_uMvbJgSDWsz4Yawj_zQpDG6Yj=Mjng@mail.gmail.com>
<CABPTF7WBh_mKi60EYLiueaZ_cdJvnrOrpSt3hQkuZ_uY4w5duA@mail.gmail.com>
<CAA4eK1LJ9=BJU2oK5aFCfvW=w2muSXNHOPM18wHXHLkRzYxhTQ@mail.gmail.com>
<CAHGQGwGGyEDL3dh7uJ6qPsGvnq4QK_R8+U=12CaprnzwrwaLGA@mail.gmail.com>
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
view thread (27+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Fix race in ReplicationSlotRelease for ephemeral slots
In-Reply-To: <CABPTF7UCqndPh8jucFtWBpFMoA2oQkSObQGXVVQNVGMZ1q-DCg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox