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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
Cc: Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Adding facility for injection points (or probe points?) for more advanced tests
Date: 2023-11-07 08:01:16
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Nov 06, 2023 at 10:28:14PM +0300, Nazir Bilal Yavuz wrote:
> I liked the idea; thanks for working on this!

Thanks for the review.

> What do you think about creating a function for updating the already
> created injection point's callback or name (mostly callback)? For now,
> you need to drop and recreate the injection point to change the
> callback or the name.

I am not sure if that's worth the addition. TBH, all the code I've
seen that would benefit from these APIs just set up a cluster,
register a few injection points with a module, and then run a set of
tests. They can also remove points. So I'm just aiming for simplest
for the moment.

> Here is my code correctness review:
> diff --git a/meson_options.txt b/meson_options.txt
> +option('injection_points', type: 'boolean', value: true,
> + description: 'Enable injection points')
> +
> It is enabled by default while building with meson.

Indeed, fixed.

> diff --git a/src/backend/utils/misc/injection_point.c
> b/src/backend/utils/misc/injection_point.c
> + LWLockRelease(InjectionPointLock);
> +
> + /* If not found, do nothing? */
> + if (!found)
> + return;
> It would be good to log a warning message here.

I don't think that's a good idea. If a code path defines a
INJECTION_POINT_RUN() we'd get spurious warnings except if a point is
always defined when the build switch is enabled.

> I tried to compile that with -Dwerror=true -Dinjection_points=false
> and got some errors (warnings):

Right, fixed these three.

> The test_injection_points test runs and passes although I set
> -Dinjection_points=false. That could be misleading, IMO the test
> should be skipped if Postgres is not compiled with the injection
> points.

The test suite has been using an alternate output, but perhaps you are
right that this has little value without the switch enabled anyway.
I've made the processing optional when the option is not used for
meson and ./configure (requires a variable in in
the latter case), removing the alternate output.

Please find v2.

Attachment Content-Type Size
v2-0001-Base-facility-for-injection-points.patch text/x-diff 29.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Xiang Gao 2023-11-07 08:05:45 RE: CRC32C Parallel Computation Optimization on ARM
Previous Message Amit Kapila 2023-11-07 07:55:33 Re: [PoC] pg_upgrade: allow to upgrade publisher node