Date: Mon, 22 Aug 2022 11:32:14 +0900 (JST)
Message-Id: <20220822.113214.157880042751504547.horikyota.ntt@gmail.com>
To: bdrouvot@amazon.com
Cc: andres@anarazel.de, stark@mit.edu, pgsql-hackers@lists.postgresql.org
Subject: Re: shared-memory based stats collector - v70
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
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>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Mon_Aug_22_11_32_14_2022_519)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20220822.113214.157880042751504547.horikyota.ntt%40gmail.com>
Precedence: bulk

----Next_Part(Mon_Aug_22_11_32_14_2022_519)--
Content-Type: Text/Plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable

At Wed, 10 Aug 2022 14:02:34 +0200, "Drouvot, Bertrand" <bdrouvot@amazo=
n.com> wrote in =

> what about?
> =

> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /*
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * Acquire the LWLock d=
irectly instead of using
> pg_stat_lock_entry_shared()
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * which requires a ref=
erence.
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 */
> =

> =

> 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

----Next_Part(Mon_Aug_22_11_32_14_2022_519)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="v2-0001-Acquire-lock-properly-when-building-stats-snapsho.patch"

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


----Next_Part(Mon_Aug_22_11_32_14_2022_519)----