Re: Unit tests for SLRU

On Wed, Apr 13, 2022 at 03:51:30PM +0400, Pavel Borisov wrote:
> Only one thing to note. Maybe it would be good not to copy-paste Assert
> after every call of SimpleLruInit, putting it into the wrapper function
> instead. So the test can call calling the inner function (without assert)
> and all other callers using the wrapper. Not sure about naming though.
> Maybe rename current SimpleLruInit -> SimpleLruInitInner and a new wrapper
> being under the old name (SimpleLruInit).

I have looked at what you have here..

This patch redesigns SimpleLruInit() so as the caller would be now in
charge of checking that the SLRU has been created in the context of
the postmaster (aka when initializing shared memory). While this
should work as long as the amount of shared memory area is correctly
sized in _PG_init() and that this area is initialized, then attached
later like for autoprewarm.c (this counts for LWLockRegisterTranche(),
for example), I am not really convinced that this is something that a
patch aimed at extending testing coverage should redesign, especially
with a routine as old as that. If you don't know what you are doing,
it could easily lead to problems with external code. Note that I
don't object to the addition of a new code path or a routine that
would be able to create a SLRU on-the-fly with less restrictions, but
I am not convinced that this we should change this behavior (well,
there is a new argument that would force a recompilation). I am not
sure what could be the use cases in favor of a SLRU created outside
the _PG_init() phase, but perhaps you have more imagination than I do
for such matters ;p

FWIW, I'd like to think that the proper way of doing things for this
test facility is to initialize a SLRU through a loading of _PG_init()
when processing shared_preload_libraries, meaning that you'd better
put this facility in src/test/modules/ with a custom configuration
file with shared_preload_libraries set and a NO_INSTALLCHECK, without
touching at SimpleLruInit().
--
Michael