| 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 |
| 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? |