MIME-Version: 1.0
Date: Wed, 19 Mar 2025 01:02:52 +0300
From: Aidar Imamov <a.imamov@postgrespro.ru>
To: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, PostgreSQL Hackers
 <pgsql-hackers@lists.postgresql.org>, koshy44@gmail.com
Subject: Re: Add pg_buffercache_evict_all() and
 pg_buffercache_mark_dirty[_all]() functions
In-Reply-To: 
 <CAN55FZ2mNTVK81jqsy2NXg9k_ex_LrdU6JmL1wvsW9-GPy9XuA@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>
Message-ID: <568288111d505a81ebb2a89cea9eacb2@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/568288111d505a81ebb2a89cea9eacb2%40postgrespro.ru>
Precedence: bulk

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