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


Reply via email to