Subject: Re: Extension Enhancement: Buffer Invalidation in pg_buffercache

From: Nitin Jadhav <[email-masked]>
Date: Wed, 5 Jul 2023 17:53:04 +0530

+1 for the idea. It's going to be more useful to test and understand
the buffer management of PostgreSQL and it can be used to explicitly
free up the buffers if there are any such requirements.

I had a quick look over the patch. Following are the comments.

First, The TryInvalidateBuffer() tries to flush the buffer if it is
dirty and then tries to invalidate it if it meets the requirement.
Instead of directly doing this can we provide an option to the caller
to mention whether to invalidate the dirty buffers or not. For
example, TryInvalidateBuffer(Buffer bufnum, bool force), if the force
is set to FALSE, then ignore invalidating dirty buffers. Otherwise,
flush the dirty buffer and try to invalidate.

Second, In TryInvalidateBuffer(), it first checks if the reference
count is greater than zero and then checks for dirty buffers. Will
there be a scenario where the buffer is dirty and its reference count
is zero? Can you please provide more information on this or adjust the
code accordingly.

> +/*
> +Try Invalidating a buffer using bufnum.
> +If the buffer is invalid, the function returns false.
> +The function checks for dirty buffer and flushes the dirty buffer before invalidating.
> +If the buffer is still dirty it returns false.
> +*/
> +bool

The star(*) and space are missing here. Please refer to the style of
function comments and change accordingly.

Thanks & Regards,
Nitin Jadhav

On Fri, Jun 30, 2023 at 4:17 PM Palak Chaturvedi
<[email-masked]> wrote:
>
> I hope this email finds you well. I am excited to share that I have
> extended the functionality of the `pg_buffercache` extension by
> implementing buffer invalidation capability, as requested by some
> PostgreSQL contributors for improved testing scenarios.
>
> This marks my first time submitting a patch to pgsql-hackers, and I am
> eager to receive your expert feedback on the changes made. Your
> insights are invaluable, and any review or comments you provide will
> be greatly appreciated.
>
> The primary objective of this enhancement is to enable explicit buffer
> invalidation within the `pg_buffercache` extension. By doing so, we
> can simulate scenarios where buffers are invalidated and observe the
> resulting behavior in PostgreSQL.
>
> As part of this patch, a new function or mechanism has been introduced
> to facilitate buffer invalidation. I would like to hear your thoughts
> on whether this approach provides a good user interface for this
> functionality. Additionally, I seek your evaluation of the buffer
> locking protocol employed in the extension to ensure its correctness
> and efficiency.
>
> Please note that I plan to add comprehensive documentation once the
> details of this enhancement are agreed upon. This documentation will
> serve as a valuable resource for users and contributors alike. I
> believe that your expertise will help uncover any potential issues and
> opportunities for further improvement.
>
> I have attached the patch file to this email for your convenience.
> Your valuable time and consideration in reviewing this extension are
> sincerely appreciated.
>
> Thank you for your continued support and guidance. I am looking
> forward to your feedback and collaboration in enhancing the PostgreSQL
> ecosystem.
>
> The working of the extension:
>
> 1. Creating the extension pg_buffercache and then call select query on
> a table and note the buffer to be cleared.
> pgbench=# create extension pg_buffercache;
> CREATE EXTENSION
> pgbench=# select count(*) from pgbench_accounts;
>  count
> --------
>  100000
> (1 row)
>
> pgbench=# SELECT *
> FROM pg_buffercache
> WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass);
>  bufferid | relfilenode | reltablespace | reldatabase | relforknumber
> | relblocknumber | isdirty | usagecount | pinning_backends
> ----------+-------------+---------------+-------------+---------------+----------------+---------+------------+------------------
>       233 |       16397 |          1663 |       16384 |             0
> |              0 | f       |          1 |                0
>       234 |       16397 |          1663 |       16384 |             0
> |              1 | f       |          1 |                0
>       235 |       16397 |          1663 |       16384 |             0
> |              2 | f       |          1 |                0
>       236 |       16397 |          1663 |       16384 |             0
> |              3 | f       |          1 |                0
>       237 |       16397 |          1663 |       16384 |             0
> |              4 | f       |          1 |                0
>
>
> 2. Clearing a single buffer by entering the bufferid.
> pgbench=# SELECT count(*)
> FROM pg_buffercache
> WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass);
>  count
> -------
>   1660
> (1 row)
>
> pgbench=# select pg_buffercache_invalidate(233);
>  pg_buffercache_invalidate
> ---------------------------
>  t
> (1 row)
>
> pgbench=# SELECT count(*)
> FROM pg_buffercache
> WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass);
>  count
> -------
>   1659
> (1 row)
>
> 3. Clearing the entire buffer for a relation using the function.
> pgbench=# SELECT count(*)
> FROM pg_buffercache
> WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass);
>  count
> -------
>   1659
> (1 row)
>
> pgbench=# select count(pg_buffercache_invalidate(bufferid)) from
> pg_buffercache where relfilenode =
> pg_relation_filenode('pgbench_accounts'::regclass);
>  count
> -------
>   1659
> (1 row)
>
> pgbench=# SELECT count(*)
> FROM pg_buffercache
> WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass);
>  count
> -------
>      0
> (1 row)
>
>
> Best regards,
> Palak

