Hi,
On Wed, Mar 06, 2024 at 10:24:46AM +0530, shveta malik wrote:
> On Tue, Mar 5, 2024 at 6:52 PM Bertrand Drouvot
> <bertranddrouvot...@gmail.com> wrote:
> Thanks. Can we try to get rid of multiple LwLockRelease in
> pgstat_reset_replslot(). Is this any better?
>
> /*
> - * Nothing to do for physical slots as we collect stats only for
> logical
> - * slots.
> + * Reset stats if it is a logical slot. Nothing to do for physical
> slots
> + * as we collect stats only for logical slots.
> */
> - if (SlotIsPhysical(slot))
> - {
> - LWLockRelease(ReplicationSlotControlLock);
> - return;
> - }
> -
> - /* reset this one entry */
> - pgstat_reset(PGSTAT_KIND_REPLSLOT, InvalidOid,
> - ReplicationSlotIndex(slot));
> + if (SlotIsLogical(slot))
> + pgstat_reset(PGSTAT_KIND_REPLSLOT, InvalidOid,
> + ReplicationSlotIndex(slot));
>
> LWLockRelease(ReplicationSlotControlLock);
>
Yeah, it's easier to read and probably reduce the pgstat_replslot.o object file
size a bit for non optimized build.
> Something similar in pgstat_fetch_replslot() perhaps?
Yeah, all of the above done in v3 attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From ed9c97b3b606f63be29a8aba81b1daf613146b70 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Fri, 1 Mar 2024 10:04:17 +0000
Subject: [PATCH v3] 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 | 42 ++++++++++++--------
1 file changed, 25 insertions(+), 17 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..6db34e31b6 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,
@@ -56,15 +58,14 @@ pgstat_reset_replslot(const char *name)
name)));
/*
- * Nothing to do for physical slots as we collect stats only for logical
- * slots.
+ * Reset stats if it is a logical slot. Nothing to do for physical slots
+ * as we collect stats only for logical slots.
*/
- if (SlotIsPhysical(slot))
- return;
+ if (SlotIsLogical(slot))
+ pgstat_reset(PGSTAT_KIND_REPLSLOT, InvalidOid,
+ ReplicationSlotIndex(slot));
- /* reset this one entry */
- pgstat_reset(PGSTAT_KIND_REPLSLOT, InvalidOid,
- ReplicationSlotIndex(slot));
+ LWLockRelease(ReplicationSlotControlLock);
}
/*
@@ -164,13 +165,20 @@ pgstat_drop_replslot(ReplicationSlot *slot)
PgStat_StatReplSlotEntry *
pgstat_fetch_replslot(NameData slotname)
{
- int idx = get_replslot_index(NameStr(slotname));
+ int idx;
+ PgStat_StatReplSlotEntry *slotentry = NULL;
- if (idx == -1)
- return NULL;
+ LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+
+ idx = get_replslot_index(NameStr(slotname), false);
+
+ if (idx != -1)
+ 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 +197,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 +217,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
