Subject: Re: Extension Enhancement: Buffer Invalidation in pg_buffercache

From: Thomas Munro <[email-masked]>
Date: Thu, 4 Apr 2024 16:22:17 +1300

On Fri, Mar 8, 2024 at 6:20 AM Maxim Orlov <[email-masked]> wrote:
> Quite an interesting patch, in my opinion.  I've decided to work on it a bit, did some refactoring (sorry) and add
> basic tests. Also, I try to take into account as much as possible notes on the patch, mentioned by Cédric Villemain.

Thanks!  Unfortunately I don't think it's possible to include a
regression test that looks at the output, because it'd be
non-deterministic.  Any other backend could pin or dirty the buffer
you try to evict, changing the behaviour.

> > and maybe better to go with FlushOneBuffer() ?
> It's a good idea, but I'm not sure at the moment.  I'll try to dig some deeper into it.  At least, FlushOneBuffer does
> not work for a local buffers.  So, we have to decide whatever pg_buffercache_invalidate should or should not
> work for local buffers.  For now, I don't see why it should not.  Maybe I miss something?

I think it's OK to ignore local buffers for now.  pg_buffercache
generally doesn't support/show them so I don't feel inclined to
support them for this.  I removed a few traces of local support.

It didn't seem appropriate to use the pg_monitor role for this, so I
made it superuser-only.  I don't think it makes much sense to use this
on any kind of production system so I don't think we need a new role
for it, and existing roles don't seem too appropriate.  pageinspect et
al use the same approach.

I added a VOLATILE qualifier to the function.

I added some documentation.

I changed the name to pg_buffercache_evict().

I got rid of the 'force' flag which was used to say 'I only want to
evict this buffer it is clean'.  I don't really see the point in that,
we might as well keep it simple.  You could filter buffers on
"isdirty" if you want.

I added comments to scare anyone off using EvictBuffer() for anything
much, and marking it as something for developer convenience.  (I am
aware of an experimental patch that uses this same function as part of
a buffer pool resizing operation, but that has other infrastructure to
make that safe and would adjust those remarks accordingly.)

I wondered whether it should really be testing for  BM_TAG_VALID
rather than BM_VALID.  Arguably, but it doesn't seem important for
now.  The distinction would arise if someone had tried to read in a
buffer, got an I/O error and abandoned ship, leaving a buffer with a
valid tag but not valid contents.  Anyone who tries to ReadBuffer() it
will then try to read it again, but in the meantime this function
won't be able to evict it (it'll just return false).  Doesn't seem
that obvious to me that this obscure case needs to be handled.  That
doesn't happen *during* a non-error case, because then it's pinned and
we already return false in this code for pins.

I contemplated whether InvalidateBuffer() or InvalidateVictimBuffer()
would be better here and realised that Andres's intuition was probably
right when he suggested the latter up-thread.  It is designed with the
right sort of arbitrary concurrent activity in mind, where the former
assumes things about locking and dropping, which could get us into
trouble if not now maybe in the future.

I ran the following diabolical buffer blaster loop while repeatedly
running installcheck:

do
$$
begin
  loop
    perform pg_buffercache_evict(bufferid)
       from pg_buffercache
      where random() <= 0.25;
  end loop;
End;
$$;

The only ill-effect was a hot laptop.

Thoughts, objections, etc?

Very simple example of use:

create or replace function uncache_relation(name text)
returns boolean
begin atomic;
  select bool_and(pg_buffercache_evict(bufferid))
    from pg_buffercache
   where reldatabase = (select oid
                          from pg_database
                         where datname = current_database())
     and relfilenode = pg_relation_filenode(name);
end;

More interesting for those of us hacking on streaming I/O stuff was
the ability to evict just parts of things and see how the I/O merging
and I/O depth react.

---

From: Thomas Munro <[email-masked]>
Date: Sun, 7 Apr 2024 11:07:58 +1200

On second thoughts, I think the original "invalidate" terminology was
fine, no need to invent a new term.

I thought of a better name for the bufmgr.c function though:
InvalidateUnpinnedBuffer().  That name seemed better to me after I
festooned it with warnings about why exactly it's inherently racy and
only for testing use.

