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

Hi,

On Mon, 31 Mar 2025 at 16:37, Aidar Imamov <a(dot)imamov(at)postgrespro(dot)ru> wrote:
>
> 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.

Thank you so much for the reviews, these were very helpful!

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

I agree. I will mark this as 'ready for committer' after writing the tests.

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

You are right, both are done.

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

Done.

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

This is better, done.

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

I do not have a strong opinion on this. I agree that these functions
can be used somewhere else later but it is easier to ignore these on
the return instead of handling NULLs in the functions. If we want to
consider the NULL possibility in the EvictUnpinnedBuffer() &
EvictRelUnpinnedBuffers(), then we need to write additional if
clauses. I think this increases the complexity unnecessarily.

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

You are right, I will try to write the tests but I am sharing the new
version without tests to speed things up.

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

You may be right. I thought they are parallel safe as we acquire the
buffer header lock for each buffer but now, I have the same doubts as
you. I want to hear other people's opinions on that before labeling
them as UNSAFE.

v5 is attached.

--
Regards,
Nazir Bilal Yavuz
Microsoft