Re: Weird test mixup

On 15/03/2024 09:39, Michael Paquier wrote:
> On Thu, Mar 14, 2024 at 07:13:53PM -0400, Tom Lane wrote:
>> 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.

For the gin test, a single "SELECT injection_points_attach_local()" at 
the top of the test file would be most convenient.

If I have to do "SELECT 
injection_points_condition('gin-finish-incomplete-split', :'datname');" 
for every injection point in the test, I will surely forget it sometimes.

In the 'gin' test, they could actually be scoped to the same backend.


Wrt. the spinlock and shared memory handling, I think this would be 
simpler if you could pass some payload in the InjectionPointAttach() 
call, which would be passed back to the callback function:

  void
  InjectionPointAttach(const char *name,
                       const char *library,
-                     const char *function)
+                     const char *function,
+                     uint64 payload)

In this case, the payload would be the "slot index" in shared memory.

Or perhaps always allocate, say, 1024 bytes of working area for every 
attached injection point that the test module can use any way it wants. 
Like for storing extra conditions, or for the wakeup counter stuff in 
injection_wait(). A fixed size working area is a little crude, but would 
be very handy in practice.

> By the way, modules/gin/ was missing missing a detach, so the test was
> not repeatable with successive installchecks.

Oops.

It would be nice to automatically detach all the injection points on 
process exit. You wouldn't always want that, but I think most tests hold 
a session open throughout the test, and for those it would be handy.

-- 
Heikki Linnakangas
Neon (https://neon.tech)