I suppose someone could propose an additional function
pg_buffercache_invalidate(db, tbspc, rel, fork, blocknum) that would
be slightly better in the sense that it couldn't accidentally evict
some innocent block that happened to replace the real target just
before it runs, but I don't think it matters much for this purpose and
it would still be racy on return (vacuum decides to load your block
back in) so I don't think it's worth bothering with.

So this is the version I plan to commit.
From 6a13349b788c8539b2d349f9553706d7c563f8f8 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Sun, 7 Apr 2024 09:13:17 +1200
Subject: [PATCH v6] Add pg_buffercache_invalidate() function for testing.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When testing buffer pool logic, it is useful to be able to evict
arbitrary blocks.  This function can be used in SQL queries over the
pg_buffercache view to set up a wide range of buffer pool states.  Of
course, buffer mappings might change concurrently so you might evict a
block other than the one you had in mind, and another session might
bring it back in at any time.  That's OK for the intended purpose of
setting up developer testing scenarios, and more complicated interlocking
schemes to give stronger guararantees about that would likely be less
flexible for actual testing work anyway.  Superuser-only.

Author: Palak Chaturvedi <[email-masked]>
Author: Thomas Munro <[email-masked]> (docs, small tweaks)
Reviewed-by: Nitin Jadhav <[email-masked]>
Reviewed-by: Andres Freund <[email-masked]>
Reviewed-by: Cary Huang <[email-masked]>
Reviewed-by: Cédric Villemain <[email-masked]>
Reviewed-by: Jim Nasby <[email-masked]>
Reviewed-by: Maxim Orlov <[email-masked]>
Reviewed-by: Thomas Munro <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 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 | 20 ++++++
 doc/src/sgml/pgbuffercache.sgml               | 37 +++++++++--
 src/backend/storage/buffer/bufmgr.c           | 63 +++++++++++++++++++
 src/include/storage/bufmgr.h                  |  2 +
 8 files changed, 125 insertions(+), 8 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 d6b58d4da9..eae65ead9e 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 c86e33cc95..1ca3452918 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 0000000000..92c530bc19
--- /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)
+RETURNS bool
+AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate'
+LANGUAGE C PARALLEL SAFE VOLATILE STRICT;
diff --git a/contrib/pg_buffercache/pg_buffercache.control b/contrib/pg_buffercache/pg_buffercache.control
index a82ae5f9bb..5ee875f77d 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 3316732365..9617bfa47b 100644
--- a/contrib/pg_buffercache/pg_buffercache_pages.c
+++ b/contrib/pg_buffercache/pg_buffercache_pages.c
@@ -63,6 +63,7 @@ typedef struct
 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_pages(PG_FUNCTION_ARGS)
@@ -347,3 +348,22 @@ pg_buffercache_usage_counts(PG_FUNCTION_ARGS)
 
 	return (Datum) 0;
 }
+
+/*
+ * Try to invalidate a shared buffer.
+ */
+Datum
+pg_buffercache_invalidate(PG_FUNCTION_ARGS)
+{
+	Buffer		buf = PG_GETARG_INT32(0);
+
+	if (!superuser())
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("must be superuser to use pg_buffercache_invalidate function")));
+
+	if (buf < 1 || buf > NBuffers)
+		elog(ERROR, "bad buffer ID: %d", buf);
+
+	PG_RETURN_BOOL(InvalidateUnpinnedBuffer(buf));
+}
diff --git a/doc/src/sgml/pgbuffercache.sgml b/doc/src/sgml/pgbuffercache.sgml
index afe2d97834..ec6e9e900f 100644
--- a/doc/src/sgml/pgbuffercache.sgml
+++ b/doc/src/sgml/pgbuffercache.sgml
@@ -21,11 +21,16 @@
   <primary>pg_buffercache_summary</primary>
  </indexterm>
 
+ <indexterm>
+  <primary>pg_buffercache_invalidate</primary>
+ </indexterm>
+
  <para>
   This module provides the <function>pg_buffercache_pages()</function>
   function (wrapped in the <structname>pg_buffercache</structname> view),
