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
