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
