Hi I am working with Aidar to give a review and I am also a beginner reviewer.
> From 813e5ec0da4c65970b4b1ce2ec2918e4652da9ab Mon Sep 17 00:00:00 2001 > From: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> > Date: Fri, 20 Dec 2024 14:06:47 +0300 > Subject: [PATCH v1 1/2] Add pg_buffercache_evict_all() function for testing > */ > bool > -EvictUnpinnedBuffer(Buffer buf) > +EvictUnpinnedBuffer(Buffer buf, bool *flushed) I think you should update the comment above this function to include details on the new `flushed` argument. > diff --git a/doc/src/sgml/pgbuffercache.sgml b/doc/src/sgml/pgbuffercache.sgml > index 802a5112d77..83950ca5cce 100644 > --- a/doc/src/sgml/pgbuffercache.sgml > +++ b/doc/src/sgml/pgbuffercache.sgml > @@ -31,8 +31,10 @@ > 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, the > - <function>pg_buffercache_usage_counts()</function> function and > - the <function>pg_buffercache_evict()</function> function. > + <function>pg_buffercache_usage_counts()</function> function, the > + <function>pg_buffercache_evict()</function>, the > + <function>pg_buffercache_evict_relation()</function>, and the > + <function>pg_buffercache_evict_all()</function> function. All the other functions have indexterm tags above this, should we add indexterms for the new functions? > + <sect2 id="pgbuffercache-pg-buffercache-evict-relation"> > + <title>The <structname>pg_buffercache_evict_relation</structname> Function</title> > + <para> > + The <function>pg_buffercache_evict_relation()</function> function is very similar > + to <function>pg_buffercache_evict()</function> function. The difference is that > + <function>pg_buffercache_evict_relation()</function> takes a relation > + identifier instead of buffer identifier. Then, it tries to evict all > + buffers in that relation. The function is intended for developer testing only. > + </para> > + </sect2> > + > + <sect2 id="pgbuffercache-pg-buffercache-evict-all"> > + <title>The <structname>pg_buffercache_evict_all</structname> Function</title> > + <para> > + The <function>pg_buffercache_evict_all()</function> function is very similar > + to <function>pg_buffercache_evict()</function> function. The difference is, > + the <function>pg_buffercache_evict_all()</function> does not take argument; > + instead it tries to evict all buffers in the buffer pool. The function is > + intended for developer testing only. > + </para> > + </sect2> The other difference is that these new functions have an additional output argument to indicate if the buffer was flushed which we probably want to mention here. Also I think it's more gramatically correct to say "the <fn-name> function" or just "<fn-name>". A couple of times you say "<fn-name> function" or "the <fn-name>". Also I think the third to last sentence should end with "... does not take **an** argument" or "... does not take argument**s**". > +CREATE FUNCTION pg_buffercache_evict_relation( > + IN regclass, > + IN fork text default 'main', > + OUT buffers_evicted int4, > + OUT buffers_flushed int4) > +AS 'MODULE_PATHNAME', 'pg_buffercache_evict_relation' > +LANGUAGE C PARALLEL SAFE VOLATILE; > + > +CREATE FUNCTION pg_buffercache_evict_all( > + OUT buffers_evicted int4, > + OUT buffers_flushed int4) > +AS 'MODULE_PATHNAME', 'pg_buffercache_evict_all' > +LANGUAGE C PARALLEL SAFE VOLATILE; Does it make sense to also update pg_buffercache_evict to also return a bool if the buffer was flushed? Or is that too much of a breaking change? > + /* 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; > + } EvictUnpinnedBuffer first checks if the buffer is locked before attempting to lock it. Do we need a similar check here? /* Lock the header if it is not already locked. */ if (!buf_state) buf_state = LockBufHdr(desc); >> I don't think *flushed is necessarily accurate this way, as it might detect >> that it doesn't need to flush the data (via StartBufferIO() returning false). > > You are right. It seems there is no way to get that information > without editing the FlushBuffer(), right? And I think editing > FlushBuffer() is not worth it. So, I can either remove it or explain > in the docs that #buffers_flushed may not be completely accurate. There's already a lot of caveats on EvictUnpinnedBuffer that it might have unexpected results when run concurrently, so I think it's fine to add one more. Thanks, Joe Koshakow