Re: Add pg_buffercache_evict_all() and pg_buffercache_mark_dirty[_all]() functions

Hi,

On Mon, 7 Apr 2025 at 16:38, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2025-04-04 18:36:57 +0300, Nazir Bilal Yavuz wrote:
> > v6 is attached. Additional changes prior to v5:
> >
> > * Tests are added.
>
> I don't think the mark-dirty stuff is quite ready to commit. Given that, could
> you change the split of the patches so that the tests are for the evict
> patches alone? I don't really see a need to add the tests separately from the
> code introducing the relevant functionality.

Done.

> > * EvictRelUnpinnedBuffers() does not call EvictUnpinnedBuffer() now. I
> > basically copied EvictUnpinnedBuffer() to the inside of
> > EvictRelUnpinnedBuffers() with the needed updates.
>
> I think I'd instead introduce a EvictBufferInternal() that is used both by
> EvictUnpinnedBuffer() and EvictRelUnpinnedBuffers(), that is expected to be
> called with the buffer header lock held.

Makes sense, done that way. I named it as 'EvictUnpinnedBufferInternal'.

> > > > + relation_close(rel, AccessExclusiveLock);
> > >
> > > Hm. Why are we dropping the lock here early? It's probably ok, but it's not
> > > clear to me why we should do so.
> >
> > We are dropping the lock after we processed the relation. I didn't
> > understand what could be the problem here. Why do you think it is
> > early?
>
> Most commonly we close relations without releasing the lock, instead relying
> on the lock being released at the end of the transaction.

I see. I was looking at pg_prewarm as an example and copied it from there.

> > > I wonder if these functions ought to also return the number of buffers that
> > > could not be evicted?
> >
> > I didn't add this as I thought this information could be gathered if wanted.
>
> How? A separate query will also return buffers that were since newly read in.

Yes, correct. I missed that.

> I think it'be better to just return all that information, given that we are
> already dropping the function. Dropping objects in extension upgrades can be
> painful for users, due to other objects potentially depending on the dropped
> objects. So we shouldn't do that unnecessarily often.

Done that way. pg_buffercache_evict_relation() and
pg_buffercache_evict_all() return the total number of buffers
processed.

> > From 88624ea0f768ebf1a015cc0c06d2c72c822577a8 Mon Sep 17 00:00:00 2001
> > From: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
> > Date: Fri, 4 Apr 2025 13:22:00 +0300
> > Subject: [PATCH v6 1/4] Return buffer flushed information in
> > pg_buffercache_evict function
> >
> > pg_buffercache_evict() function shows buffer flushed information now.
> > This feature will be used in the following commits.
> >
> > Author: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
> > Suggested-by: Joseph Koshakow <koshy44(at)gmail(dot)com>
> > Reviewed-by: Aidar Imamov <a(dot)imamov(at)postgrespro(dot)ru>
> > Reviewed-by: Andres Freund <andres(at)anarazel(dot)de>
> > Discussion: https://postgr.es/m/CAN55FZ0h_YoSqqutxV6DES1RW8ig6wcA8CR9rJk358YRMxZFmw%40mail.gmail.com
> > ---
> > src/include/storage/bufmgr.h | 2 +-
> > src/backend/storage/buffer/bufmgr.c | 11 +++++++++-
> > src/test/modules/test_aio/test_aio.c | 4 ++--
> > doc/src/sgml/pgbuffercache.sgml | 15 ++++++++-----
> > contrib/pg_buffercache/Makefile | 3 ++-
> > contrib/pg_buffercache/meson.build | 1 +
> > .../pg_buffercache--1.5--1.6.sql | 9 ++++++++
>
> I think I'd squash this with the following changes, as it seems unnecessary to
> increase the version by multiple steps in immediately successive commits.

Done.

