Re: Allow a condition string in an injection point

From: Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com>
To: Sami Imseih <samimseih(at)gmail(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, 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-07-24 12:58:36
Message-ID: amNhfOZYqlY94IEF@bdtpg
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2026-07-24 12:58:40 Re: Better shared data structure management and resizable shared data structures
Previous Message Ashutosh Bapat 2026-07-24 12:56:12 Re: Changing shared_buffers without restart