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

Hi,

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'm not_evicted or such would also work.

- EvictAllUnpinnedBuffers() did not check whether the buffer was valid before
  locking the buffer, which made it a fair bit more expensive than
  EvictRelUnpinnedBuffers, which kinda has such a check via the buffer tag
  check.

  That sped up EvictAllUnpinnedBuffers() 3x when using a cluster with mostly
  unused shared buffers.

- The optional pointer arguments made the code look a bit ugly. I made them
  mandatory now.

- I made EvictUnpinnedBufferInternal()'s argument the BufferDesc, rather than
  the Buffer.

- The tests for STRICTness worked, they just errored out because there isn't a
  function of the relevant names without arguments. I called them with NULL.

- The count(*) tests would have succeeded even if the call had "failed" due to
  STRICTness. I used <colname> IS NOT NULL instead.

- rebased over the other pg_buffercache changes


Other points:

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

  Otoh, pg_buffercache_summary() and pg_buffercache_usage_counts() already
  inconsistent in a similar way with each other.


- Arguably these functions ought to check BM_TAG_VALID, not BM_VALID. But that
  only rather rarely happens when there are no pins. Since this is a
  pre-existing pattern, I left it alone.


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


Greetings,

Andres Freund