MIME-Version: 1.0
Date: Mon, 31 Mar 2025 16:37:44 +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: 
 <CAN55FZ24CoEijAwkWgEiyMzYQ6PqsWdaKxffN0mQy6DxTNv6Tg@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>
 <aa6f2841f541dd537087e21cd2d916c6@postgrespro.ru>
 <CAN55FZ24CoEijAwkWgEiyMzYQ6PqsWdaKxffN0mQy6DxTNv6Tg@mail.gmail.com>
Message-ID: <c51eab5478e94026a40b51cba39e58dc@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/c51eab5478e94026a40b51cba39e58dc%40postgrespro.ru>
Precedence: bulk

Hi!

I've reviewed the latest version of the patches and found a few things 
worth
discussing. This is probably my final feedback on the patches at this 
point.
Maybe Joseph has something to add.

After this discussion, I think it would be helpful if one of the more 
experienced
hackers could take a look at the overall picture (perhaps we should set 
the
status "Ready for Committer"? To be honest, I'm not sure what step that 
status
should be set at).


pg_buffercache--1.5--1.6.sql:
It seems that there is no need for the OR REPLACE clause after the
pg_buffercache_evict() function has been dropped.

Maybe we should remove the RETURNS clause from function declarations 
that have
OUT parameters?
Doc: "When there are OUT or INOUT parameters, the RETURNS clause can be 
omitted"


pg_buffercache_evict_relation():
The function is marked as STRICT so I think we do not need for redundant 
check:
> if (PG_ARGISNULL(0))
>  ereport(ERROR,
>    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>     errmsg("relation cannot be null")));
Doc: "returns null whenever any of its arguments are null", "the 
function is not
executed when there are null arguments;".


EvictUnpinnedBuffer():
Maybe edit this comment a little (just not to repeat the sentences):
> buf_state is used to check if the buffer header lock has been acquired 
> before
> calling this function. If buf_state has a non-zero value, it means that 
> the buffer
> header has already been locked. This information is useful for evicting 
> specific
> relation's buffers, as the buffers headers need to be locked before 
> this function
> can be called with such a intention.


EvictUnpinnedBuffer() & EvictRelUnpinnedBuffers():
Why did you decide to use the assert to check for NULLs?
I understand that currently, the use of these functions is limited to a 
specific set
of calls through the pg_buffercache interface. However, this may not 
always be the
case. Wouldn't it be better to allow users to choose whether or not they 
want to
receive additional information? For example, you could simply ignore any 
arguments
that are passed as NULL.


Additionally, I believe it would be beneficial to include some 
regression tests to
check at least the following cases: return type, being a superuser, bad 
buffer id,
local buffers case.


Also, there's a little thing about declaring functions as PARALLEL SAFE. 
To be honest,
I don't really have any solid arguments against it. I just have some 
doubts. For
example, how will it work if the plan is split up and we try to work on 
an object in
one part, while the other part of the plan evicts the pages of that 
object or marks
them as dirty... I can't really say for sure about that. And in that 
context, I'd
suggest referring to that awesome statement in the documentation: "If in 
doubt,
functions should be labeled as UNSAFE, which is the default."


regards,
Aidar Imamov