Extension Enhancement: Buffer Invalidation in pg_buffercache

On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi
<chaturvedipalak1911@gmail.com> wrote:

pgbench=# select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('pgbench_accounts'::regclass);

Hi Palak,

Thanks for working on this! I think this will be very useful for
testing existing workloads but also for testing future work on
prefetching with AIO (and DIO), work on putting SLRUs (or anything
else) into the buffer pool, nearby proposals for caching buffer
mapping information, etc etc.

Palak and I talked about this idea a bit last week (stimulated by a
recent thread[1]/messages/by-id/CAFSGpE3y_oMK1uHhcHxGxBxs+KrjMMdGrE+6HHOu0vttVET0UQ@mail.gmail.com, but the topic has certainly come up before), and we
discussed some different ways one could specify which pages are
dropped. For example, perhaps the pg_prewarm extension could have an
'unwarm' option instead. I personally thought the buffer ID-based
approach was quite good because it's extremely simple, while giving
the user the full power of SQL to say which buffers. Half a table?
Visibility map? Everything? Root page of an index? I think that's
probably better than something that requires more code and
complication but is less flexible in the end. It feels like the right
level of rawness for something primarily of interest to hackers and
advanced users. I don't think it matters that there is a window
between selecting a buffer ID and invalidating it, for the intended
use cases. That's my vote, anyway, let's see if others have other
ideas...

We also talked a bit about how one might control the kernel page cache
in more fine-grained ways for testing purposes, but it seems like the
pgfincore project has that covered with its pgfadvise_willneed() and
pgfadvise_dontneed(). IMHO that project could use more page-oriented
operations (instead of just counts and coarse grains operations) but
that's something that could be material for patches to send to the
extension maintainers. This work, in contrast, is more tangled up
with bufmgr.c internals, so it feels like this feature belongs in a
core contrib module.

Some initial thoughts on the patch:

I wonder if we should include a simple exercise in
contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that
it's not guaranteed to succeed in general. It doesn't wait for pins
to go away, and it doesn't retry cleaning dirty buffers after one
attempt, it just returns false, which I think is probably the right
approach, but it makes the behaviour too non-deterministic for simple
tests. Perhaps it's enough to include an exercise where we call it a
few times to hit a couple of cases, but not verify what effect it has.

It should be restricted by role, but I wonder which role it should be.
Testing for superuser is now out of fashion.

Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs
to do the same. That's because PostgreSQL is currently in transition
from autoconf/gmake to meson/ninja[2]https://wiki.postgresql.org/wiki/Meson, so for now we have to maintain
both build systems. That's why it fails to build in some CI tasks[3]http://cfbot.cputube.org/palak-chaturvedi.html.
You can enable CI in your own GitHub account if you want to run test
builds on several operating systems, see [4]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD for info.

[1]: /messages/by-id/CAFSGpE3y_oMK1uHhcHxGxBxs+KrjMMdGrE+6HHOu0vttVET0UQ@mail.gmail.com
[2]: https://wiki.postgresql.org/wiki/Meson
[3]: http://cfbot.cputube.org/palak-chaturvedi.html
[4]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD

Hi Thomas,
Thank you for your suggestions. I have added the sql in the meson
build as well.

On Mon, Jul 3, 2023 at 4:26 PM Palak Chaturvedi
<chaturvedipalak1911@gmail.com> wrote:

Hi Thomas,
Thank you for your suggestions. I have added the sql in the meson
build as well.

On Sat, 1 Jul 2023 at 03:39, Thomas Munro <thomas.munro@gmail.com> wrote:

On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi
<chaturvedipalak1911@gmail.com> wrote:

pgbench=# select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('pgbench_accounts'::regclass);

Hi Palak,

Thanks for working on this! I think this will be very useful for
testing existing workloads but also for testing future work on
prefetching with AIO (and DIO), work on putting SLRUs (or anything
else) into the buffer pool, nearby proposals for caching buffer
mapping information, etc etc.

Palak and I talked about this idea a bit last week (stimulated by a
recent thread[1], but the topic has certainly come up before), and we
discussed some different ways one could specify which pages are
dropped. For example, perhaps the pg_prewarm extension could have an
'unwarm' option instead. I personally thought the buffer ID-based
approach was quite good because it's extremely simple, while giving
the user the full power of SQL to say which buffers. Half a table?
Visibility map? Everything? Root page of an index? I think that's
probably better than something that requires more code and
complication but is less flexible in the end. It feels like the right
level of rawness for something primarily of interest to hackers and
advanced users. I don't think it matters that there is a window
between selecting a buffer ID and invalidating it, for the intended
use cases. That's my vote, anyway, let's see if others have other
ideas...

