> On Mon, Aug 10, 2026 at 05:31:00PM -0500, Sami Imseih wrote:
> > The facilities that this will be useful for are those in which the
> > injection_point_set_local()
> > cannot be used and thus we cannot clearly determine if the injection
> > point is reached
> > at the correct condition. [0] was one example, which led to this
> > thread, but we dealt
> > with this by using wait_for_log to ensure that we reached the correct code
> > path.
>
> 0001 feels overall OK. I just have a few comments.
>
> +-- injection point condition string matching.
> +SELECT injection_points_attach('TestConditionString', 'notice', 'MyString');
> +SELECT injection_points_run('TestConditionString', 'MyString'); -- notice
> +SELECT injection_points_run('TestConditionString', 'WrongString'); -- nothing
> +SELECT injection_points_run('TestConditionString', NULL); -- nothing
> +SELECT injection_points_detach('TestConditionString');
>
> This is a duplicate set of tests with the local case. Let's trim the
> cases a bit a drop this part.
Yeah, you're right, local vs shared injection points tests don't
really make a difference
here. With that said, just because shared points are the more common
case, I kept
those instead.
> SELECT injection_points_run('TestConditionLocal2'); -- nothing
> +SELECT injection_points_run('TestConditionLocalString', 'LocalData'); --
> nothing
> This one also to check that the local point has been dropped is not
> really required to me: we already make sure that a bunch of the other
> points have been detached.
removed.
> INJ_CONDITION_ALWAYS = 0, /* always run */
> - INJ_CONDITION_PID, /* PID restriction */
> + INJ_CONDITION_PID = 1 << 0, /* PID restriction */
> + INJ_CONDITION_STRING = 1 << 1, /* generic string match against arg */
>
> Hmm. Could it be better to rename "ALWAYS" to "NONE" then? That
> would feel less confusing to me after switching to a bitmask as we may
> want to apply multiple conditions.
Right, good catch. Updated along with the comment.
> While I was tweaking with the stats code, I got a little bit annoyed
> by the timing of the flushes. So one idea I could think about is a
> point in a vacuum report, where we could play with a VACUUM command
> and some partitions, checking that some relations trigger or not?
>
> But actually, there is even simpler.. As one example, in 051 for
> Sawada-san's effective WAL level, we have some local points that could
> be made more efficient by targetting only the slot name we want. So
> we could use this new facility to make the tests more surgical in the
> way the points are run. Perhaps that sounds a bit pedantic, but
> something like the attached would be enough for me as a starting
> point. We don't really have to be ambitious in the first step, and we
> could expand that to tighten as well some of the PID-based checks,
> perhaps, for the checkpointer or the startup process. Basic idea is
> attached.
I do like the 051 test you mention and I think we should add a string
to the "sync_slot" as well. This is good for demo.
See attached 0002 for this.
With that said, I think the stronger case for injection points string filters
are tests in which we can't use set_local and we can do wait_for_event
which is more robust that using regexp.
--
Sami
v3-0001-Allow-a-condition-string-in-an-injection-point.patch
Description: Binary data
v3-0002-Add-new-string-based-wait-in-051_effective_wal_le.patch
Description: Binary data