---

From: Palak Chaturvedi <[email-masked]>
Date: Tue, 11 Jul 2023 18:08:56 +0530

Hey Nitin,
>Will
>is zero?
There might be a buffer that has been dirtied but is not pinned or
being used currently by a process. So checking the refcount and then
dirty buffers helps.
dirty and then tries to invalidate it if it meets the requirement.
Instead of directly doing this can we provide an option to the caller
to mention whether to invalidate the dirty buffers or not.
Yes that can be implemented with a default value of force. Will
implement it in the next patch.

---

From: Palak Chaturvedi <[email-masked]>
Date: Tue, 11 Jul 2023 18:39:36 +0530

Can you please review the new patch of the extension with implemented
force variable.

---

From: Cédric Villemain <[email-masked]>
Date: Wed, 3 Jan 2024 17:25:03 +0100

Hi Palak,

I did a quick review of the patch:

+CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default true)
+RETURNS bool
+AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate'
+LANGUAGE C PARALLEL SAFE;

--> Not enforced anywhere, but you can also add a comment to the 
function, for end users...

+PG_FUNCTION_INFO_V1(pg_buffercache_invalidate);
+Datum
+pg_buffercache_invalidate(PG_FUNCTION_ARGS)
+{
+    Buffer        bufnum;

"Buffer blocknum" is not correct in this context I believe. Buffer is 
when you have to manage Local buffer too (negative number).
Here uint32 is probably the good choice at the end, as used in 
pg_buffercache in other places.

Also in this extension bufferid is used, not buffernum.

+    bufnum = PG_GETARG_INT32(0);

+    if (bufnum <= 0 || bufnum > NBuffers)

maybe have a look at pageinspect and its PG_GETARG_UINT32.

+    {
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("buffernum is not valid")));

https://www.postgresql.org/docs/16/error-style-guide.html let me think 
that message like 'buffernum is not valid' can be enhanced: out of 
range, cannot be negative or exceed number of shared buffers.... ? Maybe 
add the value to the message.

+
+    }
+
+    /*
+     * Check whether to force invalidate the dirty buffer. The default 
value of force is true.
+     */
+
+    force = PG_GETARG_BOOL(1);

I think you also need to test PG_ARGISNULL with force parameter.

