MIME-Version: 1.0
References: 
 <CAA5RZ0tsGHu2h6YLnVu4HiK05q+gTE_9WVUAqihW2LSscAYS-g@mail.gmail.com>
 <adgqbgfA7KaNwCIS@paquier.xyz>
In-Reply-To: <adgqbgfA7KaNwCIS@paquier.xyz>
From: Sami Imseih <samimseih@gmail.com>
Date: Thu, 9 Apr 2026 18:05:02 -0500
Message-ID: 
 <CAA5RZ0sbaw7=WdztVm71p0jsVnTVvn7SMYaO+hWR474bTiaGeQ@mail.gmail.com>
Subject: Re: Allow a condition string in an injection point
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
	Daniil Davydov <3danissimo@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA5RZ0sbaw7%3DWdztVm71p0jsVnTVvn7SMYaO%2BhWR474bTiaGeQ%40mail.gmail.com>
Precedence: bulk

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

> > 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] [https://www.postgresql.org/message-id/CAA5RZ0tExiffcu7qvrUbpq_qqz%3DzCD2aJ5_Qigo6eP2kgTx3eQ%40mail.gmail.com]