Re: Adding facility for injection points (or probe points?) for more advanced tests

On Wed, Nov 22, 2023 at 09:23:21PM +0530, Ashutosh Bapat wrote:
> On Tue, Nov 21, 2023 at 6:56 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> I've never seen in recent years a need for a given test to use more
>> than 4~5 points. But I guess that you've seen more than that wanted
>> in a prod environment with a fork of Postgres?
>
> A given case may not require more than 4 -5 points but users may
> create scripts with many frequently required injection points and
> install handlers for them.

Sure, if a callback is generic enough it could be shared across
multiple points.

> The injection_run function is called from the place where the
> injection point is declared but that place does not know what
> injection function is going to be run. So a user can not pass
> arguments to injection declaration.

It is possible to make that predictible but it means that a callback
is most likely to be used by one single point. This makes extensions
in charge of holding the callbacks more complicated, at the benefit of
keeping a minimal footprint in the backend code.

> What injection to run is decided
> by the injection_attach and thus one can pass arguments to it but then
> injection_attach stores the information in the shared memory from
> where it's picked up by injection_run. So even though you don't want
> to store the arguments in the shared memory, you are creating a design
> which takes us towards that direction eventually - otherwise users
> will end up writing many injection functions - one for each possible
> combination of count, sleep, conditional variable etc. But let's see
> whether that happens to be the case in practice. We will need to
> evolve this feature based on usage.

A one-one mapping between callback and point is not always necessary.
If you wish to use a combination of N points with a sleep callback and
different sleep times, one can just register a second shmem area in
the extension holding the callbacks that links the point names with
the sleep time to use.

> shmem hash size won't depend upon the number of functions we write in
> the core. Yes it will add to the core code and may add maintenance
> burden. So I understand your inclination to keep the core minimal.

Yeah, without a clear benefit, my point is just to throw the
responsibility to extension developers for now, which would mean the
addition of tests that depend on test_injection_points/, or just
install this extension optionally in other code path that need it.
Maybe 0004 should be in src/test/recovery/ and do that, actually..
I'll most likely agree with extending all the backend stuff in a more
meaningful way, but I am not sure which method should be enforced.

> If the session which attaches to an injection point is same as the
> session where the injection point is triggered (most of the ERROR and
> NOTICE injections will see this pattern), we don't need shared memory.
> There's a performance penalty to it since injection_run will look up
> shared memory. For WAIT we may or may not need shared memory. But
> let's see what other think and what usage patterns we see.

The first POC of the patch that you can find at the top of this thread
did that, actually, but this is too limited. IMO, linking things to a
central table is just *much* more useful.

I've implemented a v5 that switches the cache to use a seconf hash
table on TopMemoryContext for the cache instead of an array. This
makes the cache handling slightly cleaner, so your suggestion was
right. 0003 and 0004 are the same as previously, passing or failing
under the same conditions. I'm wondering if folks have other comments
about 0001 and 0002? It sounds to me like the consensus is that this
stuff is useful and that there are no string objections, so feel free
to comment.

I don't want to propose 0003 in the tree, just an improved version of
0004 for the test coverage (still need to improve that).
--
Michael