+/*
+ * Try Invalidating a buffer using bufnum.
+ * If the buffer is invalid, the function returns false.
+ * The function checks for dirty buffer and flushes the dirty buffer 
before invalidating.
+ * If the buffer is still dirty it returns false.
+ */
+bool
+TryInvalidateBuffer(Buffer bufnum, bool force)
+{
+    BufferDesc *bufHdr = GetBufferDescriptor(bufnum - 1);

this is not safe, GetBufferDescriptor() accepts uint, but can receive 
negative here. Use uint32 and bufferid.

+    uint32        buf_state;
+    ReservePrivateRefCountEntry();
+
+    buf_state = LockBufHdr(bufHdr);
+    if ((buf_state & BM_VALID) == BM_VALID)
+    {
+        /*
+         * The buffer is pinned therefore cannot invalidate.
+         */
+        if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
+        {
+            UnlockBufHdr(bufHdr, buf_state);
+            return false;
+        }
+        if ((buf_state & BM_DIRTY) == BM_DIRTY)
+        {
+            /*
+             * If the buffer is dirty and the user has not asked to 
clear the dirty buffer return false.
+             * Otherwise clear the dirty buffer.
+             */
+            if(!force){
+                return false;

probably need to unlockbuffer here too.

+            }
+            /*
+             * Try once to flush the dirty buffer.
+             */
+            ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+            PinBuffer_Locked(bufHdr);
+            LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), 
LW_SHARED);
+            FlushBuffer(bufHdr, NULL, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
+ LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
+            UnpinBuffer(bufHdr);

I am unsure of this area (the code is correct, but I wonder why there is 
no static code for this part -from pin to unpin- in PostgreSQL), and 
maybe better to go with FlushOneBuffer() ?
Also it is probably required to account for the shared buffer eviction 
in some pg_stat* view or table.
Not sure how disk syncing is handled after this sequence nor if it's 
important ?

+            buf_state = LockBufHdr(bufHdr);
+            if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
+            {
+                UnlockBufHdr(bufHdr, buf_state);
+                return false;
+            }
+
+            /*
+             * If its dirty again or not valid anymore give up.
+             */
+
+            if ((buf_state & (BM_DIRTY | BM_VALID)) != (BM_VALID))
+            {
+                UnlockBufHdr(bufHdr, buf_state);
+                return false;
+            }
+
+        }
+
+        InvalidateBuffer(bufHdr);
+        return true;
+    }
+    else
+    {
+        UnlockBufHdr(bufHdr, buf_state);
+        return false;
+    }

Maybe safe to remove the else {} ...
Maybe more tempting to start the big if with the following instead less 
nested...):
+    if ((buf_state & BM_VALID) != BM_VALID)
+    {
+        UnlockBufHdr(bufHdr, buf_state);
+        return false;
+    }

Doc and test are absent.

---
Cédric Villemain +33 (0)6 20 30 22 52
https://Data-Bene.io
PostgreSQL Expertise, Support, Training, R&D

---

From: Jim Nasby <[email-masked]>
Date: Wed, 3 Jan 2024 17:15:11 -0600

On 1/3/24 10:25 AM, Cédric Villemain
wrote:

Hi
Palak,

+CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default
true)

+RETURNS bool

--> Not enforced anywhere, but you can also add a comment to
the function, for end users...

The arguments should also have names...

Actually, that's true for the first argument as well. Or, just mark
the function as STRICT.

--
Jim Nasby, Data Architect, Austin TX

---

From: Cédric Villemain <[email-masked]>
Date: Sun, 14 Jan 2024 14:36:26 +0100

Hi Palak,

there is currently even more interest in your patch as it should help 
building tests for on-going development around cache/read 
management/effects.

Do you expect to be able to follow-up in the coming future ?

Thank you,
Cédric

On 04/01/2024 00:15, Jim Nasby wrote:
> On 1/3/24 10:25 AM, Cédric Villemain wrote:
>> Hi Palak,
>>
> Actually, that's true for the first argument as well. Or, just mark the 
> function as STRICT.
> 
> --

---

From: Thomas Munro <[email-masked]>
Date: Tue, 27 Feb 2024 11:41:23 +1300

[Sorry to those who received this message twice -- the first time got
bounced by the list because of a defunct email address in the CC
list.]

