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

Hi,

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.

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

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

Very nice speedup, thank you.

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

I agree, I think they look better 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.

These look more correct now.

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

Both are okay to me, I still couldn't decide which I like more.

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

I tried to update it without changing the old doc, perhaps that was
the wrong move.

Thank you so much for the review and the updates!

v8 applies cleanly and passes the CI.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft