Re: Add pg_buffercache_evict_all() and pg_buffercache_mark_dirty[_all]() functions - Mailing list pgsql-hackers

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.


> * 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.


> > > +     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 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.

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.




> From 88624ea0f768ebf1a015cc0c06d2c72c822577a8 Mon Sep 17 00:00:00 2001
> From: Nazir Bilal Yavuz <byavuz81@gmail.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@gmail.com>
> Suggested-by: Joseph Koshakow <koshy44@gmail.com>
> Reviewed-by: Aidar Imamov <a.imamov@postgrespro.ru>
> Reviewed-by: Andres Freund <andres@anarazel.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.



> +/*
> + * 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.


> +    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().

> +/*
> + * 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.



> --- 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?

Greetings,

Andres Freund