Here is a rebase of Palak's v2 patch.  I didn't change anything except
for the required resource manager API change, a pgindent run, and
removal of a stray file, and there is still some feedback to be
addressed before we can get this in, but I wanted to fix the bitrot
and re-open this CF item because this is very useful work.  It's
essential for testing the prefetching-related stuff happening in
various other threads, where you want to be able to get the buffer
pool into various interesting states.
From 911776e07a767e8775f9e43948f4145f72d1de65 Mon Sep 17 00:00:00 2001
From: Palak <[email-masked]>
Date: Fri, 30 Jun 2023 08:21:06 +0000
Subject: [PATCH v3] Invalidate Buffer By Bufnum

---
 contrib/pg_buffercache/Makefile               |  2 +-
 contrib/pg_buffercache/meson.build            |  1 +
 .../pg_buffercache--1.4--1.5.sql              |  6 ++
 contrib/pg_buffercache/pg_buffercache.control |  2 +-
 contrib/pg_buffercache/pg_buffercache_pages.c | 35 +++++++++
 src/backend/storage/buffer/bufmgr.c           | 74 +++++++++++++++++++
 src/include/storage/bufmgr.h                  |  2 +
 7 files changed, 120 insertions(+), 2 deletions(-)
 create mode 100644 contrib/pg_buffercache/pg_buffercache--1.4--1.5.sql

diff --git a/contrib/pg_buffercache/Makefile b/contrib/pg_buffercache/Makefile
index d6b58d4da94..eae65ead9e5 100644
--- a/contrib/pg_buffercache/Makefile
+++ b/contrib/pg_buffercache/Makefile
@@ -8,7 +8,7 @@ OBJS = \
 EXTENSION = pg_buffercache
 DATA = pg_buffercache--1.2.sql pg_buffercache--1.2--1.3.sql \
 	pg_buffercache--1.1--1.2.sql pg_buffercache--1.0--1.1.sql \
-	pg_buffercache--1.3--1.4.sql
+	pg_buffercache--1.3--1.4.sql pg_buffercache--1.4--1.5.sql
 PGFILEDESC = "pg_buffercache - monitoring of shared buffer cache in real-time"
 
 REGRESS = pg_buffercache
diff --git a/contrib/pg_buffercache/meson.build b/contrib/pg_buffercache/meson.build
index c86e33cc958..1ca3452918b 100644
--- a/contrib/pg_buffercache/meson.build
+++ b/contrib/pg_buffercache/meson.build
@@ -22,6 +22,7 @@ install_data(
   'pg_buffercache--1.2--1.3.sql',
   'pg_buffercache--1.2.sql',
   'pg_buffercache--1.3--1.4.sql',
+  'pg_buffercache--1.4--1.5.sql',
   'pg_buffercache.control',
   kwargs: contrib_data_args,
 )
diff --git a/contrib/pg_buffercache/pg_buffercache--1.4--1.5.sql b/contrib/pg_buffercache/pg_buffercache--1.4--1.5.sql
new file mode 100644
index 00000000000..c96848c77d8
--- /dev/null
+++ b/contrib/pg_buffercache/pg_buffercache--1.4--1.5.sql
@@ -0,0 +1,6 @@
+\echo Use "ALTER EXTENSION pg_buffercache UPDATE TO '1.5'" to load this file. \quit
+
+CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default true)
+RETURNS bool
+AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate'
+LANGUAGE C PARALLEL SAFE;
diff --git a/contrib/pg_buffercache/pg_buffercache.control b/contrib/pg_buffercache/pg_buffercache.control
index a82ae5f9bb5..5ee875f77dd 100644
--- a/contrib/pg_buffercache/pg_buffercache.control
+++ b/contrib/pg_buffercache/pg_buffercache.control
@@ -1,5 +1,5 @@
 # pg_buffercache extension
 comment = 'examine the shared buffer cache'
-default_version = '1.4'
+default_version = '1.5'
 module_pathname = '$libdir/pg_buffercache'
 relocatable = true
