Subject: Re: Stats collector's idx_blks_hit value is highly misleading in practice

From: John Naylor <[email-masked]>
Date: Thu, 3 Feb 2022 19:08:22 -0500

On Fri, Oct 30, 2020 at 9:46 PM Tomas Vondra <[email-masked]>
wrote:
>
> On Fri, Oct 16, 2020 at 03:35:51PM -0700, Peter Geoghegan wrote:

> >I suppose that a change like this could end up affecting other things,
> >such as EXPLAIN ANALYZE statistics. OTOH we only break out index pages
> >separately for bitmap scans at the moment, so maybe it could be fairly
> >well targeted. And, maybe this is unappealing given the current
> >statistics collector limitations. I'm not volunteering to work on it
> >right now, but it would be nice to fix this. Please don't wait for me
> >to do it.
> >
>
> It seems to me this should not be a particularly difficult patch in
> principle, so suitable for new contributors. The main challenge would be
> passing information about what page we're dealing with (internal/leaf)
> to the place actually calling pgstat_count_buffer_(read|hit). That
> happens in ReadBufferExtended, which just has no idea what page it's
> dealing with. Not sure how to do that cleanly ...

Is this a TODO candidate? What would be a succinct title for it?

--
John Naylor
EDB: http://www.enterprisedb.com

---

From: Peter Geoghegan <[email-masked]>
Date: Fri, 4 Feb 2022 11:18:43 -0500

> Is this a TODO candidate? What would be a succinct title for it?

I definitely think that it's worth working on. I suppose it follows
that it should go on the TODO list.

-- 
Peter Geoghegan

---

From: John Naylor <[email-masked]>
Date: Fri, 4 Feb 2022 11:39:12 -0500

> that it should go on the TODO list.

Added TODO item "Teach stats collector to differentiate between
internal and leaf index pages"

---

From: Sergey Dudoladov <[email-masked]>
Date: Wed, 29 Jun 2022 22:42:44 +0200

Hello,

I would like to get some feedback on that task.

> pg_statio_*_tables.idx_blks_hit are highly misleading in practice
> because they fail to take account of the difference between internal
> pages and leaf pages in B-Tree indexes.

I see it is still the case, so the issue is relevant, isn't it ?

> The main challenge would be

I do not immediately see the way to pass the information in a
completely clean manner.

Either
(1) ReadBufferExtended needs to know the type of an index page (leaf/internal)
or
(2) caller of ReadBufferExtended that can check the page type needs to learn
if there was a hit and call pgstat_count_buffer_(read|hit) accordingly.

In either case necessary code changes seem quite invasive to me.
I have attached a code snippet to illustrate the second idea.

Regards,
Sergey
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 20adb602a4..d8c22be949 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -31,6 +31,7 @@
 #include "miscadmin.h"
 #include "storage/indexfsm.h"
 #include "storage/lmgr.h"
+#include "pgstat.h"
 #include "storage/predicate.h"
 #include "storage/procarray.h"
 #include "utils/memdebug.h"
@@ -870,14 +871,23 @@ _bt_log_reuse_page(Relation rel, BlockNumber blkno, FullTransactionId safexid)
 Buffer
 _bt_getbuf(Relation rel, BlockNumber blkno, int access)
 {
-	Buffer		buf;
+	Buffer			buf;
+	Page			page;
+	BTPageOpaque	opaque;
 
 	if (blkno != P_NEW)
 	{
+		bool		hit;
 		/* Read an existing block of the relation */
-		buf = ReadBuffer(rel, blkno);
+		buf = ReadIndexBuffer(rel, blkno, &hit);
 		_bt_lockbuf(rel, buf, access);
 		_bt_checkpage(rel, buf);
+
+		page = BufferGetPage(buf);
+		opaque = BTPageGetOpaque(page);
+		if (hit && P_ISLEAF(opaque))
+		pgstat_count_buffer_hit(rel);
+
 	}
 	else
 	{
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index ae13011d27..ab9522fd50 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -704,6 +704,16 @@ ReadBuffer(Relation reln, BlockNumber blockNum)
 	return ReadBufferExtended(reln, MAIN_FORKNUM, blockNum, RBM_NORMAL, NULL);
 }
 
+/*
+ * ReadIndexBuffer -- a shorthand for ReadIndexBufferExtended, for reading from main
+ *		fork with RBM_NORMAL mode and default strategy.
+ */
+Buffer
+ReadIndexBuffer(Relation reln, BlockNumber blockNum, bool *hit)
+{
+	return ReadIndexBufferExtended(reln, MAIN_FORKNUM, blockNum, RBM_NORMAL, NULL, hit);
+}
+
 /*
  * ReadBufferExtended -- returns a buffer containing the requested
  *		block of the requested relation.  If the blknum
@@ -774,6 +784,34 @@ ReadBufferExtended(Relation reln, ForkNumber forkNum, BlockNumber blockNum,
 	return buf;
 }
 
+/*
+ * Proof-of-concept.
+ *
+ * Same as ReadBufferExtended but returns the value of "hit" to improve 
+ * statistics collection for indexes.
+ */
+Buffer
+ReadIndexBufferExtended(Relation reln, ForkNumber forkNum, BlockNumber blockNum,
+				   ReadBufferMode mode, BufferAccessStrategy strategy, bool *hit)
+{
+	Buffer		buf;
+
+	/*
+	 * Reject attempts to read non-local temporary relations; we would be
+	 * likely to get wrong data since we have no visibility into the owning
+	 * session's local buffers.
+	 */
+	if (RELATION_IS_OTHER_TEMP(reln))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("cannot access temporary tables of other sessions")));
+
+	buf = ReadBuffer_common(RelationGetSmgr(reln), reln->rd_rel->relpersistence,
+							forkNum, blockNum, mode, strategy, hit);
+
+	return buf;
+}
+
 
 /*
  * ReadBufferWithoutRelcache -- like ReadBufferExtended, but doesn't require
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 58391406f6..d4483c1666 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -179,9 +179,13 @@ extern PrefetchBufferResult PrefetchBuffer(Relation reln, ForkNumber forkNum,
 extern bool ReadRecentBuffer(RelFileNode rnode, ForkNumber forkNum,
 							 BlockNumber blockNum, Buffer recent_buffer);
 extern Buffer ReadBuffer(Relation reln, BlockNumber blockNum);
+extern Buffer ReadIndexBuffer(Relation reln, BlockNumber blockNum, bool *hit);
 extern Buffer ReadBufferExtended(Relation reln, ForkNumber forkNum,
 								 BlockNumber blockNum, ReadBufferMode mode,
 								 BufferAccessStrategy strategy);
+extern Buffer ReadIndexBufferExtended(Relation reln, ForkNumber forkNum,
+									  BlockNumber blockNum, ReadBufferMode mode,
+									  BufferAccessStrategy strategy, bool *hit);
 extern Buffer ReadBufferWithoutRelcache(RelFileNode rnode,
 										ForkNumber forkNum, BlockNumber blockNum,
 										ReadBufferMode mode, BufferAccessStrategy strategy,

---

From: Sergey Dudoladov <[email-masked]>
Date: Thu, 11 Aug 2022 07:42:04 +0200

Hi again,

Having played with the task for a little while,  I am no longer sure
it completely justifies the effort involved.
The reason being the task requires modifying the buffer pool in one
way or the other, which implies
(a) significant effort on performance testing and
(b) changes in the buffer pool interfaces that community might not
welcome just to get 1-2 extra statistics numbers.

Any ideas ?

Regards,
Sergey