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

Hi,

On 2025-04-08 03:15:57 +0300, Nazir Bilal Yavuz wrote:
> On Tue, 8 Apr 2025 at 02:29, Andres Freund <andres(at)anarazel(dot)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