Re: Weird test mixup

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Weird test mixup
Date: 2024-03-15 07:39:12
Message-ID: ZfP7IDs9TvrKe49x@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Introduce-runtime-conditions-in-module-injection_poi.patch text/x-diff 9.9 KB
0002-Restrict-GIN-test-with-injection-points-to-run-on-lo.patch text/x-diff 3.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bharath Rupireddy 2024-03-15 07:42:35 Be strict when request to flush past end of WAL in WaitXLogInsertionsToFinish
Previous Message John Naylor 2024-03-15 07:36:10 Re: [PoC] Improve dead tuple storage for lazy vacuum