Re: Injection points: preloading and runtime arguments
> On 6 Aug 2024, at 12:47, Michael Paquier wrote: > > Hmm. How about loading injection_points with shared_preload_libraries > now that it has a _PG_init() thanks to 75534436a477 to take care of > the initialization you need here? We could add two hooks to request > some shmem based on a size and to do the shmem initialization. SQL initialisation is fine for test purposes. I just considered that I'd better share that doing the same from C code is non-trivial. Thanks! Best regards, Andrey Borodin.
Re: Injection points: preloading and runtime arguments
On Sun, Aug 04, 2024 at 01:02:22AM +0900, Michael Paquier wrote:
> On Fri, Aug 02, 2024 at 07:03:58PM +0300, Andrey M. Borodin wrote:
> > The test works fine with SQL interface “select
> > injection_points_load('get-new-multixact-id');”.
>
> Yes, just use a load() here to make sure that the DSM required by the
> waits are properly initialized before entering in the critical section
> where the wait of the point get-new-multixact-id happens.
Hmm. How about loading injection_points with shared_preload_libraries
now that it has a _PG_init() thanks to 75534436a477 to take care of
the initialization you need here? We could add two hooks to request
some shmem based on a size and to do the shmem initialization.
--
Michael
signature.asc
Description: PGP signature
Re: Injection points: preloading and runtime arguments
On Fri, Aug 02, 2024 at 07:03:58PM +0300, Andrey M. Borodin wrote:
> The test works fine with SQL interface “select
> injection_points_load('get-new-multixact-id');”.
Yes, just use a load() here to make sure that the DSM required by the
waits are properly initialized before entering in the critical section
where the wait of the point get-new-multixact-id happens.
--
Michael
signature.asc
Description: PGP signature
Re: Injection points: preloading and runtime arguments
> On 18 Jul 2024, at 03:55, Michael Paquier wrote:
>
> If there is anything else you would like to see adjusted in this area,
> please let me know.
I’ve tried to switch my multixact test to new INJECTION_POINT_CACHED… and it
does not work for me. Could you please take a look?
2024-08-02 18:52:32.244 MSK [53155] 001_multixact.pl LOG: statement: select
test_create_multixact();
TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"),
File: "mcxt.c", Line: 1186, PID: 53155
0 postgres0x0001031212f0 ExceptionalCondition
+ 236
1 postgres0x00010317a01c MemoryContextAlloc +
240
2 postgres0x000102e66158
dsm_create_descriptor + 80
3 postgres0x000102e66474 dsm_attach + 416
4 postgres0x00010316c264 dsa_attach + 24
5 postgres0x000102e69994 init_dsm_registry +
256
6 postgres0x000102e6965c GetNamedDSMSegment +
492
7 injection_points.dylib 0x00010388f2cc injection_init_shmem
+ 68
8 injection_points.dylib 0x00010388efbc injection_wait + 72
9 postgres0x0001031606bc InjectionPointCached
+ 72
10 postgres0x0001028ffc70
MultiXactIdCreateFromMembers + 360
11 postgres0x0001028ffac8 MultiXactIdCreate +
344
12 test_slru.dylib 0x00010376fa04
test_create_multixact + 52
The test works fine with SQL interface “select
injection_points_load('get-new-multixact-id');”.
Thanks!
Best regards, Andrey Borodin.
vAug2-0001-Add-multixact-CV-sleep-test.patch
Description: Binary data
Re: Injection points: preloading and runtime arguments
On Wed, Jul 17, 2024 at 11:19:41AM +0900, Michael Paquier wrote: > It works for me to do as you are proposing at the end, that could > always be changed if there are more arguments in favor of a different > behavior that plays more with the shmem data. I have taken some time this morning and applied that after a second lookup. Thanks! If there is anything else you would like to see adjusted in this area, please let me know. -- Michael signature.asc Description: PGP signature
Re: Injection points: preloading and runtime arguments
On Tue, Jul 16, 2024 at 11:20:57AM +0300, Heikki Linnakangas wrote: > 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. My point was just to be more aggressive with the cache correctness even in this context. You've also mentioned upthread the point that we should worry about a concurrent detach, which is something that injection_point_cache_get() alone is not able to do as we would not cross-check the generation with what's in the shared area, so I also saw a point about being more aggressive with the check here. It works for me to do as you are proposing at the end, that could always be changed if there are more arguments in favor of a different behavior that plays more with the shmem data. > 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. I'm OK with that, as well. Thanks. -- Michael signature.asc Description: PGP signature
Re: Injection points: preloading and runtime arguments
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
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:
+
+INJECTION_POINT_CACHED(name);
+
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/modul
Re: Injection points: preloading and runtime arguments
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?
--
Michael
From e33ab4202cf5e6ee79eef1927978d45e1cfa6034 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 16 Jul 2024 13:07:58 +0900
Subject: [PATCH] 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.
---
src/include/utils/injection_point.h | 3 ++
src/backend/utils/misc/injection_point.c | 33 ---
.../expected/injection_points.out | 13
.../injection_points--1.0.sql | 10 ++
.../injection_points/injection_points.c | 14
.../injection_points/sql/injection_points.sql | 2 ++
doc/src/sgml/xfunc.sgml | 6 +++-
7 files changed, 76 insertions(+), 5 deletions(-)
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/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 84ad5e470d..9dd6575a9a 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -415,10 +415,11 @@ InjectionPointDetach(const char *name)
* Common workhorse of InjectionPointRun() and InjectionPointLoad()
*
* Checks if an injection point exists in shared memory, and update
- * the local cache entry accordingly.
+ * the local cache entry accordingly. If "direct" is true, return the
+ * point after looking up for it only in the cache.
*/
static InjectionPointCacheEntry *
-InjectionPointCacheRefresh(const char *name)
+InjectionPointCacheRefresh(const char *name, bool direct)
{
uint32 max_inuse;
int namelen;
@@ -461,6 +462,13 @@ InjectionPointCacheRefresh(const char *name)
cached = NULL;
}
+ /*
+ * If using a direct lookup, forget about the shared memory array
+ * and return immediately with the data found in the cache.
+ */
+ if (direct)
+ return cached;
+
/*
* Search the shared memory array.
*
@@ -531,7 +539,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
@@ -546,7 +554,24 @@ InjectionPointRun(const char *name)
#ifdef USE_INJECTION_POINTS
InjectionPointCacheEntry *cache_entry;
- cache_entry = InjectionPointCacheRefresh(name);
+ cache_entry = InjectionPointCacheRefresh(name, false);
+ if (cache_entry)
+ cache_entry->callback(name, cache_entry->private_data);
+#else
+ 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 = InjectionPointCacheRefresh(name, true);
if (cache_entry)
cache_entry->callback(name, cache_entry->private_data);
#else
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('TestInjection
Re: Injection points: preloading and runtime arguments
On Tue, Jul 09, 2024 at 12:08:26PM +0300, Heikki Linnakangas wrote: > And the injection point is attached in between the INJECTION_POINT_LOAD() > and INJECTION_POINT() calls, you will still get an assertion failure. For a > testing facility, maybe that's acceptable, but it could be fixed pretty > easily. > > I propose we introduce an INJECTION_POINT_CACHED(name) macro that *only* > uses the local cache. 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. > We could then also add an assertion in > InjectionPointRun() to check that it's not used in a critical section, to > enforce correct usage. That would be the same as what we do currently with a palloc() coming from load_external_function() or hash_create(), whichever comes first. Okay, the stack reported is deeper in this case. [1]: https://www.postgresql.org/message-id/87c748b3-0786-490f-a17a-51bef63e6...@iki.fi -- Michael signature.asc Description: PGP signature
Re: Injection points: preloading and runtime arguments
On 05/07/2024 12:16, Michael Paquier wrote: On Mon, Jun 10, 2024 at 03:10:33PM +0900, Michael Paquier wrote: OK, cool. I'll try to get that into the tree once v18 opens up. And I've spent more time on this one, and applied it to v18 after some slight tweaks. If you do: INJECTION_POINT_LOAD(foo); START_CRIT_SECTION(); INJECTION_POINT(foo); END_CRIT_SECTION(); And the injection point is attached in between the INJECTION_POINT_LOAD() and INJECTION_POINT() calls, you will still get an assertion failure. For a testing facility, maybe that's acceptable, but it could be fixed pretty easily. I propose we introduce an INJECTION_POINT_CACHED(name) macro that *only* uses the local cache. We could then also add an assertion in InjectionPointRun() to check that it's not used in a critical section, to enforce correct usage. -- Heikki Linnakangas Neon (https://neon.tech)
Re: Injection points: preloading and runtime arguments
On Mon, Jun 10, 2024 at 03:10:33PM +0900, Michael Paquier wrote: > OK, cool. I'll try to get that into the tree once v18 opens up. And I've spent more time on this one, and applied it to v18 after some slight tweaks. Please feel free to re-post your tests with multixacts, Andrey. -- Michael signature.asc Description: PGP signature
Re: Injection points: preloading and runtime arguments
On Sat, Jun 08, 2024 at 04:52:25PM +0500, Andrey M. Borodin wrote:
> Alvaro, here’s the test for multixact CV sleep that I was talking
> about on PGConf.
> It is needed to test [0]. It is based on loaded injection
> points.
> This technique is not committed yet, but the patch looks good.
OK, cool. I'll try to get that into the tree once v18 opens up. I
can see that GetNewMultiXactId() opens a critical section. I am
slightly surprised that you need both the SQL function
injection_points_load() and the macro INJECTION_POINT_LOAD().
Wouldn't one or the other be sufficient?
The test takes 20ms to run here, which is good enough.
+ INJECTION_POINT_LOAD("GetNewMultiXactId-done");
[...]
+ INJECTION_POINT("GetNewMultiXactId-done");
[...]
+ INJECTION_POINT("GetMultiXactIdMembers-CV-sleep");
Be careful about the naming here. All the points use lower case
characters currently.
+# and another multixact have no offest yet, we must wait until this offset
s/offest/offset/.
> When all prerequisites are ready I will post it to corresponding
> thread and create CF item.
OK, let's do that.
--
Michael
signature.asc
Description: PGP signature
Re: Injection points: preloading and runtime arguments
> On 7 Jun 2024, at 04:38, Michael Paquier wrote: Thanks Michael! Tests of injection points with injection points are neat :) Alvaro, here’s the test for multixact CV sleep that I was talking about on PGConf. It is needed to test [0]. It is based on loaded injection points. This technique is not committed yet, but the patch looks good. When all prerequisites are ready I will post it to corresponding thread and create CF item. Thanks! Best regards, Andrey Borodin. [0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a0e0fb1ba vAB1-0001-Support-loading-of-injection-points.patch Description: Binary data vAB1-0002-Add-multixact-CV-sleep-test.patch Description: Binary data
Re: Injection points: preloading and runtime arguments
On Thu, Jun 06, 2024 at 03:47:47PM +0500, Andrey M. Borodin wrote: > Is it OK to detach() before wakeup()? Or, perhaps, can a detach() do a > wakeup() automatically? It is OK to do a detach before a wakeup. Noah has been relying on this behavior in an isolation test for a patch he's worked on. See inplace110-successors-v1.patch here: https://www.postgresql.org/message-id/20240512232923.aa.nmi...@google.com That's also something we've discussed for 33181b48fd0e, where Noah wanted to emulate in an automated fashion what one can do with a debugger and one or more breakpoints. Not sure that wakeup() involving a automated detach() is the behavior to hide long-term, actually, as there is also an argument for waking up a point and *not* detach it to force multiple waits. -- Michael signature.asc Description: PGP signature
Re: Injection points: preloading and runtime arguments
> On 5 Jun 2024, at 03:52, Michael Paquier wrote:
>
> Another thing you could do is to define a
> INJECTION_POINT_LOAD() in the code path you're stressing outside the
> critical section where the point is run. This should save from a call
> to the SQL function. This choice is up to the one implementing the
> test, both can be useful depending on what one is trying to achieve.
Thanks!
Interestingly, previously having INJECTION_POINT_PRELOAD() was not enough.
But now both INJECTION_POINT_LOAD() or injection_points_load() do the trick, so
for me any of them is enough.
My test works with current version, but I have one slight problem, I need to
call
$node->safe_psql('postgres', q(select
injection_points_detach('GetMultiXactIdMembers-CV-sleep')));
Before
$node->safe_psql('postgres', q(select
injection_points_wakeup('GetMultiXactIdMembers-CV-sleep')));
Is it OK to detach() before wakeup()? Or, perhaps, can a detach() do a wakeup()
automatically?
Best regards, Andrey Borodin.
Re: Injection points: preloading and runtime arguments
On Tue, May 21, 2024 at 04:29:54PM +0500, Andrey M. Borodin wrote:
> Currently I'm working on the test using this
> $creator->query_until(qr/start/, q(
> \echo start
> select injection_points_wakeup('');
> select test_create_multixact();
> ));
>
> I'm fine if instead of injection_points_wakeup('') I'll have to use
> select injection_points_preload('point name');.
Based on our discussion of last week, please find attached the
promised patch set to allow your SLRU tests to work. I have reversed
the order of the patches, moving the loading part in 0001 and the
addition of the runtime arguments in 0002 as we have a use-case for
the loading, nothing yet for the runtime arguments.
I have also come back to the naming, feeling that "preload" was
overcomplicated. So I have used the word "load" instead across the
board for 0001.
Note that the SQL function injection_points_load() does now an
initialization of the shmem area when a process plugs into the module
for the first time, fixing the issue you have mentioned with your SLRU
test. Hence, you should be able to do a load(), then a wait in the
critical section as there would be no memory allocation done when the
point runs. Another thing you could do is to define a
INJECTION_POINT_LOAD() in the code path you're stressing outside the
critical section where the point is run. This should save from a call
to the SQL function. This choice is up to the one implementing the
test, both can be useful depending on what one is trying to achieve.
--
Michael
From fe58c08881c7fea3be94e6a166d621e8acc78a92 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Wed, 5 Jun 2024 07:35:29 +0900
Subject: [PATCH v2 1/2] Support loading of injection points
This can be used to load an injection point in the backend-level cache
before running it, to avoid issues if the point cannot be loaded due to
restrictions in the code path where it would be run, like a critical
section.
---
src/include/utils/injection_point.h | 3 +
src/backend/utils/misc/injection_point.c | 116 --
.../expected/injection_points.out | 32 +
.../injection_points--1.0.sql | 10 ++
.../injection_points/injection_points.c | 17 +++
.../injection_points/sql/injection_points.sql | 7 ++
doc/src/sgml/xfunc.sgml | 14 +++
7 files changed, 163 insertions(+), 36 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index a61d5d4439..bd3a62425c 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -15,8 +15,10 @@
* Injections points require --enable-injection-points.
*/
#ifdef USE_INJECTION_POINTS
+#define INJECTION_POINT_LOAD(name) InjectionPointLoad(name)
#define INJECTION_POINT(name) InjectionPointRun(name)
#else
+#define INJECTION_POINT_LOAD(name) ((void) name)
#define INJECTION_POINT(name) ((void) name)
#endif
@@ -34,6 +36,7 @@ extern void InjectionPointAttach(const char *name,
const char *function,
const void *private_data,
int private_data_size);
+extern void InjectionPointLoad(const char *name);
extern void InjectionPointRun(const char *name);
extern bool InjectionPointDetach(const char *name);
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 5c2a0d2297..f02ed6c08b 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -129,20 +129,47 @@ injection_point_cache_remove(const char *name)
(void) hash_search(InjectionPointCache, name, HASH_REMOVE, NULL);
}
+/*
+ * injection_point_cache_load
+ *
+ * Load an injection point into the local cache.
+ */
+static void
+injection_point_cache_load(InjectionPointEntry *entry_by_name)
+{
+ char path[MAXPGPATH];
+ void *injection_callback_local;
+
+ snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+ entry_by_name->library, DLSUFFIX);
+
+ if (!pg_file_exists(path))
+ elog(ERROR, "could not find library \"%s\" for injection point \"%s\"",
+ path, entry_by_name->name);
+
+ injection_callback_local = (void *)
+ load_external_function(path, entry_by_name->function, false, NULL);
+
+ if (injection_callback_local == NULL)
+ elog(ERROR, "could not find function \"%s\" in library \"%s\" for injection point \"%s\"",
+ entry_by_name->function, path, entry_by_name->name);
+
+ /* add it to the local cache when found */
+ injection_point_cache_add(entry_by_name->name, injection_callback_local,
+ entry_by_name->private_data);
+}
+
/*
* injection_point_cache_get
*
* Retrieve an injection point from the local cache, if any.
*/
-static InjectionPointCallback
-injection_point_cache_get(const char *name, const void **private_data)
+static InjectionPointCacheEntry *
+injection_point_cache_get(const char *name)
{
bool found;
InjectionPointCacheEntry *entry;
- if (private_data)
- *private_data = NULL;
-
/* no
Re: Injection points: preloading and runtime arguments
> On 21 May 2024, at 06:31, Michael Paquier wrote:
>
> So I agree that 0002 ought to call injection_init_shmem() when calling
> injection_points_preload(), but it also seems to me that the test is
> missing the fact that it should heat the backend cache to avoid the
> allocations in the critical sections.
>
> Note that I disagree with taking a shortcut in the backend-side
> injection point code where we would bypass CritSectionCount or
> allowInCritSection. These states should stay consistent for the sake
> of the callbacks registered so as these can rely on the same stack and
> conditions as the code where they are called.
Currently I'm working on the test using this
$creator->query_until(qr/start/, q(
\echo start
select injection_points_wakeup('');
select test_create_multixact();
));
I'm fine if instead of injection_points_wakeup('') I'll have to use select
injection_points_preload('point name');.
Thanks!
Best regards, Andrey Borodin.
Re: Injection points: preloading and runtime arguments
On Mon, May 20, 2024 at 05:01:15PM +0500, Andrey M. Borodin wrote:
> Both features look useful to me.
> I've tried to rebase my test of CV sleep during multixact generation[0]. I
> used it like this:
>
> INJECTION_POINT_PRELOAD("GetNewMultiXactId-done");
> multi = GetNewMultiXactId(nmembers, &offset); // starts critsection
> INJECTION_POINT("GetNewMultiXactId-done");
Thanks for the feedback.
> And it fails like this:
>
> 2024-05-20 16:50:40.430 +05 [21830] 001_multixact.pl LOG: statement: select
> test_create_multixact();
> TRAP: failed Assert("CritSectionCount == 0 ||
> (context)->allowInCritSection"), File: "mcxt.c", Line: 1185, PID: 21830
> 0 postgres0x000101452ed0
> ExceptionalCondition + 220
> 1 postgres0x0001014a6050 MemoryContextAlloc
> + 208
> 2 postgres0x0001011c3bf0
> dsm_create_descriptor + 72
> 3 postgres0x0001011c3ef4 dsm_attach + 400
> 4 postgres0x0001014990d8 dsa_attach + 24
> 5 postgres0x0001011c716c init_dsm_registry
> + 240
> 6 postgres0x0001011c6e60 GetNamedDSMSegment
> + 456
> 7 injection_points.dylib 0x000101c871f8
> injection_init_shmem + 60
> 8 injection_points.dylib 0x000101c86f1c injection_wait + 64
> 9 postgres0x00010148e228
> InjectionPointRunInternal + 376
> 10 postgres0x00010148e0a4 InjectionPointRun
> + 32
> 11 postgres0x000100cab798
> MultiXactIdCreateFromMembers + 344
> 12 postgres0x000100cab604 MultiXactIdCreate
> + 312
>
> Am I doing something wrong? Seems like extension have to know too that it is
> preloaded.
Your stack is pointing at the shared memory section initialized in the
module injection_points, which is a bit of a chicken-and-egg problem
because you'd want an extra preload to happen before even that, like a
pre-preload. From what I can see, you have a good point about the
shmem initialized in the module: injection_points_preload() should
call injection_init_shmem() so as this area would not trigger the
assertion.
However, there is a second thing here inherent to your test: shouldn't
the script call injection_points_preload() to make sure that the local
cache behind GetNewMultiXactId-done is fully allocated and prepared
for the moment where injection point will be run?
So I agree that 0002 ought to call injection_init_shmem() when calling
injection_points_preload(), but it also seems to me that the test is
missing the fact that it should heat the backend cache to avoid the
allocations in the critical sections.
Note that I disagree with taking a shortcut in the backend-side
injection point code where we would bypass CritSectionCount or
allowInCritSection. These states should stay consistent for the sake
of the callbacks registered so as these can rely on the same stack and
conditions as the code where they are called.
--
Michael
signature.asc
Description: PGP signature
Re: Injection points: preloading and runtime arguments
> On 20 May 2024, at 17:01, Andrey M. Borodin wrote: Ugh, accidentally send without attaching the patch itself. Sorry for the noise. Best regards, Andrey Borodin. 0001-Add-multixact-CV-sleep.patch Description: Binary data
Re: Injection points: preloading and runtime arguments
Hi!
> On 20 May 2024, at 08:18, Michael Paquier wrote:
Both features look useful to me.
I've tried to rebase my test of CV sleep during multixact generation[0]. I used
it like this:
INJECTION_POINT_PRELOAD("GetNewMultiXactId-done");
multi = GetNewMultiXactId(nmembers, &offset); // starts critsection
INJECTION_POINT("GetNewMultiXactId-done");
And it fails like this:
2024-05-20 16:50:40.430 +05 [21830] 001_multixact.pl LOG: statement: select
test_create_multixact();
TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"),
File: "mcxt.c", Line: 1185, PID: 21830
0 postgres0x000101452ed0 ExceptionalCondition
+ 220
1 postgres0x0001014a6050 MemoryContextAlloc +
208
2 postgres0x0001011c3bf0
dsm_create_descriptor + 72
3 postgres0x0001011c3ef4 dsm_attach + 400
4 postgres0x0001014990d8 dsa_attach + 24
5 postgres0x0001011c716c init_dsm_registry +
240
6 postgres0x0001011c6e60 GetNamedDSMSegment +
456
7 injection_points.dylib 0x000101c871f8 injection_init_shmem
+ 60
8 injection_points.dylib 0x000101c86f1c injection_wait + 64
9 postgres0x00010148e228
InjectionPointRunInternal + 376
10 postgres0x00010148e0a4 InjectionPointRun +
32
11 postgres0x000100cab798
MultiXactIdCreateFromMembers + 344
12 postgres0x000100cab604 MultiXactIdCreate +
312
Am I doing something wrong? Seems like extension have to know too that it is
preloaded.
Best regards, Andrey Borodin.
[0]
https://www.postgresql.org/message-id/0925F9A9-4D53-4B27-A87E-3D83A757B0E0%40yandex-team.ru
Injection points: preloading and runtime arguments
Hi all,
I have a couple of extra toys for injection points in my bucket that
I'd like to propose for integration in v18, based on some feedback I
have received:
1) Preload an injection point into the backend-level cache without
running it. This has come up because an injection point run for the
first time needs to be loaded with load_external_function that
internally does some allocations, and this would not work if the
injection point is in a critical section. Being able to preload an
injection point requires an extra macro, called
INJECTION_POINT_PRELOAD. Perhaps "load" makes more sense as keyword,
here.
2) Grab values at runtime from the code path where an injection point
is run and give them to the callback. The case here is to be able to
do some dynamic manipulation or a stack, reads of some runtime data or
even decide of a different set of actions in a callback based on what
the input has provided. One case that I've been playing with here is
the dynamic manipulation of pages in specific code paths to stress
internal checks, as one example. This introduces a 1-argument
version, as multiple args could always be passed down to the callback
within a structure.
The in-core module injection_points is extended to provide a SQL
interface to be able to do the preloading or define a callback with
arguments. The two changes are split into patches of their own.
These new facilities could be backpatched if there is a need for them
in the future in stable branches, as these are aimed for tests and the
changes do not introduce any ABI breakages with the existing APIs or
the in-core module.
Thoughts and comments are welcome.
--
Michael
From ed617725d1e6a156363384ed5fcbf4e79b5f8ab4 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 20 May 2024 11:50:42 +0900
Subject: [PATCH 1/2] Extend injection points with optional runtime arguments
This extends injections points with a 1-argument flavor, so as it
becomes possible for callbacks to pass down values coming from the code
paths where an injection point is defined. The primary use case that
can be covered in this function is runtime manipulation of a given
stack, where it would be possible to corrupt a running state, based on a
runtime set of conditions.
For example, imagine a class of failures in the solar flare category
causing bits to flip on a page.
---
src/include/utils/injection_point.h | 9 +-
src/backend/utils/misc/injection_point.c | 92 +--
.../expected/injection_points.out | 31 +++
.../injection_points--1.0.sql | 10 ++
.../injection_points/injection_points.c | 39 +++-
.../injection_points/sql/injection_points.sql | 9 ++
doc/src/sgml/xfunc.sgml | 9 +-
7 files changed, 169 insertions(+), 30 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index a61d5d4439..c2c0840706 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -16,8 +16,10 @@
*/
#ifdef USE_INJECTION_POINTS
#define INJECTION_POINT(name) InjectionPointRun(name)
+#define INJECTION_POINT_1ARG(name, arg1) InjectionPointRun1Arg(name, arg1)
#else
#define INJECTION_POINT(name) ((void) name)
+#define INJECTION_POINT_1ARG(name) ((void) name, (void) arg1)
#endif
/*
@@ -25,6 +27,9 @@
*/
typedef void (*InjectionPointCallback) (const char *name,
const void *private_data);
+typedef void (*InjectionPointCallback1Arg) (const char *name,
+ const void *private_data,
+ const void *arg1);
extern Size InjectionPointShmemSize(void);
extern void InjectionPointShmemInit(void);
@@ -33,8 +38,10 @@ extern void InjectionPointAttach(const char *name,
const char *library,
const char *function,
const void *private_data,
- int private_data_size);
+ int private_data_size,
+ int num_args);
extern void InjectionPointRun(const char *name);
+extern void InjectionPointRun1Arg(const char *name, void *arg1);
extern bool InjectionPointDetach(const char *name);
#endif /* INJECTION_POINT_H */
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 5c2a0d2297..2bcdb2708c 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -56,6 +56,9 @@ typedef struct InjectionPointEntry
* callbacks, registered when attached.
*/
char private_data[INJ_PRIVATE_MAXLEN];
+
+ /* Number of arguments used by the callback */
+ int num_args;
} InjectionPointEntry;
#define INJECTION_POINT_HASH_INIT_SIZE 16
@@ -69,7 +72,12 @@ typedef struct InjectionPointCacheEntry
{
char name[INJ_NAME_MAXLEN];
char private_data[INJ_PRIVATE_MAXLEN];
- InjectionPointCallback callback;
+ int num_args;
+ union
+ {
+ InjectionPointCallback callback;
+ InjectionPointCallback1Arg callback_1arg;
+ } data;
} InjectionPointCacheEntry;
static HTAB