We also talked a bit about how one might control the kernel page cache
in more fine-grained ways for testing purposes, but it seems like the
pgfincore project has that covered with its pgfadvise_willneed() and
pgfadvise_dontneed(). IMHO that project could use more page-oriented
operations (instead of just counts and coarse grains operations) but
that's something that could be material for patches to send to the
extension maintainers. This work, in contrast, is more tangled up
with bufmgr.c internals, so it feels like this feature belongs in a
core contrib module.

Some initial thoughts on the patch:

I wonder if we should include a simple exercise in
contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that
it's not guaranteed to succeed in general. It doesn't wait for pins
to go away, and it doesn't retry cleaning dirty buffers after one
attempt, it just returns false, which I think is probably the right
approach, but it makes the behaviour too non-deterministic for simple
tests. Perhaps it's enough to include an exercise where we call it a
few times to hit a couple of cases, but not verify what effect it has.

It should be restricted by role, but I wonder which role it should be.
Testing for superuser is now out of fashion.

Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs
to do the same. That's because PostgreSQL is currently in transition
from autoconf/gmake to meson/ninja[2], so for now we have to maintain
both build systems. That's why it fails to build in some CI tasks[3].
You can enable CI in your own GitHub account if you want to run test
builds on several operating systems, see [4] for info.

[1] /messages/by-id/CAFSGpE3y_oMK1uHhcHxGxBxs+KrjMMdGrE+6HHOu0vttVET0UQ@mail.gmail.com
[2] https://wiki.postgresql.org/wiki/Meson
[3] http://cfbot.cputube.org/palak-chaturvedi.html
[4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD

newbie question:
quote from: https://www.interdb.jp/pg/pgsql08.html

Pinned: When the corresponding buffer pool slot stores a page and any PostgreSQL processes are accessing the page (i.e. refcount and usage_count are greater than or equal to 1), the state of this buffer descriptor is pinned.
Unpinned: When the corresponding buffer pool slot stores a page but no PostgreSQL processes are accessing the page (i.e. usage_count is greater than or equal to 1, but refcount is 0), the state of this buffer descriptor is unpinned.

So do you need to check BUF_STATE_GET_REFCOUNT(buf_state) and
BUF_STATE_GET_USAGECOUNT(state)?

hi,
I don't think we need to check the usage count. Because we are
clearing all the buffers that are not pinned.
Checking the usage count is for buffer replacement since we are not
replacing it does not matter.

On Mon, 03 Jul 2023 at 16:26, Palak Chaturvedi <chaturvedipalak1911@gmail.com> wrote:

Hi Thomas,
Thank you for your suggestions. I have added the sql in the meson
build as well.

On Sat, 1 Jul 2023 at 03:39, Thomas Munro <thomas.munro@gmail.com> wrote:

On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi
<chaturvedipalak1911@gmail.com> wrote:

pgbench=# select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('pgbench_accounts'::regclass);

Hi Palak,

Thanks for working on this! I think this will be very useful for
testing existing workloads but also for testing future work on
prefetching with AIO (and DIO), work on putting SLRUs (or anything
else) into the buffer pool, nearby proposals for caching buffer
mapping information, etc etc.

Palak and I talked about this idea a bit last week (stimulated by a
recent thread[1], but the topic has certainly come up before), and we
discussed some different ways one could specify which pages are
dropped. For example, perhaps the pg_prewarm extension could have an
'unwarm' option instead. I personally thought the buffer ID-based
approach was quite good because it's extremely simple, while giving
the user the full power of SQL to say which buffers. Half a table?
Visibility map? Everything? Root page of an index? I think that's
probably better than something that requires more code and
complication but is less flexible in the end. It feels like the right
level of rawness for something primarily of interest to hackers and
advanced users. I don't think it matters that there is a window
between selecting a buffer ID and invalidating it, for the intended
use cases. That's my vote, anyway, let's see if others have other
ideas...

We also talked a bit about how one might control the kernel page cache
in more fine-grained ways for testing purposes, but it seems like the
pgfincore project has that covered with its pgfadvise_willneed() and
pgfadvise_dontneed(). IMHO that project could use more page-oriented
operations (instead of just counts and coarse grains operations) but
that's something that could be material for patches to send to the
extension maintainers. This work, in contrast, is more tangled up
with bufmgr.c internals, so it feels like this feature belongs in a
core contrib module.

Some initial thoughts on the patch:

I wonder if we should include a simple exercise in
contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that
it's not guaranteed to succeed in general. It doesn't wait for pins
to go away, and it doesn't retry cleaning dirty buffers after one
attempt, it just returns false, which I think is probably the right
approach, but it makes the behaviour too non-deterministic for simple
tests. Perhaps it's enough to include an exercise where we call it a
few times to hit a couple of cases, but not verify what effect it has.

It should be restricted by role, but I wonder which role it should be.
Testing for superuser is now out of fashion.

Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs
to do the same. That's because PostgreSQL is currently in transition
from autoconf/gmake to meson/ninja[2], so for now we have to maintain
both build systems. That's why it fails to build in some CI tasks[3].
You can enable CI in your own GitHub account if you want to run test
builds on several operating systems, see [4] for info.

[1] /messages/by-id/CAFSGpE3y_oMK1uHhcHxGxBxs+KrjMMdGrE+6HHOu0vttVET0UQ@mail.gmail.com
[2] https://wiki.postgresql.org/wiki/Meson
[3] http://cfbot.cputube.org/palak-chaturvedi.html
[4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD

I think, zero is not a valid buffer identifier. See src/include/storage/buf.h.

+	bufnum = PG_GETARG_INT32(0);
+	if (bufnum < 0 || bufnum > NBuffers)
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("buffernum is not valid")));
+
+	}

