On 16/07/2024 07:09, Michael Paquier wrote:
On Wed, Jul 10, 2024 at 01:16:15PM +0900, Michael Paquier wrote:
You mean with something that does a injection_point_cache_get()
followed by a callback run if anything is found in the local cache?
Why not. Based on what you have posted at [1], it looks like this had
better check the contents of the cache's generation with what's in
shmem, as well as destroying InjectionPointCache if there is nothing
else, so there's a possible dependency here depending on how much
maintenance this should do with the cache to keep it consistent.
Now that 86db52a5062a is in the tree, this could be done with a
shortcut in InjectionPointCacheRefresh(). What do you think about
something like the attached, with your suggested naming?
Yes, +1 for something like that.
The "direct" argument to InjectionPointCacheRefresh() feels a bit weird.
Also weird that it still checks ActiveInjectionPoints->max_inuse, even
though it otherwise operates on the cached version only. I think you can
just call injection_point_cache_get() directly from
InjectionPointCached(), per attached.
I also rephrased the docs section a bit, focusing more on why and how
you use the LOAD/CACHED pair, and less on the mechanics of how it works.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 03a91db4fc7af15651f735eef4295c7d5883def7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 16 Jul 2024 13:07:58 +0900
Subject: [PATCH v2 1/2] Add INJECTION_POINT_CACHED()
This works in combination with INJECTION_POINT_LOAD(), ensuring that an
injection point can be run without touching the shared memory area
related to injection points.
---
doc/src/sgml/xfunc.sgml | 6 +++++-
src/backend/utils/misc/injection_point.c | 19 ++++++++++++++++++-
src/include/utils/injection_point.h | 3 +++
.../expected/injection_points.out | 13 +++++++++++++
.../injection_points--1.0.sql | 10 ++++++++++
.../injection_points/injection_points.c | 14 ++++++++++++++
.../injection_points/sql/injection_points.sql | 2 ++
7 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 756a9d07fb..f02a48ead4 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3629,7 +3629,11 @@ INJECTION_POINT_LOAD(name);
doing all memory allocations at this stage without running the callback.
This is useful when an injection point is attached in a critical section
where no memory can be allocated: load the injection point outside the
- critical section, then run it in the critical section.
+ critical section, then run it in the critical section directly from
+ the process cache:
+<programlisting>
+INJECTION_POINT_CACHED(name);
+</programlisting>
</para>
<para>
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 84ad5e470d..1fa7d95686 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -531,7 +531,7 @@ void
InjectionPointLoad(const char *name)
{
#ifdef USE_INJECTION_POINTS
- InjectionPointCacheRefresh(name);
+ InjectionPointCacheRefresh(name, false);
#else
elog(ERROR, "Injection points are not supported by this build");
#endif
@@ -553,3 +553,20 @@ InjectionPointRun(const char *name)
elog(ERROR, "Injection points are not supported by this build");
#endif
}
+
+/*
+ * Execute an injection point directly from the cache, if defined.
+ */
+void
+InjectionPointCached(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+ InjectionPointCacheEntry *cache_entry;
+
+ cache_entry = injection_point_cache_get(name);
+ if (cache_entry)
+ cache_entry->callback(name, cache_entry->private_data);
+#else
+ elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index bd3a62425c..a385e3df64 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -17,9 +17,11 @@
#ifdef USE_INJECTION_POINTS
#define INJECTION_POINT_LOAD(name) InjectionPointLoad(name)
#define INJECTION_POINT(name) InjectionPointRun(name)
+#define INJECTION_POINT_CACHED(name) InjectionPointCached(name)
#else
#define INJECTION_POINT_LOAD(name) ((void) name)
#define INJECTION_POINT(name) ((void) name)
+#define INJECTION_POINT_CACHED(name) ((void) name)
#endif
/*
@@ -38,6 +40,7 @@ extern void InjectionPointAttach(const char *name,
int private_data_size);
extern void InjectionPointLoad(const char *name);
extern void InjectionPointRun(const char *name);
+extern void InjectionPointCached(const char *name);
extern bool InjectionPointDetach(const char *name);
#endif /* INJECTION_POINT_H */
diff --git a/src/test/modules/injection_points/expected/injection_points.out b/src/test/modules/injection_points/expected/injection_points.out
index 2f60da900b..f25bbe4966 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -129,6 +129,12 @@ SELECT injection_points_detach('TestInjectionLog2');
(1 row)
-- Loading
+SELECT injection_points_cached('TestInjectionLogLoad'); -- nothing in cache
+ injection_points_cached
+-------------------------
+
+(1 row)
+
SELECT injection_points_load('TestInjectionLogLoad'); -- nothing
injection_points_load
-----------------------
@@ -147,6 +153,13 @@ SELECT injection_points_load('TestInjectionLogLoad'); -- nothing happens
(1 row)
+SELECT injection_points_cached('TestInjectionLogLoad'); -- runs from cache
+NOTICE: notice triggered for injection point TestInjectionLogLoad
+ injection_points_cached
+-------------------------
+
+(1 row)
+
SELECT injection_points_run('TestInjectionLogLoad'); -- runs from cache
NOTICE: notice triggered for injection point TestInjectionLogLoad
injection_points_run
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
index e275c2cf5b..0f280419a5 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -34,6 +34,16 @@ RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_run'
LANGUAGE C STRICT PARALLEL UNSAFE;
+--
+-- injection_points_cached()
+--
+-- Executes the action attached to the injection point, from local cache.
+--
+CREATE FUNCTION injection_points_cached(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_cached'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
--
-- injection_points_wakeup()
--
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index b6c8e89324..15f9d0233c 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -333,6 +333,20 @@ injection_points_run(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+/*
+ * SQL function for triggering an injection point from cache.
+ */
+PG_FUNCTION_INFO_V1(injection_points_cached);
+Datum
+injection_points_cached(PG_FUNCTION_ARGS)
+{
+ char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+ INJECTION_POINT_CACHED(name);
+
+ PG_RETURN_VOID();
+}
+
/*
* SQL function for waking up an injection point waiting in injection_wait().
*/
diff --git a/src/test/modules/injection_points/sql/injection_points.sql b/src/test/modules/injection_points/sql/injection_points.sql
index fabf0a8823..e3a481d604 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -42,9 +42,11 @@ SELECT injection_points_run('TestInjectionLog2'); -- notice
SELECT injection_points_detach('TestInjectionLog2');
-- Loading
+SELECT injection_points_cached('TestInjectionLogLoad'); -- nothing in cache
SELECT injection_points_load('TestInjectionLogLoad'); -- nothing
SELECT injection_points_attach('TestInjectionLogLoad', 'notice');
SELECT injection_points_load('TestInjectionLogLoad'); -- nothing happens
+SELECT injection_points_cached('TestInjectionLogLoad'); -- runs from cache
SELECT injection_points_run('TestInjectionLogLoad'); -- runs from cache
SELECT injection_points_detach('TestInjectionLogLoad');
--
2.39.2
From e12026251ae4817e73a41c00abb1bb6884a2b248 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 16 Jul 2024 11:15:39 +0300
Subject: [PATCH v2 2/2] rephrase docs paragraph
---
doc/src/sgml/xfunc.sgml | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index f02a48ead4..7e92e89846 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3619,21 +3619,20 @@ INJECTION_POINT(name);
</para>
<para>
- An injection point with a given <literal>name</literal> can be loaded
- using macro:
+ Executing an injection point can require allocating a small amount of
+ memory, which can fail. If you need to have an injection point in a
+ critical section where dynamic allocations are not allowed, you can use
+ a two-step approach with the following macros:
<programlisting>
INJECTION_POINT_LOAD(name);
-</programlisting>
-
- This will load the injection point callback into the process cache,
- doing all memory allocations at this stage without running the callback.
- This is useful when an injection point is attached in a critical section
- where no memory can be allocated: load the injection point outside the
- critical section, then run it in the critical section directly from
- the process cache:
-<programlisting>
INJECTION_POINT_CACHED(name);
</programlisting>
+
+ Before entering the critical section,
+ call <function>INJECTION_POINT_LOAD</function>. It checks the shared
+ memory state, and loads the callback into backend-private memory if it is
+ active. Inside the critical section, use
+ <function>INJECTION_POINT_CACHED</function> to execute the callback.
</para>
<para>
--
2.39.2
