Thread: Add pg_get_injection_points() for information of injection points

Hi all,

One thing that's been lacking in injection points is the possibility
to look at the state of the injection points in shared memory through
some SQL.  This was on my tablets for some time, but I have not taken
the time to do the actual legwork.

The attached patch adds a SRF that returns a set of tuples made of the
name, library and function for all the injection points attached to
the system.  This implementation relies on a new function added in
injection_point.c, called InjectionPointList(), that retrieves a
palloc()'d array of the injection points, hiding from the callers the
internals of what this stuff does with the shmem array lookup.

This is useful for monitoring or in tests, to make sure for example
that nothing is left around at the end of a script.  I have a
different proposal planned for this area of the code, where this
function would be good to have, but I am sending an independent patch
as this stuff is useful on its own.

The patch includes a couple of tests and some documentation.

Thanks,
--
Michael

Dear Michael,

Thanks for creating the patch! Let me confirm two points:

Apart from functions related with injection_points, this can be called even when
postgres has been built with -Dinjection_points=false. This is intentional because
this function does not have any side-effect and just refers the status. Is my
understanding correct?

I'm not sure it is directly related, but ISTM there are no direct ways to check
whether the injection_points is enabled or not. How do you think adding the
function?


Regarding the internal of the patch, it could be crashed when two points are
attached and then first one is detached [1]. I think we should not use "idx" for
the result array - PSA the fix.