If we use SELECT pg_buffercache_invalidate(0), it will crash.

--
Regrads,
Japin Li.

the following will also crash. no idea why.
begin;
select count(*) from onek;
select relpages from pg_class where relname = 'onek'; --queryA

SELECT count(*) FROM pg_buffercache WHERE relfilenode =
pg_relation_filenode('onek'::regclass); --queryB

insert into onek values(default);

select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('onek'::regclass);

---------------------------------
queryA returns 35, queryB returns 37.
----------------------------------
crash info:
test_dev=*# insert into onek values(default);
INSERT 0 1
test_dev=*# select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('onek'::regclass);
TRAP: failed Assert("resarr->nitems < resarr->maxitems"), File:
"../../Desktop/pg_sources/main/postgres/src/backend/utils/resowner/resowner.c",
Line: 275, PID: 1533312
postgres: jian test_dev [local]
SELECT(ExceptionalCondition+0xa1)[0x55fc8f8d14e1]
postgres: jian test_dev [local] SELECT(+0x9e7ab3)[0x55fc8f915ab3]
postgres: jian test_dev [local]
SELECT(ResourceOwnerRememberBuffer+0x1d)[0x55fc8f91696d]
postgres: jian test_dev [local] SELECT(+0x78ab17)[0x55fc8f6b8b17]
postgres: jian test_dev [local]
SELECT(TryInvalidateBuffer+0x6d)[0x55fc8f6c507d]
/home/jian/postgres/pg16_test/lib/pg_buffercache.so(pg_buffercache_invalidate+0x3d)[0x7f2361837abd]
postgres: jian test_dev [local] SELECT(+0x57eebc)[0x55fc8f4acebc]
postgres: jian test_dev [local]
SELECT(ExecInterpExprStillValid+0x3c)[0x55fc8f4a6e2c]
postgres: jian test_dev [local] SELECT(+0x5a0f16)[0x55fc8f4cef16]
postgres: jian test_dev [local] SELECT(+0x5a3588)[0x55fc8f4d1588]
postgres: jian test_dev [local] SELECT(+0x58f747)[0x55fc8f4bd747]
postgres: jian test_dev [local]
SELECT(standard_ExecutorRun+0x1f0)[0x55fc8f4b29f0]
postgres: jian test_dev [local] SELECT(ExecutorRun+0x46)[0x55fc8f4b2d16]
postgres: jian test_dev [local] SELECT(+0x7eb3b0)[0x55fc8f7193b0]
postgres: jian test_dev [local] SELECT(PortalRun+0x1eb)[0x55fc8f71b7ab]
postgres: jian test_dev [local] SELECT(+0x7e8cf4)[0x55fc8f716cf4]
postgres: jian test_dev [local] SELECT(PostgresMain+0x134f)[0x55fc8f71869f]
postgres: jian test_dev [local] SELECT(+0x70f80c)[0x55fc8f63d80c]
postgres: jian test_dev [local]
SELECT(PostmasterMain+0x1758)[0x55fc8f63f278]
postgres: jian test_dev [local] SELECT(main+0x27e)[0x55fc8f27067e]
/lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7f2361629d90]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7f2361629e40]
postgres: jian test_dev [local] SELECT(_start+0x25)[0x55fc8f272bb5]
2023-07-04 16:56:13.088 CST [1532822] LOG: server process (PID 1533312)
was terminated by signal 6: Aborted
2023-07-04 16:56:13.088 CST [1532822] DETAIL: Failed process was running:
select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('onek'::regclass);
2023-07-04 16:56:13.088 CST [1532822] LOG: terminating any other active
server processes
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: 2023-07-04
16:56:13.091 CST [1533381] FATAL: the database system is in recovery mode
Failed.
The connection to the server was lost. Attempting reset: Failed.

