Hi,
On Wed, 19 Mar 2025 at 01:02, Aidar Imamov <a.imamov@postgrespro.ru> wrote:
>
> Hi!
>
> This is my first time trying out as a patch reviewer, and I don't claim
> to be
> an expert. I am very interested in the topic of buffer cache and would
> like to
> learn more about it.
Thank you so much for the review!
> pg_buffercache_evict_all():
> > for (int buf = 1; buf < NBuffers; buf++)
> Mb it would be more correct to use <= NBuffers?
Yes, done.
> I also did not fully understand what A. Freund was referring to.
> However, we
> cannot avoid blocking the buffer header, as we need to ensure that the
> buffer
> is not being pinned by anyone else. Perhaps it would be beneficial to
> call
> LockBufHdr() outside and check if BUT_STATE_GET_REFCOUNT == 0 before
> calling
> EvictUnpinnedBuffer(). This would help to prevent unnecessary calls to
> EvictUnpinnedBuffer() itself, ResourceOwnerEnlarge() and
> ReservePrivateRefCountEntry().
I had an off-list talk with Andres and learned that
ResourceOwnerEnlarge() and ReservePrivateRefCountEntry() need to be
called before acquiring the buffer header lock. I introduced the
EvictUnpinnedBuffersFromSharedRelation() function for that [1].
> pg_buffercache_mark_dirty_all():
> > for (int buf = 1; buf < NBuffers; buf++)
> Mb it would be more correct to use <= NBuffers?
Done.
> pg_buffercache_evict_relation():
> > errmsg("must be superuser to use pg_buffercache_evict function")
> '_relation' postfix got lost here
Done.
> > /* Open relation. */
> > rel = relation_open(relOid, AccessShareLock);
> If I understand correctly, this function is meant to replicate the
> behavior of
> VACUUM FULL, but only for dirty relation pages. In this case, wouldn't
> it be
> necessary to obtain an exclusive access lock?
I think VACUUM FULL does more things but I agree with you, obtaining
an exclusive access lock is the correct way.
> > for (int buf = 1; buf < NBuffers; buf++)
> Mb it would be more correct to use <= NBuffers?
Done.
>
> > /*
> > * No need to call UnlockBufHdr() if BufTagMatchesRelFileLocator()
> > * returns true, EvictUnpinnedBuffer() will take care of it.
> > */
> > buf_state = LockBufHdr(bufHdr);
> > if (BufTagMatchesRelFileLocator(&bufHdr->tag, &rel->rd_locator))
> > {
> > if (EvictUnpinnedBuffer(buf, buf_state, &flushed))
> > buffers_evicted++;
> > if (flushed)
> > buffers_flushed++;
> > }
>
> If you have previously acquired the exclusive access lock to the
> relation,
> you may want to consider replacing the order of the
> BufTagMatchesRelFileLocator()
> and LockBufHdr() calls for improved efficiency (as mentioned in the
> comment on
> the BufTagMatchesRelFileLocator() call in the DropRelationBuffers()
> function in
> bufmgr.c).
I think you are right, we are basically copying FlushRelationBuffers()
to the contrib/pg_buffer_cache. Done.
> And it maybe useful also to add BUT_STATE_GET_REFCOUNT == 0 precheck
> before calling
> EvictUnpinnedBuffer()?
I think we do not need to do this. I see EvictUnpinnedBuffer() as a
central place that checks all conditions. If we add this pre-check
then we should not do the same check in the EvictUnpinnedBuffer(),
creating this logic would add unnecessary complexity to
EvictUnpinnedBuffer().
I addressed these reviews in v3. I will share the patches in my next reply.
--
Regards,
Nazir Bilal Yavuz
Microsoft
