Hi,
On Tue, Mar 05, 2024 at 09:55:32AM +0200, Heikki Linnakangas wrote:
> On 01/03/2024 12:15, Bertrand Drouvot wrote:
> > Hi hackers,
> >
> > I think that pgstat_reset_replslot() is missing LWLock protection. Indeed,
> > we
> > don't have any guarantee that the slot is active (then preventing it to be
> > dropped/recreated) when this function is executed.
>
> Yes, so it seems at quick glance.
Thanks for looking at it!
> We have a similar issue in
> pgstat_fetch_replslot(); it might return stats for wrong slot, if the slot
> is dropped/recreated concurrently.
Good catch!
> Do we care?
Yeah, I think we should: done in v2 attached.
> > --- a/src/backend/utils/activity/pgstat_replslot.c
> > +++ b/src/backend/utils/activity/pgstat_replslot.c
> > @@ -46,6 +46,8 @@ pgstat_reset_replslot(const char *name)
> > Assert(name != NULL);
> > + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> > +
> > /* Check if the slot exits with the given name. */
> > slot = SearchNamedReplicationSlot(name, true);
>
> SearchNamedReplicationSlot() will also acquire the lock in LW_SHARED mode,
> when you pass need_lock=true. So that at least should be changed to false.
>
Right, done in v2. Also had to add an extra "need_lock" argument to
get_replslot_index() for the same reason while taking care of
pgstat_fetch_replslot().
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From fabee924c3131692addfe8941e4bdb3dc5540aae Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Fri, 1 Mar 2024 10:04:17 +0000
Subject: [PATCH v2] Adding LWLock protection in pgstat_reset_replslot() and
pgstat_fetch_replslot()
pgstat_reset_replslot() and pgstat_fetch_replslot() are missing a LWLock
protection as we don't have any guarantee that the slot is active (then
preventing it to be dropped/recreated) when those functions are executed.
---
src/backend/utils/activity/pgstat_replslot.c | 35 +++++++++++++++-----
1 file changed, 27 insertions(+), 8 deletions(-)
100.0% src/backend/utils/activity/
diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.c
index 70cabf2881..7c409af670 100644
--- a/src/backend/utils/activity/pgstat_replslot.c
+++ b/src/backend/utils/activity/pgstat_replslot.c
@@ -30,7 +30,7 @@
#include "utils/pgstat_internal.h"
-static int get_replslot_index(const char *name);
+static int get_replslot_index(const char *name, bool need_lock);
/*
@@ -46,8 +46,10 @@ pgstat_reset_replslot(const char *name)
Assert(name != NULL);
+ LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+
/* Check if the slot exits with the given name. */
- slot = SearchNamedReplicationSlot(name, true);
+ slot = SearchNamedReplicationSlot(name, false);
if (!slot)
ereport(ERROR,
@@ -60,11 +62,16 @@ pgstat_reset_replslot(const char *name)
* slots.
*/
if (SlotIsPhysical(slot))
+ {
+ LWLockRelease(ReplicationSlotControlLock);
return;
+ }
/* reset this one entry */
pgstat_reset(PGSTAT_KIND_REPLSLOT, InvalidOid,
ReplicationSlotIndex(slot));
+
+ LWLockRelease(ReplicationSlotControlLock);
}
/*
@@ -164,13 +171,25 @@ pgstat_drop_replslot(ReplicationSlot *slot)
PgStat_StatReplSlotEntry *
pgstat_fetch_replslot(NameData slotname)
{
- int idx = get_replslot_index(NameStr(slotname));
+ int idx;
+ PgStat_StatReplSlotEntry *slotentry;
+
+ LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+
+ idx = get_replslot_index(NameStr(slotname), false);
if (idx == -1)
+ {
+ LWLockRelease(ReplicationSlotControlLock);
return NULL;
+ }
+
+ slotentry = (PgStat_StatReplSlotEntry *) pgstat_fetch_entry(PGSTAT_KIND_REPLSLOT,
+ InvalidOid, idx);
+
+ LWLockRelease(ReplicationSlotControlLock);
- return (PgStat_StatReplSlotEntry *)
- pgstat_fetch_entry(PGSTAT_KIND_REPLSLOT, InvalidOid, idx);
+ return slotentry;
}
void
@@ -189,7 +208,7 @@ pgstat_replslot_to_serialized_name_cb(const PgStat_HashKey *key, const PgStatSha
bool
pgstat_replslot_from_serialized_name_cb(const NameData *name, PgStat_HashKey *key)
{
- int idx = get_replslot_index(NameStr(*name));
+ int idx = get_replslot_index(NameStr(*name), true);
/* slot might have been deleted */
if (idx == -1)
@@ -209,13 +228,13 @@ pgstat_replslot_reset_timestamp_cb(PgStatShared_Common *header, TimestampTz ts)
}
static int
-get_replslot_index(const char *name)
+get_replslot_index(const char *name, bool need_lock)
{
ReplicationSlot *slot;
Assert(name != NULL);
- slot = SearchNamedReplicationSlot(name, true);
+ slot = SearchNamedReplicationSlot(name, need_lock);
if (!slot)
return -1;
--
2.34.1