On Tue, 04 Jul 2023 at 17:00, jian he <jian.universality@gmail.com> wrote:

the following will also crash. no idea why.
begin;
select count(*) from onek;
select relpages from pg_class where relname = 'onek'; --queryA

SELECT count(*) FROM pg_buffercache WHERE relfilenode =
pg_relation_filenode('onek'::regclass); --queryB

insert into onek values(default);

select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('onek'::regclass);

---------------------------------
queryA returns 35, queryB returns 37.
----------------------------------
crash info:
test_dev=*# insert into onek values(default);
INSERT 0 1
test_dev=*# select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('onek'::regclass);
TRAP: failed Assert("resarr->nitems < resarr->maxitems"), File:
"../../Desktop/pg_sources/main/postgres/src/backend/utils/resowner/resowner.c",
Line: 275, PID: 1533312

According to the comments of ResourceArrayAdd(), the caller must have previously
done ResourceArrayEnlarge(). I tried to call ResourceOwnerEnlargeBuffers() before
PinBuffer_Locked(), so it can avoid this crash.

		if ((buf_state & BM_DIRTY) == BM_DIRTY)
		{
+			/* make sure we can handle the pin */
+			ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
			/*
			 * Try once to flush the dirty buffer.
			 */
			PinBuffer_Locked(bufHdr);

--
Regrads,
Japin Li.

On Tue, Jul 4, 2023 at 5:45 PM Japin Li <japinli@hotmail.com> wrote:

On Tue, 04 Jul 2023 at 17:00, jian he <jian.universality@gmail.com> wrote:

the following will also crash. no idea why.
begin;
select count(*) from onek;
select relpages from pg_class where relname = 'onek'; --queryA

SELECT count(*) FROM pg_buffercache WHERE relfilenode =
pg_relation_filenode('onek'::regclass); --queryB

insert into onek values(default);

select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('onek'::regclass);

---------------------------------
queryA returns 35, queryB returns 37.
----------------------------------
crash info:
test_dev=*# insert into onek values(default);
INSERT 0 1
test_dev=*# select count(pg_buffercache_invalidate(bufferid)) from
pg_buffercache where relfilenode =
pg_relation_filenode('onek'::regclass);
TRAP: failed Assert("resarr->nitems < resarr->maxitems"), File:

"../../Desktop/pg_sources/main/postgres/src/backend/utils/resowner/resowner.c",

Line: 275, PID: 1533312

According to the comments of ResourceArrayAdd(), the caller must have
previously
done ResourceArrayEnlarge(). I tried to call ResourceOwnerEnlargeBuffers()
before
PinBuffer_Locked(), so it can avoid this crash.

if ((buf_state & BM_DIRTY) == BM_DIRTY)
{
+                       /* make sure we can handle the pin */
+                       ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
/*
* Try once to flush the dirty buffer.
*/
PinBuffer_Locked(bufHdr);

--
Regrads,
Japin Li.

thanks. tested flush pg_catalog, public schema, now, both works as pitched.

On Sat, Jul 1, 2023 at 6:09 AM Thomas Munro <thomas.munro@gmail.com> wrote:

It should be restricted by role, but I wonder which role it should be.
Testing for superuser is now out of fashion.

as pg_buffercache/pg_buffercache--1.2--1.3.sql. You need pg_maintain
privilege to use pg_buffercache.
The following query works on a single user. Obviously you need a role who
can gain pg_monitor privilege.

begin;
create role test login nosuperuser;
grant select, insert on onek to test;
grant pg_monitor to test;
set role test;
select count(*) from onek;
insert into onek values(default);
(SELECT count(*) FROM pg_buffercache WHERE relfilenode =
pg_relation_filenode('onek'::regclass))
except
(
select count(pg_buffercache_invalidate(bufferid))
from pg_buffercache where relfilenode =
pg_relation_filenode('onek'::regclass)
);

rollback;

+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
<chaturvedipalak1911@gmail.com> wrote:

Hey Nitin,

Will
there be a scenario where the buffer is dirty and its reference count
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.

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.
Yes that can be implemented with a default value of force. Will
implement it in the next patch.

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

On Tue, 11 Jul 2023 at 18:08, Palak Chaturvedi
<chaturvedipalak1911@gmail.com> wrote:

Hi,

I wanted this feature a couple times before...

On 2023-07-03 13:56:29 +0530, Palak Chaturvedi wrote:

+PG_FUNCTION_INFO_V1(pg_buffercache_invalidate);
+Datum
+pg_buffercache_invalidate(PG_FUNCTION_ARGS)

I don't think "invalidating" is the right terminology. Note that we already
have InvalidateBuffer() - but it's something we can't allow users to do, as it
throws away dirty buffer contents (it's used for things like dropping a
table).

How about using "discarding" for this functionality?

Using the buffer ID as the identifier doesn't seem great, because what that
buffer is used for, could have changed since the buffer ID has been acquired
(via the pg_buffercache view presumably)?

My suspicion is that the usual usecase for this would be to drop all buffers
that can be dropped?

+	if (bufnum < 0 || bufnum > NBuffers)
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("buffernum is not valid")));
+
+	}
+
+	result = TryInvalidateBuffer(bufnum);
+	PG_RETURN_BOOL(result);
+}

