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.

I have reviewed the patches after they were
edited and I would like to share my thoughts on some points.

pg_buffercache_evict_all():
for (int buf = 1; buf < NBuffers; buf++)
Mb it would be more correct to use <= NBuffers?

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


pg_buffercache_mark_dirty_all():
for (int buf = 1; buf < NBuffers; buf++)
Mb it would be more correct to use <= NBuffers?


pg_buffercache_evict_relation():
errmsg("must be superuser to use pg_buffercache_evict function")
'_relation' postfix got lost here

  /* 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?

for (int buf = 1; buf < NBuffers; buf++)
Mb it would be more correct to use <= NBuffers?

   /*
    * 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).
And it maybe useful also to add BUT_STATE_GET_REFCOUNT == 0 precheck before calling
EvictUnpinnedBuffer()?


regards,
Aidar Imamov


Reply via email to