Re: Weird test mixup

On Thu, Mar 14, 2024 at 07:13:53PM -0400, Tom Lane wrote:
> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>> Or we could just disable runningcheck because of the concurrency
>> requirement in this test. The test would still be able to run, just
>> less times.
>
> No, actually we *must* mark all these tests NO_INSTALLCHECK if we
> stick with the current definition of injection points. The point
> of installcheck mode is that the tests are supposed to be safe to
> run in a live installation. Side-effects occurring in other
> databases are completely not OK.

I really don't want to plug any runtime conditions into the backend
APIs, because there can be so much more that can be done there than
only restricting a callback to a database. I can imagine process type
restrictions, process PID restrictions, etc. So this knowledge should
stick into the test module itself, and be expanded there. That's
easier ABI-wise, as well.

> I can see that some tests would want to be able to inject code
> cluster-wide, but I bet that's going to be a small minority.
> I suggest that we invent a notion of "global" vs "local"
> injection points, where a "local" one only fires in the DB it
> was defined in. Then only tests that require a global injection
> point need to be NO_INSTALLCHECK.

Attached is a POC of what could be done. I have extended the module
injection_points so as it is possible to register what I'm calling a
"condition" in the module that can be defined with a new SQL function.

The condition is stored in shared memory with the point name, then at
runtime the conditions are cross-checked in the callbacks. With the
interface of this patch, the condition should be registered *before* a
point is attached, but this stuff could also be written so as
injection_points_attach() takes an optional argument with a database
name. Or this could use a different, new SQL function, say a
injection_points_attach_local() that registers a condition with
MyDatabaseId on top of attaching the point, making the whole happening
while holding once the spinlock of the shmem state for the module.

By the way, modules/gin/ was missing missing a detach, so the test was
not repeatable with successive installchecks. Adding a pg_sleep of a
few seconds after 'gin-leave-leaf-split-incomplete' is registered
enlarges the window, and the patch avoids failures when running
installcheck in parallel for modules/gin/ and something else using
gin, like contrib/btree_gin/:
while make USE_MODULE_DB=1 installcheck; do :; done

0001 is the condition facility for the module, 0002 is a fix for the
GIN test. Thoughts are welcome.
--
Michael