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 Makefile.global.in in
the latter case), removing the alternate output.
Please find v2.
--
Michael