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

Hi,

On 2025-04-08 03:15:57 +0300, Nazir Bilal Yavuz wrote:
> On Tue, 8 Apr 2025 at 02:29, Andres Freund <andres@anarazel.de> wrote:
> > On 2025-04-07 19:37:50 +0300, Nazir Bilal Yavuz wrote:
> > > > > > > +     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 don't think we're particularly consistent about it. And I think there's some
> > differing views about what precisely the right behaviour is...
> >
> >
> > I've tried to polish the patch.  Changes I made:
> >
> > - The number of processed buffers for EvictAllUnpinnedBuffers() was alwasy
> >   NBuffers, that didn't seem right. But that's obsoleted by the next point:
> >
> > - I think it'd be more useful to return the number of skipped buffers,
> >   i.e. buffers that could not be evicted, than the number of processed
> >   buffers.
> 
> I agree. The number of buffers that we tried to evict but couldn't
> evict will give more information rather than NBuffers.

Cool.

> >
> >   I'm not_evicted or such would also work.
> 
> Did you mean that 'not_evicted' can be used instead of
> 'skipped_buffers' as a column name? If that is the case, both are okay
> to me.

Correct, sorry for that garbled sentence.


> > - I don't love the buffers_ prefix for the column names / C function
> >   arguments. Seems long.  It seems particularly weird because
> >   pg_buffercache_evict() doesn't have a buffer_ prefix.
> >
> >   I left it as-is, but I think something perhaps ought to change before
> >   commit.
> 
> Both are okay to me, I still couldn't decide which I like more.

I went back and forth at least three times.  In the end I added the buffer_
(singular) prefix for pg_buffercache_evict().


> > - The documentation format of the functions isn't quite what we usually do (a
> >   table documenting the columns returned by a function with multiple columns),
> >   but otoh, these are really developer oriented functions, so spending 30
> >   lines of a <table> on each of these functions feels a bit silly.
> >
> >   I'd be ok with it as-is.
> >
> > - The docs for pg_buffercache_evict() don't quite sound right to me, there's
> >   some oddity in the phrasing.  Nothing too bad, but perhaps worht a small bit
> >   of additional polish.
> 
> I tried to update it without changing the old doc, perhaps that was
> the wrong move.

I think it's ok for now.  It might be worth doing a larger redesign of the
pgbuffercache docs at some point...


Pushed.


Thanks for your patches and thanks for all the reviewers getting this ready!


Greetings,

Andres Freund