Hi, On Wed, 19 Mar 2025 at 01:02, Aidar Imamov <a.ima...@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