I think this should be restricted to superuser by default (by revoking
permissions from PUBLIC). We allow normal users to use pg_prewarm(...), true -
but we perform an ACL check on the relation, so it can only be used for
relations you have access too. This function could be used to affect
performance of other users quite substantially.

+/*
+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)
+{
+	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)
+		{
+			/*
+			 * Try once to flush the dirty buffer.
+			 */
+			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);

I'm wary of using InvalidateBuffer() here, it's typically used for different
purposes, including throwing valid contents away. That seems a bit scary.

I think you should be able to just use InvalidateVictimBuffer()?

Greetings,

Andres Freund

On Wed, Jul 19, 2023 at 12:45 PM Andres Freund <andres@anarazel.de> wrote:

I don't think "invalidating" is the right terminology. Note that we already
have InvalidateBuffer() - but it's something we can't allow users to do, as it
throws away dirty buffer contents (it's used for things like dropping a
table).

How about using "discarding" for this functionality?

+1

Using the buffer ID as the identifier doesn't seem great, because what that
buffer is used for, could have changed since the buffer ID has been acquired
(via the pg_buffercache view presumably)?

My suspicion is that the usual usecase for this would be to drop all buffers
that can be dropped?

Well the idea was to be able to drop less than everything. Instead of
having to bike-shed what the user interface should look like to
specify what subset of everything you want to drop, you can just write
SQL queries (mostly likely involving the pg_buffercache view, indeed).
It's true that buffer IDs can change underneath your feet between
SELECT and discard, but the whole concept is inherently racy like
that. Suppose we instead had pg_unwarm('my_table') or whatever
instead. Immediately after it runs and before it even returns, some
blocks of my_table can finish up coming back into the pool. It's also
interesting to be able to kick individual pages out when testing code
that caches buffers IDs for ReadRecentBuffer(), and other buffer-pool
work. Hence desire to not try to be clever at all here, and just come
up with the absolute bare minimum thing that can kick buffers out by
ID and leave the rest up to hackers/experts who are willing and able
to write queries to supply them. You can still drop everything that
can be dropped -- generate_series. Or whatever you want.

Hello

