At Tue, 9 Aug 2022 09:53:19 -0700, Andres Freund <and...@anarazel.de> wrote in
> Hi,
>
> On 2022-08-09 17:24:35 +0900, Kyotaro Horiguchi wrote:
> > If I'm not missing something, it's strange that pgstat_lock_entry()
> > only takes LW_EXCLUSIVE.
>
> I think it makes some sense, given that there's a larger number of callers for
> that in various stats-emitting code. Perhaps we could just add a separate
> function with a _shared() suffix?
Sure. That was an alternative I had in my mind.
> > The atached changes the interface of
> > pgstat_lock_entry() but there's only one user since another read of
> > shared stats entry is not using reference. Thus the interface change
> > might be too much. If I just add bare LWLockAcquire/Release() to
> > pgstat_fetch_entry,the amount of the patch will be reduced.
>
> Could you try the pgstat_lock_entry_shared() approach?
Of course. Please find the attached.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From f406fd384d7ca145e37810a2f6a7a410f74d5795 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Wed, 10 Aug 2022 10:53:19 +0900
Subject: [PATCH] Take lock while building stats snapshot
It got lost somewhere while developing shared memory stats collector
but it is undoubtedly required.
---
src/backend/utils/activity/pgstat.c | 8 ++++++++
src/backend/utils/activity/pgstat_shmem.c | 12 ++++++++++++
src/include/utils/pgstat_internal.h | 1 +
3 files changed, 21 insertions(+)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 88e5dd1b2b..c23142a670 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -844,9 +844,11 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
else
stats_data = MemoryContextAlloc(pgStatLocal.snapshot.context,
kind_info->shared_data_len);
+ pgstat_lock_entry_shared(entry_ref, false);
memcpy(stats_data,
pgstat_get_entry_data(kind, entry_ref->shared_stats),
kind_info->shared_data_len);
+ pgstat_unlock_entry(entry_ref);
if (pgstat_fetch_consistency > PGSTAT_FETCH_CONSISTENCY_NONE)
{
@@ -983,9 +985,15 @@ pgstat_build_snapshot(void)
entry->data = MemoryContextAlloc(pgStatLocal.snapshot.context,
kind_info->shared_size);
+ /*
+ * Take lwlock directly instead of using pg_stat_lock_entry_shared()
+ * which requires a reference.
+ */
+ LWLockAcquire(&stats_data->lock, LW_SHARED);
memcpy(entry->data,
pgstat_get_entry_data(kind, stats_data),
kind_info->shared_size);
+ LWLockRelease(&stats_data->lock);
}
dshash_seq_term(&hstat);
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 89060ef29a..0bfa460af1 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -579,6 +579,18 @@ pgstat_lock_entry(PgStat_EntryRef *entry_ref, bool nowait)
return true;
}
+bool
+pgstat_lock_entry_shared(PgStat_EntryRef *entry_ref, bool nowait)
+{
+ LWLock *lock = &entry_ref->shared_stats->lock;
+
+ if (nowait)
+ return LWLockConditionalAcquire(lock, LW_SHARED);
+
+ LWLockAcquire(lock, LW_SHARED);
+ return true;
+}
+
void
pgstat_unlock_entry(PgStat_EntryRef *entry_ref)
{
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 9303d05427..901d2041d6 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -581,6 +581,7 @@ extern void pgstat_detach_shmem(void);
extern PgStat_EntryRef *pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, Oid objoid,
bool create, bool *found);
extern bool pgstat_lock_entry(PgStat_EntryRef *entry_ref, bool nowait);
+extern bool pgstat_lock_entry_shared(PgStat_EntryRef *entry_ref, bool nowait);
extern void pgstat_unlock_entry(PgStat_EntryRef *entry_ref);
extern bool pgstat_drop_entry(PgStat_Kind kind, Oid dboid, Oid objoid);
extern void pgstat_drop_all_entries(void);
--
2.31.1