[1]:
```
SELECT injection_points_attach('TestInjectionLog', 'notice');
 injection_points_attach
-------------------------

(1 row)

SELECT injection_points_attach('TestInjectionError', 'error');
 injection_points_attach
-------------------------

(1 row)

SELECT injection_points_detach('TestInjectionLog');
 injection_points_detach
-------------------------

(1 row)

SELECT name, library, function FROM pg_get_injection_points()
  ORDER BY name COLLATE "C";
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
connection to server was lost
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, 14 Apr 2025 at 05:36, Michael Paquier <michael@paquier.xyz> wrote:
>
> Hi all,
>
> One thing that's been lacking in injection points is the possibility
> to look at the state of the injection points in shared memory through
> some SQL.  This was on my tablets for some time, but I have not taken
> the time to do the actual legwork.
>
> The attached patch adds a SRF that returns a set of tuples made of the
> name, library and function for all the injection points attached to
> the system.  This implementation relies on a new function added in
> injection_point.c, called InjectionPointList(), that retrieves a
> palloc()'d array of the injection points, hiding from the callers the
> internals of what this stuff does with the shmem array lookup.
>
> This is useful for monitoring or in tests, to make sure for example
> that nothing is left around at the end of a script.  I have a
> different proposal planned for this area of the code, where this
> function would be good to have, but I am sending an independent patch
> as this stuff is useful on its own.
>
> The patch includes a couple of tests and some documentation.
>
> Thanks,
> --
> Michael

Hi! I noticed you do not bump catalog version here, so i was a little
confused with
```
reshke=# SELECT name, library, function FROM pg_get_injection_points();
ERROR:  function pg_get_injection_points() does not exist
```
for a while

Also, should we define pg_get_injection_points in the injection_points
extension maybe?

-- 
Best regards,
Kirill Reshke

On Mon, Apr 14, 2025 at 05:26:14PM +0500, Kirill Reshke wrote:
> Hi! I noticed you do not bump catalog version here, so i was a little
> confused with
> ```
> reshke=# SELECT name, library, function FROM pg_get_injection_points();
> ERROR:  function pg_get_injection_points() does not exist
> ```
> for a while

Catalog version bumps are done by committers when code is committed.
When sending patches for reviews (this one is for July and v19),
including catversion bumps in the patches sent just leads to useless
code conflicts, so this should not be done when submitting something.

> Also, should we define pg_get_injection_points in the injection_points
> extension maybe?

As this is global for all libraries, that's not something I would add
there.
--
Michael

On Mon, 14 Apr 2025 at 17:32, Michael Paquier <michael@paquier.xyz> wrote:
>
> As this is global for all libraries, that's not something I would add
> there.


Same applies for pg_stat_statements, but it is defined in extension.
Also, injection points are tests-only, so maybe no need to have this
function callable in production systems?
One more concern is about pre-defined oids: they are limited. Maybe we
should not consume predefined oid in case when this is avoidable.


-- 
Best regards,
Kirill Reshke

Hi,

> > Also, should we define pg_get_injection_points in the injection_points
> > extension maybe?
>
> As this is global for all libraries, that's not something I would add
> there.

If I didn't miss anything, all SQL functions dealing with injection
points are gathered in injection_points extension so IMO
pg_get_injection_points() belongs there. It would be awkward to have
it in the core considering the fact that injection points presumably
should be disabled in release builds. Users will see a function in \df
that does nothing.

-- 
Best regards,
Aleksander Alekseev

On Mon, Apr 14, 2025 at 04:29:37PM +0300, Aleksander Alekseev wrote:
> If I didn't miss anything, all SQL functions dealing with injection
> points are gathered in injection_points extension so IMO
> pg_get_injection_points() belongs there. It would be awkward to have
> it in the core considering the fact that injection points presumably
> should be disabled in release builds.

There are two more in test_aio, and by design out-of-core extensions
can define their own.

> Users will see a function in \df that does nothing.

Yeah, this one's true if --enable-injection-points is not used.
--
Michael

On Mon, Apr 14, 2025 at 06:15:07AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Apart from functions related with injection_points, this can be called even when
> postgres has been built with -Dinjection_points=false. This is intentional because
> this function does not have any side-effect and just refers the status. Is my
> understanding correct?

Yes.  The function could be changed to return an ERROR if the build
does not support this option.  It's more portable to return NULL if
you have some queries that do joins.  This could be used with
pg_stat_activity and wait events for example, and some points are in
positions strategic enough that they could be used across more than
one library, like the restart point one or the autovacuum startup one.

> I'm not sure it is directly related, but ISTM there are no direct ways to check
> whether the injection_points is enabled or not. How do you think adding the
> function?

Yeah, we could use something like that, not sure if that's worth it.

> Regarding the internal of the patch, it could be crashed when two points are
> attached and then first one is detached [1]. I think we should not use "idx" for
> the result array - PSA the fix.

Oops.  That's a brain fart from my side.  Thanks.
--
Michael

Dear Michael,

> Yes.  The function could be changed to return an ERROR if the build
> does not support this option.  It's more portable to return NULL if
> you have some queries that do joins.  This could be used with
> pg_stat_activity and wait events for example, and some points are in
> positions strategic enough that they could be used across more than
> one library, like the restart point one or the autovacuum startup one.

Thanks for the description. +1.

> > I'm not sure it is directly related, but ISTM there are no direct ways to check
> > whether the injection_points is enabled or not. How do you think adding the
> > function?
>
> Yeah, we could use something like that, not sure if that's worth it.

We can fork new thread when it is required...

> > Regarding the internal of the patch, it could be crashed when two points are
> > attached and then first one is detached [1]. I think we should not use "idx" for
> > the result array - PSA the fix.
>
> Oops.  That's a brain fart from my side.  Thanks.

Confirmed v2 could fix the issue. One minor comment related with my part:

Should cur_pos be uint32 instead of int? Either of them can work because
cur_pos can be [0, 128], but it may be clearer.

Apart from above, LGTM.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Apr 15, 2025 at 02:49:24AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Should cur_pos be uint32 instead of int? Either of them can work because
> cur_pos can be [0, 128], but it may be clearer.
>
> Apart from above, LGTM.

Sure, why not.  Thanks for the review.

An important thing may be to split the change into two:
- First one for the API in injection_point.c.
- Second one for the new function.

The second patch is still something folks seem to be meh about, but if
the end consensus is to put the new SQL function in the module
injection_points we are going to need the first patch anyway.
--
Michael