Allow a condition string in an injection point

On Thu, Apr 09, 2026 at 01:02:37PM -0500, Sami Imseih wrote:

A follow-up to the discussion here [0], here is a patch that allows
for an arbitrary string in injection points to be able to apply more
granular filters for running an injection point. This will be useful
for autovacuum testing as discussed in the referenced thread,
and perhaps in some other places.

Are the patches under discussion required for v19 or is that something
that can wait before v20 opens for business? We have always required
a use-case in core before adding a new API in this module, to justify
its existence.

The string is capped at 256 bytes which seems like a reasonable
value. I considered using a flexible_array_member and to track
the length, but that hardly seemed worth it at this stage.

This change is local to the module injection_points. That can be
changed at will even in the back-branches.

A case I envision, and there could be more is only run
the injection point for a specific rel.

SELECT injection_points_attach('my-inj-pt', 'wait', 'tab1');

We may be able to simplify a couple more places that rely on multiple
INJECTION_POINT currently, with slightly different names. I doubt
these currently in the tree are worth changing. That would make
backpatches more invasive for the tests, which is more noise, so
most likely no.

#ifdef USE_INJECTION_POINTS
INJECTION_POINT("my-inj-pt", RelationGetRelationName(rel));
#endif

Ahah, nice. You'd probably want to schema-qualify or database-quality
that anyway.

Worth noting, the condition types were changed to bit flags since
we may need to combine conditions such as local injection point
and string.

It's definitely more useful to allow combinations of them in an AND
fashion (if two conditions are defined then check both, and allow the
point to run if both conditions pass). Just to say that what you are
doing looks sensible for me. And I find that pretty cool for its
simplicity and what it could provide for future tests.

