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