diff --git a/contrib/pg_buffercache/pg_buffercache_pages.c b/contrib/pg_buffercache/pg_buffercache_pages.c
index 33167323653..485cd988dd4 100644
--- a/contrib/pg_buffercache/pg_buffercache_pages.c
+++ b/contrib/pg_buffercache/pg_buffercache_pages.c
@@ -64,6 +64,41 @@ PG_FUNCTION_INFO_V1(pg_buffercache_pages);
 PG_FUNCTION_INFO_V1(pg_buffercache_summary);
 PG_FUNCTION_INFO_V1(pg_buffercache_usage_counts);
 
+PG_FUNCTION_INFO_V1(pg_buffercache_invalidate);
+Datum
+pg_buffercache_invalidate(PG_FUNCTION_ARGS)
+{
+	Buffer		bufnum;
+	bool		result;
+	bool		force;
+
+	if (PG_ARGISNULL(0))
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("buffernum cannot be NULL")));
+	}
+
+	bufnum = PG_GETARG_INT32(0);
+	if (bufnum <= 0 || bufnum > NBuffers)
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("buffernum is not valid")));
+
+	}
+
+	/*
+	 * Check whether to force invalidate the dirty buffer. The default value
+	 * of force is true.
+	 */
+
+	force = PG_GETARG_BOOL(1);
+
+	result = TryInvalidateBuffer(bufnum, force);
+	PG_RETURN_BOOL(result);
+}
+
 Datum
 pg_buffercache_pages(PG_FUNCTION_ARGS)
 {
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index bdf89bbc4dc..1d0f185edbc 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5705,3 +5705,77 @@ ResOwnerPrintBufferPin(Datum res)
 {
 	return DebugPrintBufferRefcount(DatumGetInt32(res));
 }
+
+/*
+ * Try Invalidating a buffer using bufnum.
+ * If the buffer is invalid, the function returns false.
+ * The function checks for dirty buffer and flushes the dirty buffer before invalidating.
+ * If the buffer is still dirty it returns false.
+ */
+bool
+TryInvalidateBuffer(Buffer bufnum, bool force)
+{
+	BufferDesc *bufHdr = GetBufferDescriptor(bufnum - 1);
+	uint32		buf_state;
+
+	ReservePrivateRefCountEntry();
+
+	buf_state = LockBufHdr(bufHdr);
+	if ((buf_state & BM_VALID) == BM_VALID)
+	{
+		/*
+		 * The buffer is pinned therefore cannot invalidate.
+		 */
+		if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
+		{
+			UnlockBufHdr(bufHdr, buf_state);
+			return false;
+		}
+		if ((buf_state & BM_DIRTY) == BM_DIRTY)
+		{
+			/*
+			 * If the buffer is dirty and the user has not asked to clear the
+			 * dirty buffer return false. Otherwise clear the dirty buffer.
+			 */
+			if (!force)
+			{
+				return false;
+			}
+
+			/*
+			 * Try once to flush the dirty buffer.
+			 */
+			ResourceOwnerEnlarge(CurrentResourceOwner);
+			PinBuffer_Locked(bufHdr);
+			LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
+			FlushBuffer(bufHdr, NULL, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
+			LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
+			UnpinBuffer(bufHdr);
+			buf_state = LockBufHdr(bufHdr);
+			if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
+			{
+				UnlockBufHdr(bufHdr, buf_state);
+				return false;
+			}
+
+			/*
+			 * If its dirty again or not valid anymore give up.
+			 */
+
+			if ((buf_state & (BM_DIRTY | BM_VALID)) != (BM_VALID))
+			{
+				UnlockBufHdr(bufHdr, buf_state);
+				return false;
+			}
+
+		}
+
+		InvalidateBuffer(bufHdr);
+		return true;
+	}
+	else
+	{
+		UnlockBufHdr(bufHdr, buf_state);
+		return false;
+	}
+}
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index d51d46d3353..a7d2dd2de0a 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -250,6 +250,8 @@ extern bool HoldingBufferPinThatDelaysRecovery(void);
 
 extern bool BgBufferSync(struct WritebackContext *wb_context);
 
+extern bool TryInvalidateBuffer(Buffer bufnum, bool force);
+
 /* in buf_init.c */
 extern void InitBufferPool(void);
 extern Size BufferShmemSize(void);
-- 
2.39.2