-  the <function>pg_buffercache_summary()</function> function, and the
-  <function>pg_buffercache_usage_counts()</function> function.
+  the <function>pg_buffercache_summary()</function> function, the
+  <function>pg_buffercache_usage_counts()</function> function and
+  the <function>pg_buffercache_invalidate()</function> function.
  </para>
 
  <para>
@@ -47,9 +52,15 @@
  </para>
 
  <para>
-  By default, use is restricted to superusers and roles with privileges of the
-  <literal>pg_monitor</literal> role. Access may be granted to others
-  using <command>GRANT</command>.
+  By default, use of the above functions is restricted to superusers and roles
+  with privileges of the <literal>pg_monitor</literal> role. Access may be
+  granted to others using <command>GRANT</command>.
+ </para>
+
+ <para>
+  The <function>pg_buffercache_invalidate()</function> function allows a block to
+  be evicted from the buffer pool given a buffer identifier.  Use of this
+  function is restricted to superusers only.
  </para>
 
  <sect2 id="pgbuffercache-pg-buffercache">
@@ -351,7 +362,21 @@
   </para>
  </sect2>
 
- <sect2 id="pgbuffercache-sample-output">
+ <sect2 id="pgbuffercache-pg-buffercache-invalidate">
+  <title>The <structname>pg_buffercache_invalidate</structname> Function</title>
+  <para>
+   The <function>pg_buffercache_invalidate()</function> function takes a buffer
+   identifier, as shown in the <structfield>bufferid</structfield> column of
+   the <structname>pg_buffercache</structname> view.  It returns true on success,
+   and false if the buffer wasn't valid, couldn't be evicted because it was
+   pinned, or became dirty again after an attempt to write it out.  The result
+   is immediately out of date upon return, as the buffer might become valid
+   again at any time due to concurrent activity.  The function is intended for
+   developer testing only.
+  </para>
+ </sect2>
+
+<sect2 id="pgbuffercache-sample-output">
   <title>Sample Output</title>
 
 <screen>
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 06e9ffd2b0..7643ea6b2c 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -6003,3 +6003,66 @@ ResOwnerPrintBufferPin(Datum res)
 {
 	return DebugPrintBufferRefcount(DatumGetInt32(res));
 }