@@ -322,6 +322,7 @@ InjectionPointAttach(const char *name,
 	strlcpy(entry->name, name, sizeof(entry->name));
 	strlcpy(entry->library, library, sizeof(entry->library));
 	strlcpy(entry->function, function, sizeof(entry->function));
+	memset(entry->private_data, 0, INJ_PRIVATE_MAXLEN);
 	if (private_data != NULL)
 		memcpy(entry->private_data, private_data, private_data_size);

Hmm, this could be qualified as a bug, and surely it's a good practice
to clean things on attach. I'll go backpatch that.
--
Michael

A follow-up to the discussion here [0], here is a patch that allows
for an arbitrary string in injection points to be able to apply more
granular filters for running an injection point. This will be useful
for autovacuum testing as discussed in the referenced thread,
and perhaps in some other places.

Are the patches under discussion required for v19 or is that something
that can wait before v20 opens for business? We have always required
a use-case in core before adding a new API in this module, to justify
its existence.

This is v20. One of the use-case is discussed here [1][/messages/by-id/CAA5RZ0tExiffcu7qvrUbpq_qqz=zCD2aJ5_Qigo6eP2kgTx3eQ@mail.gmail.com%5D. When testing of
autovacuum for a specific table, we need a way to run the injection point
for that table only, else we end up running the point it for all tables. This
is especially true for check-world where other non-related tables are
being autovacuumed. So this gives more granular control.

Worth noting, the condition types were changed to bit flags since
we may need to combine conditions such as local injection point
and string.

It's definitely more useful to allow combinations of them in an AND
fashion (if two conditions are defined then check both, and allow the
point to run if both conditions pass). Just to say that what you are
doing looks sensible for me. And I find that pretty cool for its
simplicity and what it could provide for future tests.

yes, I did test with some other new hypothetical condition in the future and
it was simple to plug it in to the code at that point.

@@ -322,6 +322,7 @@ InjectionPointAttach(const char *name,
strlcpy(entry->name, name, sizeof(entry->name));
strlcpy(entry->library, library, sizeof(entry->library));
strlcpy(entry->function, function, sizeof(entry->function));
+       memset(entry->private_data, 0, INJ_PRIVATE_MAXLEN);
if (private_data != NULL)
memcpy(entry->private_data, private_data, private_data_size);

Hmm, this could be qualified as a bug, and surely it's a good practice
to clean things on attach. I'll go backpatch that.

It did not matter before this new condition type since the private_data
was never NULL. Not the case with this patch, as it caused inection_points test
regressions with this change. So, yeah, it's a latent bug.

--
Sami

[1]: [/messages/by-id/CAA5RZ0tExiffcu7qvrUbpq_qqz=zCD2aJ5_Qigo6eP2kgTx3eQ@mail.gmail.com%5D

Hi,

On Thu, Apr 09, 2026 at 06:05:02PM -0500, Sami Imseih wrote:

A follow-up to the discussion here [0], here is a patch that allows
for an arbitrary string in injection points to be able to apply more
granular filters for running an injection point. This will be useful
for autovacuum testing as discussed in the referenced thread,
and perhaps in some other places.

Are the patches under discussion required for v19 or is that something
that can wait before v20 opens for business? We have always required
a use-case in core before adding a new API in this module, to justify
its existence.

This is v20. One of the use-case is discussed here [1]. When testing of
autovacuum for a specific table, we need a way to run the injection point
for that table only, else we end up running the point it for all tables. This
is especially true for check-world where other non-related tables are
being autovacuumed. So this gives more granular control.

+1 for the idea and the use case mentioned above makes sense to me.

A few comments:

=== 1

-LANGUAGE C STRICT PARALLEL UNSAFE;
+LANGUAGE C PARALLEL UNSAFE;

and then

{
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
char *action = text_to_cstring(PG_GETARG_TEXT_PP(1));
+ char *str = PG_ARGISNULL(2) ? NULL : text_to_cstring(PG_GETARG_TEXT_PP(2));

As STRICT has been removed, I think that makes sense to also check PG_ARGISNULL
on 0 and 1 otherwise PG_GETARG_TEXT_PP dereferences a NULL.

=== 2

-       if (!injection_point_allowed(condition))
+       if (!injection_point_allowed(condition, arg))

I think arg should be cast to char * (like injection_error() and injection_notice()
do).

=== 3

+               if (strlen(str) >= INJ_DATA_MAXLEN)
+                       ereport(ERROR,
+                                       (errmsg("injection point condition string too long"),
+                                        errdetail("injection point condition string must be less than %d characters.", INJ_DATA_MAXLEN)
));
+

Maybe this should be consistent with existing ones like:

elog(ERROR, "injection point name %s too long (maximum of %u characters)", name, INJ_NAME_MAXLEN - 1);

=== 4

+       /* did not match the condition string */
+       if ((condition->type & INJ_CONDITION_STRING) &&
+               (condition->str[0] == '\0' ||
+                arg == NULL || strcmp(condition->str, arg) != 0))
+               return false;

I think we should reject empty condition strings at attach time: when the string
is empty, condition->str[0] == '\0' is true, so the injection point can never
fire regardless of arg.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, Jul 24, 2026 at 12:58:36PM +0000, Bertrand Drouvot wrote:

I think we should reject empty condition strings at attach time: when the string
is empty, condition->str[0] == '\0' is true, so the injection point can never
fire regardless of arg.

While looking at this patch, I'd like to have at least one valid test
using it. Could you propose one within the scope of this thread? An
autovacuum case has been proposed, at least, but perhaps we have
something simpler proposed recently where this facility could prove
useful?
--
Michael

On 3 Aug 2026, at 13:16, Michael Paquier <michael@paquier.xyz> wrote:

FWIW, I like the idea proposed in this patch.

While looking at this patch, I'd like to have at least one valid test
using it.

+1, a test using the new facility seems like a requirement for this. At some
point we should probably consider documenting this module with a README, but
that's for another patch.

--
Daniel Gustafsson

Hi,

Thanks all for the reviews/comments.

v2- attached addresses Bertrand's points. Fixed all.
I also included some additional tests for injection_points_attach()
since this function is no longer STRICT.

On Mon, Aug 3, 2026 at 9:09 AM Daniel Gustafsson <daniel@yesql.se> wrote:

On 3 Aug 2026, at 13:16, Michael Paquier <michael@paquier.xyz> wrote:

FWIW, I like the idea proposed in this patch.

While looking at this patch, I'd like to have at least one valid test
using it.

+1, a test using the new facility seems like a requirement for this. At some
point we should probably consider documenting this module with a README, but
that's for another patch.

--
Daniel Gustafsson

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]/messages/by-id/adWcVrX3jrHPoCmD@paquier.xyz 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.

Looking at [1]/messages/by-id/aFrUkYVyATVk2nsD@nathan, if we want to test toast reloptions for autovacuum, we
will need some type
of way to ensure that we are inspecting the correct relations for the
test. Since
autovacuum cannot rely on injection_point_set_local because workers will not
set the local injection point, we can use string conditions to ensure
we reached the
correct table. the nocfbot test attached shows how we can build such tests for
autovacuum. The test fails without the latest patchset in [1]/messages/by-id/aFrUkYVyATVk2nsD@nathan. CCing Nathan as
his patch is being discussed.

Without this condition string, to implement such a test, we will need
a NOTICE to log
and we will need to use regexp to examine the log to make sure our
toast table fired
at the correct injection point. With the condition string, we can
simply just use
wait_for_event since we know which table the injection point will fire on.

This feature could enable further autovacuum tests as well.

[0]: /messages/by-id/adWcVrX3jrHPoCmD@paquier.xyz
[1]: /messages/by-id/aFrUkYVyATVk2nsD@nathan

--
Sami Imseih
Amazon Web Services (AWS)

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.

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.

 CREATE FUNCTION injection_points_attach(IN point_name TEXT,
[ ... ]
-LANGUAGE C STRICT PARALLEL UNSAFE;
+LANGUAGE C PARALLEL UNSAFE;

Okay with the strictness here.

 	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.

Looking at [1], if we want to test toast reloptions for autovacuum, we
will need some type
of way to ensure that we are inspecting the correct relations for the
test. Since
autovacuum cannot rely on injection_point_set_local because workers will not
set the local injection point, we can use string conditions to ensure
we reached the
correct table. the nocfbot test attached shows how we can build such tests for
autovacuum. The test fails without the latest patchset in [1]. CCing Nathan as
his patch is being discussed.

+# A TOAST relation inherits vacuum reloptions from its main table when it has
+# no toast.* value of its own.  Condition strings key each wait to the TOAST
+# relation, so its resolution is observed on its own.

Better perhaps to wait for Nathan's input on how this would help.
That depends on the resulting commit merged into the tree. But I
don't think that we should, see below for extra ideas.

Without this condition string, to implement such a test, we will need
a NOTICE to log
and we will need to use regexp to examine the log to make sure our
toast table fired
at the correct injection point. With the condition string, we can
simply just use
wait_for_event since we know which table the injection point will fire on.

So your argument is that we lack context data regarding these GUCs in
a non-backend context. Why not.

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.
--
Michael

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

On Thu, Aug 13, 2026 at 04:53:29PM -0500, Sami Imseih wrote:

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.

Okay, that works here. My previous point feels a bit moot due to
TestInjectionNoticeFunc, but that would be my fault.

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.

If others would like to be fancier, that could always happen later.
It does not change the fact that this stuff has merit in the long run.

Edited a couple of things, like comments, some ereport() vs elog().
And then applied both things.
--
Michael