public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bertrand Drouvot <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: [email protected]
Subject: Re: Missing LWLock protection in pgstat_reset_replslot()
Date: Tue, 5 Mar 2024 13:20:54 +0000
Message-ID: <ZeccNgXdFD79GMN/@ip-10-97-1-34.eu-west-3.compute.internal> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
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
Attachments:
[text/x-diff] v2-0001-Adding-LWLock-protection-in-pgstat_reset_replslot.patch (3.2K, ../ZeccNgXdFD79GMN%[email protected]/2-v2-0001-Adding-LWLock-protection-in-pgstat_reset_replslot.patch)
download | inline diff:
From fabee924c3131692addfe8941e4bdb3dc5540aae Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
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
view thread (3+ messages)
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]
Subject: Re: Missing LWLock protection in pgstat_reset_replslot()
In-Reply-To: <ZeccNgXdFD79GMN/@ip-10-97-1-34.eu-west-3.compute.internal>
* 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