Re: Add pg_buffercache_evict_all() and pg_buffercache_mark_dirty[_all]() functions

Hi,

On Fri, 28 Mar 2025 at 01:53, Aidar Imamov <a(dot)imamov(at)postgrespro(dot)ru> wrote:
>
> Hi!
>
> I've been looking at the patches v3 and there are a few things I want to
> talk
> about.

Thank you for looking into this!

> 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."

This is correct if the buffer header lock is acquired before calling
the EvictUnpinnedBuffer(). Otherwise, the buffer might be flushed
after calling the EvictUnpinnedBuffer() and before acquiring the
buffer header lock. I slightly edited this comment and also added a
comment for the buf_state variable:

* buf_state is used to understand if the buffer header lock is acquired
* before calling this function. If it has non-zero value, it is assumed that
* buffer header lock is acquired before calling this function. This is
* helpful for evicting buffers in the relation as the buffer header lock
* needs to be taken before calling this function in this case.
*
* *flushed is set to true if the buffer was dirty and has been flushed.
* However, this does not necessarily mean that we flushed the buffer, it
* could have been flushed by someone else.

> I also think it'd be a good idea to add null-checks for "flushed" before
> dereferencing it:
> > *flushed = false;
> > *flushed = true;

Assert check is added.

> 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);
> > }

I think this is better, done.

> EvictUnpinnedBuffersFromSharedRelation():
> Maybe it will be more accurate to name the function as
> EvictRelUnpinnedBuffers()?

I liked that, done.

> 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 added all comments except the not null part, I also preserved the
explanation of why we need this function.

> 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;

Assert check is added.

> 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)++;
> > }

I think we need it. Copy-pasting a comment from the DropRelationBuffers():

* We can make this a tad faster by prechecking the buffer tag before
* we attempt to lock the buffer; this saves a lot of lock
* acquisitions in typical cases. It should be safe because the
* caller must have AccessExclusiveLock on the relation, or some other
* reason to be certain that no one is loading new pages of the rel
* into the buffer pool. (Otherwise we might well miss such pages
* entirely.) Therefore, while the tag might be changing while we
* look at it, it can't be changing *to* a value we care about, only
* *away* from such a value. So false negatives are impossible, and
* false positives are safe because we'll recheck after getting the
* buffer lock.

> 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."

Done.

> And also I think the function should return true at the case when the
> buffer was
> already dirty. What do you think?

I asked the same question in the first email and I still could not
decide but I will be applying this as it was not decided before.

> pg_buffercache_evict_relation():
> "pg_buffercache_evict_relation function is intended to" - 'be' is missed
> here.

Done.

> pg_buffercache_mark_dirty():
> Maybe edit the comment to: "Try to mark a shared buffer as dirty."?

Done.

> Maybe edit the elog text to: "bad shared buffer ID" - just to clarify
> the case when
> provided buffer number is negative (local buffer number).

Although I think your suggestion makes sense, I did not change this
since it is already implemented like that in the
pg_buffercache_evict().

> pg_buffercache_mark_dirty_all():
> Maybe also edit the comment to: "Try to mark all the shared buffers as
> dirty."?

Done.

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

I thought that pgindent already does this, done.

v4 is attached.

--
Regards,
Nazir Bilal Yavuz
Microsoft