Re: Allow a condition string in an injection point

From: Sami Imseih <samimseih(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com>, Daniel Gustafsson <daniel(at)yesql(dot)se>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Daniil Davydov <3danissimo(at)gmail(dot)com>
Subject: Re: Allow a condition string in an injection point
Date: 2026-08-13 21:53:29
Message-ID: CAA5RZ0t4DQv5FDPC02B987oF4SBG4sJgdBgB-DMVDEsZR2_Bzg@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 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

Attachment Content-Type Size
v3-0001-Allow-a-condition-string-in-an-injection-point.patch application/octet-stream 10.1 KB
v3-0002-Add-new-string-based-wait-in-051_effective_wal_le.patch application/octet-stream 2.2 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Corey Huinker 2026-08-13 22:36:55 Re: Credits For v19
Previous Message Nathan Bossart 2026-08-13 21:40:17 Re: Why is_admin_of_role() use ROLERECURSE_MEMBERS rather than ROLERECURSE_PRIVS?