I had a look at the patch and tested it on CI bot, it compiles and tests fine both autoconf and meson. I noticed that the v2 patch contains the v1 patch file as well. Not sure if intended but put there my accident.

I don't think "invalidating" is the right terminology. Note that we already
have InvalidateBuffer() - but it's something we can't allow users to do, as it
throws away dirty buffer contents (it's used for things like dropping a
table).

How about using "discarding" for this functionality?

I think "invalidating" is the right terminology here, it is exactly what the feature is doing, it tries to invalidate a buffer ID by calling InvalidateBuffer() routine inside buffer manager and calls FlushBuffer() before invalidating if marked dirty.

The problem here is that InvalidateBuffer() could be dangerous because it allows a user to invalidate buffer that may have data in other tables not owned by the current user,

I think it all comes down to the purpose of this feature. Based on the description in this email thread, I feel like this feature should be categorized as a developer-only feature, to be used by PG developer to experiment and observe some development works by invalidating one more more specific buffers..... If this is the case, it may be helpful to add a "DEVELOPER_OPTIONS" in GUC, which allows or disallows the TryInvalidateBuffer() to run or to return error if user does not have this developer option enabled.

If the purpose of this feature is for general users, then it would make sense to have something like pg_unwarm (exactly opposite of pg_prewarm) that takes table name (instead of buffer ID) and drop all buffers associated with that table name. There will be permission checks as well so a user cannot pg_unwarm a table owned by someone else. User in this case won't be able to invalidate a particular buffer, but he/she should not have to as a regular user anyway.

thanks!

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

Hii,
Thanks for your feedback. We have decided to add a role for the
extension to solve that problem.
And concerning to pg_unwarm table I think we can create a new function
to do that but I think a general user would not require to clear a
table from buffercache.
We can use bufferid and where statements to do the same if a
superuser/(specific role) requests it.

Thanks.

Le 01/07/2023 à 00:09, Thomas Munro a écrit :

On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi
<chaturvedipalak1911@gmail.com> wrote:

We also talked a bit about how one might control the kernel page cache
in more fine-grained ways for testing purposes, but it seems like the
pgfincore project has that covered with its pgfadvise_willneed() and
pgfadvise_dontneed(). IMHO that project could use more page-oriented
operations (instead of just counts and coarse grains operations) but
that's something that could be material for patches to send to the
extension maintainers. This work, in contrast, is more tangled up
with bufmgr.c internals, so it feels like this feature belongs in a
core contrib module.

Precisely what pgfincore is doing/offering already.
Happy to propose to postgresql tree if there are interest. Next step for
pgfincore is to add cachestat() syscall and evaluates benefits for
PostgreSQL cost estimators of this new call.

Here an example to achieve the warm/unwarm, each bit is a PostgreSQL
page, so here we warm cache with the first 3 and remove the last 3 from
cache (system cache, not shared buffers).

-- Loading and Unloading
cedric=# select * from pgfadvise_loader('pgbench_accounts', 0, true,
true, B'111000');
relpath | os_page_size | os_pages_free | pages_loaded |
pages_unloaded
------------------+--------------+---------------+--------------+----------------
base/11874/16447 | 4096 | 408376 | 3 |
3

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

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

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">On 1/3/24 10:25 AM, Cédric Villemain
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:8b8c709a-c7cc-4965-8296-64f549c27501@abcsql.com">Hi
      Palak,
      <br>
      <br>
      I did a quick review of the patch:
      <br>
      <br>
      +CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default
      true)
      <br>
      +RETURNS bool
      <br>
      +AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate'
      <br>
      +LANGUAGE C PARALLEL SAFE;
      <br>
      <br>
      --&gt; Not enforced anywhere, but you can also add a comment to
      the function, for end users...
      <br>
    </blockquote>
    <p>The arguments should also have names...</p>
    <blockquote type="cite"
      cite="mid:8b8c709a-c7cc-4965-8296-64f549c27501@abcsql.com"><br>
      +    force = PG_GETARG_BOOL(1);
      <br>
      <br>
      I think you also need to test PG_ARGISNULL with force parameter.
      <br>
    </blockquote>
    Actually, that's true for the first argument as well. Or, just mark
    the function as STRICT.<br>
    <pre class="moz-signature" cols="72">-- 
Jim Nasby, Data Architect, Austin TX</pre>
  </body>
</html>