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
