agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: bdrouvot@amazon.com
Cc: andres@anarazel.de
Cc: stark@mit.edu
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: shared-memory based stats collector - v70
Date: Mon, 22 Aug 2022 11:32:14 +0900 (JST)
Message-ID: <20220822.113214.157880042751504547.horikyota.ntt@gmail.com> (raw)
In-Reply-To: <3d853c6f-445f-782e-eab0-525729952d4c@amazon.com>
References: <20220809165319.jdzd225xohqf6o57@awork3.anarazel.de>
<20220810.113910.92411510444435201.horikyota.ntt@gmail.com>
<3d853c6f-445f-782e-eab0-525729952d4c@amazon.com>
At Wed, 10 Aug 2022 14:02:34 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in
> what about?
>
> + /*
> + * Acquire the LWLock directly instead of using
> pg_stat_lock_entry_shared()
> + * which requires a reference.
> + */
>
>
> I think that's more consistent with other comments mentioning LWLock
> acquisition.
Sure. Thaks!. I did that in the attached.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
[text/x-patch] v2-0001-Acquire-lock-properly-when-building-stats-snapsho.patch (3.0K, ../20220822.113214.157880042751504547.horikyota.ntt@gmail.com/2-v2-0001-Acquire-lock-properly-when-building-stats-snapsho.patch)
download | inline diff:
From 202ef49a8885f46e339a6d81c723ac3b0a7d55ca Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Mon, 22 Aug 2022 11:29:07 +0900
Subject: [PATCH v2] Acquire lock properly when 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..95f09cfcc7 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);
+ /*
+ * Acquire the 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
view thread (93+ messages) latest in thread
Message-ID: <20220822.113214.157880042751504547.horikyota.ntt@gmail.com>
Permalink: ../20220822.113214.157880042751504547.horikyota.ntt@gmail.com/
Also on: postgresql.org/message-id/20220822.113214.157880042751504547.horikyota.ntt@gmail.com
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: pgsql-hackers@postgresql.org
Cc: horikyota.ntt@gmail.com, bdrouvot@amazon.com, andres@anarazel.de, stark@mit.edu, pgsql-hackers@lists.postgresql.org
Subject: Re: shared-memory based stats collector - v70
In-Reply-To: <20220822.113214.157880042751504547.horikyota.ntt@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