Hi!
I've been looking at the patches v3 and there are a few things I want to
talk
about.
EvictUnpinnedBuffer():
I think we should change the paragraph with "flushed" of the comment to
something more like this: "If the buffer was dirty at the time of the
call it
will be flushed by calling FlushBuffer() and if 'flushed' is not NULL
'*flushed'
will be set to true."
I also think it'd be a good idea to add null-checks for "flushed" before
dereferencing it:
*flushed = false;
*flushed = true;
If the (!buf_state) clause is entered then we assume that the header is
not locked.
Maybe edit the comment: "Lock the header if it is not already locked."
-> "Lock the header"?
if (!buf_state)
{
/* Make sure we can pin the buffer. */
ResourceOwnerEnlarge(CurrentResourceOwner);
ReservePrivateRefCountEntry();
/* Lock the header if it is not already locked. */
buf_state = LockBufHdr(desc);
}
EvictUnpinnedBuffersFromSharedRelation():
Maybe it will be more accurate to name the function as
EvictRelUnpinnedBuffers()?
I think the comment will seem more correct in the following manner:
"Try to evict all the shared buffers containing provided relation's
pages.
This function is intended for testing/development use only!
Before calling this function, it is important to acquire
AccessExclusiveLock on
the specified relation to avoid replacing the current block of this
relation with
another one during execution.
If not null, buffers_evicted and buffers_flushed are set to the total
number of
buffers evicted and flushed respectively."
I also think it'd be a good idea to add null-checks for
"buffers_evicted" and
"buffers_flushed" before dereferencing them:
*buffers_evicted = *buffers_flushed = 0;
I think we don't need to check this clause again if AccessExclusiveLock
was acquired
before function call. Don't we?
if (BufTagMatchesRelFileLocator(&bufHdr->tag, &rel->rd_locator))
{
if (EvictUnpinnedBuffer(buf, buf_state, &flushed))
(*buffers_evicted)++;
if (flushed)
(*buffers_flushed)++;
}
MarkUnpinnedBufferDirty():
I think the comment will seem more correct in the following manner:
"Try to mark provided shared buffer as dirty.
This function is intended for testing/development use only!
Same as EvictUnpinnedBuffer() but with MarkBufferDirty() call inside.
Returns true if the buffer was already dirty or it has successfully been
marked as
dirty."
And also I think the function should return true at the case when the
buffer was
already dirty. What do you think?
pg_buffercache_evict_relation():
"pg_buffercache_evict_relation function is intended to" - 'be' is missed
here.
pg_buffercache_mark_dirty():
Maybe edit the comment to: "Try to mark a shared buffer as dirty."?
Maybe edit the elog text to: "bad shared buffer ID" - just to clarify
the case when
provided buffer number is negative (local buffer number).
pg_buffercache_mark_dirty_all():
Maybe also edit the comment to: "Try to mark all the shared buffers as
dirty."?
bufmgr.h:
I think it might be a good idea to follow the Postgres formatting style
and move the
function's arguments to the next line if they exceed 80 characters.
regards,
Aidar Imamov