+
+/*
+ * Try to invalidate a shared buffer.
+ *
+ * This function is intended for testing/development use only!
+ *
+ * To succeed, the buffer should not be pinned on entry, so if the caller had a
+ * particular block in mind, it might already have been replaced by some other
+ * block by the time this function runs.  It's also unpinned on return, so the
+ * buffer might be occupied again by the time control is returned, potentially
+ * even by the same block.  This inherent raciness without other interlocking
+ * makes the function unsuitable for non-testing usage.
+ *
+ * Returns true if the buffer was valid and it has now been made invalid.
+ * Returns false if the wasn't valid, or it couldn't be evicted due to a pin,
+ * or if the buffer becomes dirty again while we're trying to write it out.
+ */
+bool
+InvalidateUnpinnedBuffer(Buffer buf)
+{
+	BufferDesc *desc;
+	uint32		buf_state;
+	bool		result;
+
+	/* Make sure we can pin the buffer. */
+	ResourceOwnerEnlarge(CurrentResourceOwner);
+	ReservePrivateRefCountEntry();
+
+	Assert(!BufferIsLocal(buf));
+	desc = GetBufferDescriptor(buf - 1);
+
+	/* Lock the header and check if it's valid. */
+	buf_state = LockBufHdr(desc);
+	if ((buf_state & BM_VALID) == 0)
+	{
+		UnlockBufHdr(desc, buf_state);
+		return false;
+	}
+
+	/* Check that it's not pinned already. */
+	if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
+	{
+		UnlockBufHdr(desc, buf_state);
+		return false;
+	}
+
+	PinBuffer_Locked(desc);		/* releases spinlock */
+
+	/* If it was dirty, try to clean it once. */
+	if (buf_state & BM_DIRTY)
+	{
+		LWLockAcquire(BufferDescriptorGetContentLock(desc), LW_SHARED);
+		FlushBuffer(desc, NULL, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
+		LWLockRelease(BufferDescriptorGetContentLock(desc));
+	}
+
+	/* This will return false if it becomes dirty or someone else pins it. */
+	result = InvalidateVictimBuffer(desc);
+
+	UnpinBuffer(desc);
+
+	return result;
+}
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 07ba1a6050..cce8eddbdd 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -305,6 +305,8 @@ extern bool BgBufferSync(struct WritebackContext *wb_context);
 extern void LimitAdditionalPins(uint32 *additional_pins);
 extern void LimitAdditionalLocalPins(uint32 *additional_pins);
 
+extern bool InvalidateUnpinnedBuffer(Buffer buf);
+
 /* in buf_init.c */
 extern void InitBufferPool(void);
 extern Size BufferShmemSize(void);
-- 
2.44.0

---

From: Melanie Plageman <[email-masked]>
Date: Sun, 7 Apr 2024 19:53:22 -0400

> So this is the version I plan to commit.

I've reviewed v6. I think you should mention in the docs that it only
works for shared buffers -- so specifically not buffers containing
blocks of temp tables.

In the function pg_buffercache_invalidate(), why not use the
BufferIsValid() function?

-   if (buf < 1 || buf > NBuffers)
+   if (!BufferIsValid(buf) || buf > NBuffers)

I thought the below would be more clear for the comment above
InvalidateUnpinnedBuffer().

- * Returns true if the buffer was valid and it has now been made invalid.
- * Returns false if the wasn't valid, or it couldn't be evicted due to a pin,
- * or if the buffer becomes dirty again while we're trying to write it out.
+ * Returns true if the buffer was valid and has now been made invalid. Returns
+ * false if it wasn't valid, if it couldn't be evicted due to a pin, or if the
+ * buffer becomes dirty again while we're trying to write it out.

Some of that probably applies for the docs too (i.e. you have some
similar wording in the docs). There is actually one typo in your
version, so even if you don't adopt my suggestion, you should fix that
typo.

I didn't notice anything else out of place. I tried it and it worked
as expected. I'm excited to have this feature!

I didn't read through this whole thread, but was there any talk of
adding other functions to let me invalidate a bunch of buffers at once
or even some options -- like invalidate every 3rd buffer or whatever?
(Not the concern of this patch, but just wondering because that would
be a useful future enhancement IMO).

- Melanie

---

From: Andres Freund <[email-masked]>
Date: Sun, 7 Apr 2024 17:10:13 -0700

Hi,

> festooned it with warnings about why exactly it's inherently racy and

I still dislike that, fwiw, due to the naming similarity to
InvalidateBuffer(), which throws away dirty buffer contents too. Which
obviously isn't acceptable from "userspace".  I'd just name it
pg_buffercache_evict() - given that the commit message's first paragraph uses
"it is useful to be able to evict arbitrary blocks" that seems to describe
things at least as well as "invalidate"?

Greetings,

Andres Freund

---

From: Thomas Munro <[email-masked]>
Date: Mon, 8 Apr 2024 16:30:34 +1200

> things at least as well as "invalidate"?

Alright, sold.  I'll go with EvictUnpinnedBuffer() in bufmgr.c and
pg_buffercache_evict() in the contrib module.

---

From: Maksim Milyutin <[email-masked]>
Date: Sun, 14 Apr 2024 21:16:15 +0300

On 07.04.2024 02:07, Thomas Munro wrote:

> +EvictUnpinnedBuffer(Buffer buf)
> +{
> ...

Hi, Thomas!

Should not we call at the end the StrategyFreeBuffer() function to add 
target buffer to freelist and not miss it after invalidation?

-- 
Best regards,
Maksim Milyutin

---

From: Maksim Milyutin <[email-masked]>
Date: Mon, 29 Apr 2024 21:47:41 +0300

> target buffer to freelist and not miss it after invalidation?

Hello everyone!

Please take a look at this issue, current implementation of 
EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost 
permanently and will not be reused again

-- 
Best regards,
Maksim Milyutin