> > +/*
> > + * Try to evict all the shared buffers containing provided relation's pages.
> > + *
> > + * This function is intended for testing/development use only!
> > + *
> > + * We need this helper function because of the following reason.
> > + * ReservePrivateRefCountEntry() needs to be called before acquiring the
> > + * buffer header lock but ReservePrivateRefCountEntry() is static and it would
> > + * be better to have it as static. Hence, it can't be called from outside of
> > + * this file. This helper function is created to bypass that problem.
> >
> > + * Also, we should minimize the amount of code outside of storage/buffer that
> > + * needs to know about BuferDescs etc., it is a layering violation to access
> > + * that outside.
> > + *
>
> FWIW, I think this explanation is kind of true, but the real reason is that
> from a layering perspective this simply is bufmgr.c's responsibility, not
> pg_buffercache's. I'd probably just drop these two paragraphs.

Done.

> > + if (!superuser())
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> > + errmsg("must be superuser to use %s()",
> > + "pg_buffercache_evict_relation")));
>
> FWIW, I'd upate the existing superuser check in the same file to use the same
> string. And perhaps I'd just put the check into a small helper function that
> is shared by pg_buffercache_evict() / pg_buffercache_evict() /
> pg_buffercache_evict_all().

Done. I added the pg_buffercache_superuser_check() function.

> > +/*
> > + * Try to evict all shared buffers.
> > + */
> > +Datum
> > +pg_buffercache_evict_all(PG_FUNCTION_ARGS)
> > +{
> > + Datum result;
> > + TupleDesc tupledesc;
> > + HeapTuple tuple;
> > + Datum values[NUM_BUFFERCACHE_EVICT_ALL_ELEM];
> > + bool nulls[NUM_BUFFERCACHE_EVICT_ALL_ELEM] = {0};
> > +
> > + int32 buffers_evicted = 0;
> > + int32 buffers_flushed = 0;
> > + bool flushed;
> > +
> > + if (get_call_result_type(fcinfo, NULL, &tupledesc) != TYPEFUNC_COMPOSITE)
> > + elog(ERROR, "return type must be a row type");
> > +
> > + if (!superuser())
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> > + errmsg("must be superuser to use %s()",
> > + "pg_buffercache_evict_all")));
> > +
> > + for (int buf = 1; buf <= NBuffers; buf++)
> > + {
> > + if (EvictUnpinnedBuffer(buf, &flushed))
> > + buffers_evicted++;
> > + if (flushed)
> > + buffers_flushed++;
> > + }
>
> FWIW, I'd put this loop in bufmgr.c, in a similar helper function as for the
> other cases.

Done. I moved this loop to the EvictAllUnpinnedBuffers() function in bufmgr.c.

> > --- a/contrib/pg_buffercache/sql/pg_buffercache.sql
> > +++ b/contrib/pg_buffercache/sql/pg_buffercache.sql
> > @@ -26,3 +26,54 @@ SET ROLE pg_monitor;
> > SELECT count(*) > 0 FROM pg_buffercache;
> > SELECT buffers_used + buffers_unused > 0 FROM pg_buffercache_summary();
> > SELECT count(*) > 0 FROM pg_buffercache_usage_counts();
> > +RESET role;
> > +
> > +------
> > +---- Test pg_buffercache_evict* and pg_buffercache_mark_dirty* functions
> > +------
> > +
> > +CREATE ROLE regress_buffercache_normal;
> > +SET ROLE regress_buffercache_normal;
> > +
> > +-- These should fail because these are STRICT functions
> > +SELECT * FROM pg_buffercache_evict();
> > +SELECT * FROM pg_buffercache_evict_relation();
> > +SELECT * FROM pg_buffercache_mark_dirty();
>
> > +-- These should fail because they need to be called as SUPERUSER
> > +SELECT * FROM pg_buffercache_evict(1);
> > +SELECT * FROM pg_buffercache_evict_relation(1);
> > +SELECT * FROM pg_buffercache_evict_all();
> > +SELECT * FROM pg_buffercache_mark_dirty(1);
> > +SELECT * FROM pg_buffercache_mark_dirty_all();
> > +
> > +RESET ROLE;
> > +CREATE ROLE regress_buffercache_superuser SUPERUSER;
> > +SET ROLE regress_buffercache_superuser;
>
> Not sure what we gain by creating this role?

I missed that the role used for running the tests is already a superuser. Done.

v7 is attached. I only attached pg_buffercache_evict* patch to run it on cfbot.

--
Regards,
Nazir Bilal Yavuz
Microsoft