MIME-Version: 1.0
Date: Fri, 28 Mar 2025 01:53:29 +0300
From: Aidar Imamov <a.imamov@postgrespro.ru>
To: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: Joseph Koshakow <koshy44@gmail.com>, Andres Freund <andres@anarazel.de>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Add pg_buffercache_evict_all() and
 pg_buffercache_mark_dirty[_all]() functions
In-Reply-To: 
 <CAN55FZ0eZ_hK4=Y-kS7sNd0tb9vovT5dAsyHRPryAGwjWuz+cg@mail.gmail.com>
References: 
 <CAN55FZ0h_YoSqqutxV6DES1RW8ig6wcA8CR9rJk358YRMxZFmw@mail.gmail.com>
 <bbxepmv3w2iapbxehkypzsut545valywzf62qlizsrzkbrdnfy@dzw5ju3itwgs>
 <CAN55FZ1Pq_80vcuHZK6dSuTfG-5jBoK0Bd2k1YUSpKT+7hOj6A@mail.gmail.com>
 <CAN55FZ2mNTVK81jqsy2NXg9k_ex_LrdU6JmL1wvsW9-GPy9XuA@mail.gmail.com>
 <568288111d505a81ebb2a89cea9eacb2@postgrespro.ru>
 <CAAvxfHeHBL6XxypexiSu0geH3hkHiFX=ApUaeaU+hgQEfjAwxQ@mail.gmail.com>
 <CAN55FZ06WCeFgQPgftiFyycGRnsuHPXumrZgyxQS30GJGT7Tbg@mail.gmail.com>
 <CAN55FZ0eZ_hK4=Y-kS7sNd0tb9vovT5dAsyHRPryAGwjWuz+cg@mail.gmail.com>
Message-ID: <aa6f2841f541dd537087e21cd2d916c6@postgrespro.ru>
Organization: PostgresPro
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/aa6f2841f541dd537087e21cd2d916c6%40postgrespro.ru>
Precedence: bulk

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