(Compiled two separate review emails into a single one)
On Tue, Jan 02, 2024 at 03:36:12PM +0530, Ashutosh Bapat wrote:
> This discussion has not been addressed in v6. I think the interface
> needs to be documented in the order below
> INJECTION_POINT - this declares an injection point - i.e. a place in
> code where an external code can be injected (and run).
> InjectionPointAttach() - this is used to associate ("attach") external
> code to an injection point.
> InjectionPointDetach() - this is used to disassociate ("detach")
> external code from an injection point.
>
> [arguments about doc organization]
>
> Even if an INJECTION_POINT is not "declared" attach would succeed but
> doesn't do anything. I think this needs to be made clear in the
> documentation. Better if we could somehow make Attach() fail if the
> specified injection point is not "declared" using INJECTION_POINT. Of
> course we don't want to bloat the hash table with all "declared"
> injection points even if they aren't being attached to and hence not
> used.
Okay, I can see your point. I have reorganized the docs in the
following order:
- INJECTION_POINT
- Attach
- Detach
> I think, exposing the current injection point strings as
> #defines and encouraging users to use these macros instead of string
> literals will be a good start.
Nah, I disagree with this one actually. It is easy to grep for the
macro INJECTION_POINT to be able to achieve the same research job, and
this would make the code more inconsistent when callbacks are run
within extensions which don't care about a #define in a backend
header.
> With the current implementation it's possible to "declare" injection
> point with same name at multiple places. It's useful but is it
> intended?
Yes. I would recommend not doing that, but I don't see why there
would be a point in restricting that, either.
> /* Field sizes */
> #define INJ_NAME_MAXLEN 64
> #define INJ_LIB_MAXLEN 128
> #define INJ_FUNC_MAXLEN 128
> I think these limits should be either documented or specified in the
> error messages for users to fix their code in case of
> errors/unexpected behaviour.
Adding them to the error messages when attaching is a good idea.
Done.
> This is not an array entry anymore. I think we should rename
> InjectionPointEntry as SharedInjectionPointEntry and InjectionPointArrayEntry
> as LocalInjectionPointEntry.
Yep, fixed.
> +/* utilities to handle the local array cache */
> +static void
> +injection_point_cache_add(const char *name,
> + InjectionPointCallback callback)
> +{
> ... snip ...
> +
> + entry = (InjectionPointCacheEntry *)
> + hash_search(InjectionPointCache, name, HASH_ENTER, &found);
> +
> + if (!found)
>
> The function is called only when the injection point is not found in the local
> cache. So this condition will always be true. An Assert will help to make it
> clear and also prevent an unintended callback replacement.
Right, as coded that seems pointless to make the found conditional. I
think that I coded it this way when doing some earlier work with this
code, and finished with a simpler thing.
> +#ifdef USE_INJECTION_POINTS
> +static bool
> +file_exists(const char *name)
>
> There's similar function in jit.c and dfmgr.c. Can we not reuse that code?
This has been mentioned in a different comment. Refactored as of
0001, but there is something here related to EACCES for the JIT path.
Seems weird to me that we would not fail if the JIT library cannot be
accessed when stat() fails.
> + /* Save the entry */
> + memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
> + entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
> + memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
> + entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
> + memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
> + entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
>
> Most of the code is using strncpy instead of memcpy. Why is this code
> different?
strncpy() is less used in the backend code. It comes to a matter of
taste, IMO.
> + injection_callback = injection_point_cache_get(name);
> + if (injection_callback == NULL)
> + {
> + char path[MAXPGPATH];
> +
> + /* Found, so just run the callback registered */
>
> The condition indicates that the callback was not found. Comment looks wrong.
Fixed.
> Consider case
>
> Backend 2
> InjectionPointAttach("xyz", "abc", "pqr");
>
> Backend 1
> INJECTION_POINT("xyz");
>
> Backend 2
> InjectionPointDetach("xyz");
> InjectionPointAttach("xyz", "uvw", "lmn");
>
> Backend 1
> INJECTION_POINT("xyz");
>
> IIUC, the last INJECTION_POINT would run abc.pqr instead of uvw.lmn.
> Am I correct?
Yeah, that's an intended design choice to keep the code simpler and
faster as there is no need to track the library and function names in
the local caches or implement something similar to invalidation
messages for this facility because it would impact performance anyway
in the call paths. In short, just don't do that, or use two distinct
points.
On Thu, Jan 04, 2024 at 06:22:35PM +0530, Ashutosh Bapat wrote:
> One more comment on 0001
> InjectionPointAttach() doesn't test whether the given function exists
> in the given library. Even if InjectionPointAttach() succeeds,
> INJECTION_POINT might throw error because the function doesn't exist.
> This can be seen as an unwanted behaviour. I think
> InjectionPointAttach() should test whether the function exists and
> possibly load it as well by adding it to the local cache.
This has the disadvantage of filling the local cache but that may not
be necessary with an extra load_external_function() in the attach
path. I agree to make things safer, but I would do that when
attempting to run the callback instead. Perhaps there's an argument
for the case of somebody replacing a library on-the-fly. I don't
really buy it, but people like doing fancy things sometimes.
> 0002 comments
> --- /dev/null
> +++
> b/src/test/modules/test_injection_points/expected/test_injection_points.out
>
> When built without injection point support, this test fails. We should
> add an alternate output file for such a build so that the behaviour
> with and without injection point support is tested. Or set up things
> such that the test is not run under make check in that directory. I
> will prefer the first option.
src/test/modules/Makefile has a safeguard for ./configure, and there's
one in test_injection_points/meson.build for Meson. The test is not
run when the switches are not used, rather than using an alternate
output file. There was a different issue when moving the tests to
src/test/recovery/, though, where we need to make the execution of the
tests conditional on get_option('injection_points').
> +
> +SELECT test_injection_points_run('TestInjectionError'); -- error
> +ERROR: error triggered for injection point TestInjectionError
> +-- Re-load and run again.
>
> What's getting Re-loaded here? \c will create a new connection and
> thus a new backend. Maybe the comment should say "test in a fresh
> backend" or something of that sort?
The local cache is reloaded. Reworded.
> Looks confusing to me, we are testing the one removed as well. Am I
> missing something?
> [...]
> We aren't removing all entries TestInjectionLog2 is still there. Am I
> missing something?
Reworded all that.
> +# after recovery, the server will not start, and log PANIC: could not
> locate a valid checkpoint record
>
> IIUC the comment describes the behaviour with 7863ee4def65 reverted.
> But the test after this comment is written for the behaviour with
> 7863ee4def65. That's confusing. Is the intent to describe both
> behaviours in the comment?
This came from the original test case posted on the thread that
treated this bug. There's more that bugs me for this script that I
would like to polish. Let's focus on 0001 and 0002 for now..
> According to the prologue of ConditionVariableSleep(), that function
> should be called in a loop checking for the desired condition. All the
> callers that I examined follow that pattern. I think we need to follow
> that pattern here as well.
>
> Below comment from ConditionVariableTimedSleep() makes me think that
> the caller of ConditionVariableSleep() can be woken up even if the
> condition variable was not signaled. That's why the while() loop
> around ConditionVariableSleep().
That's the thing here, we don't have an extra condition to check
after. The variable sleep is what triggers the stop. :)
Perhaps this could be made smarter or with something else, I'm OK to
revisit that with the polishing for 0003 I'm planning. We could use a
separate shared state, for example, but that does not improve the test
readability, either.
Attached is a v7 series. What do you think? 0004 and 0005 for the
extra tests still need more discussion and much more polishing, IMO.
--
Michael
From 1aea179dd07e4551a676c3c0a3bbf2a1bffc9fec Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 10:32:29 +0900
Subject: [PATCH v7 1/5] Refactor code to check file file existence
jit.c and dfgr.c had a copy of the same code to check if a file exists
or not. This refactored routine will be used by an upcoming patch.
Note that this adds a check on EACCES for the JIT path.
---
src/include/storage/fd.h | 1 +
src/backend/jit/jit.c | 20 +-------------------
src/backend/storage/file/fd.c | 22 ++++++++++++++++++++++
src/backend/utils/fmgr/dfmgr.c | 25 ++++---------------------
4 files changed, 28 insertions(+), 40 deletions(-)
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index c4c60bc0a8..60bba5c970 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -182,6 +182,7 @@ extern int pg_fsync(int fd);
extern int pg_fsync_no_writethrough(int fd);
extern int pg_fsync_writethrough(int fd);
extern int pg_fdatasync(int fd);
+extern bool pg_file_exists(const char *fname);
extern void pg_flush_data(int fd, off_t offset, off_t nbytes);
extern int pg_truncate(const char *path, off_t length);
extern void fsync_fname(const char *fname, bool isdir);
diff --git a/src/backend/jit/jit.c b/src/backend/jit/jit.c
index 3f9848e726..d323c199ea 100644
--- a/src/backend/jit/jit.c
+++ b/src/backend/jit/jit.c
@@ -45,7 +45,6 @@ static bool provider_failed_loading = false;
static bool provider_init(void);
-static bool file_exists(const char *name);
/*
@@ -89,7 +88,7 @@ provider_init(void)
*/
snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path, jit_provider, DLSUFFIX);
elog(DEBUG1, "probing availability of JIT provider at %s", path);
- if (!file_exists(path))
+ if (!pg_file_exists(path))
{
elog(DEBUG1,
"provider not available, disabling JIT for current session");
@@ -188,20 +187,3 @@ InstrJitAgg(JitInstrumentation *dst, JitInstrumentation *add)
INSTR_TIME_ADD(dst->optimization_counter, add->optimization_counter);
INSTR_TIME_ADD(dst->emission_counter, add->emission_counter);
}
-
-static bool
-file_exists(const char *name)
-{
- struct stat st;
-
- Assert(name != NULL);
-
- if (stat(name, &st) == 0)
- return !S_ISDIR(st.st_mode);
- else if (!(errno == ENOENT || errno == ENOTDIR))
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not access file \"%s\": %m", name)));
-
- return false;
-}
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 8917c6004a..c3585ced34 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -493,6 +493,28 @@ retry:
return rc;
}
+/*
+ * pg_file_exists -- check that a given file exists.
+ *
+ * This requires an absolute path to the file.
+ */
+bool
+pg_file_exists(const char *name)
+{
+ struct stat st;
+
+ Assert(name != NULL);
+
+ if (stat(name, &st) == 0)
+ return !S_ISDIR(st.st_mode);
+ else if (!(errno == ENOENT || errno == ENOTDIR || errno == EACCES))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not access file \"%s\": %m", name)));
+
+ return false;
+}
+
/*
* pg_flush_data --- advise OS that the described dirty data should be flushed
*
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 638eddf19f..eafa0128ef 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -33,6 +33,7 @@
#include "fmgr.h"
#include "lib/stringinfo.h"
#include "miscadmin.h"
+#include "storage/fd.h"
#include "storage/shmem.h"
#include "utils/hsearch.h"
@@ -78,7 +79,6 @@ char *Dynamic_library_path;
static void *internal_load_library(const char *libname);
static void incompatible_module_error(const char *libname,
const Pg_magic_struct *module_magic_data) pg_attribute_noreturn();
-static bool file_exists(const char *name);
static char *expand_dynamic_library_name(const char *name);
static void check_restricted_library_name(const char *name);
static char *substitute_libpath_macro(const char *name);
@@ -400,23 +400,6 @@ incompatible_module_error(const char *libname,
errdetail_internal("%s", details.data)));
}
-static bool
-file_exists(const char *name)
-{
- struct stat st;
-
- Assert(name != NULL);
-
- if (stat(name, &st) == 0)
- return !S_ISDIR(st.st_mode);
- else if (!(errno == ENOENT || errno == ENOTDIR || errno == EACCES))
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not access file \"%s\": %m", name)));
-
- return false;
-}
-
/*
* If name contains a slash, check if the file exists, if so return
@@ -447,7 +430,7 @@ expand_dynamic_library_name(const char *name)
else
{
full = substitute_libpath_macro(name);
- if (file_exists(full))
+ if (pg_file_exists(full))
return full;
pfree(full);
}
@@ -465,7 +448,7 @@ expand_dynamic_library_name(const char *name)
{
full = substitute_libpath_macro(new);
pfree(new);
- if (file_exists(full))
+ if (pg_file_exists(full))
return full;
pfree(full);
}
@@ -582,7 +565,7 @@ find_in_dynamic_libpath(const char *basename)
elog(DEBUG3, "find_in_dynamic_libpath: trying \"%s\"", full);
- if (file_exists(full))
+ if (pg_file_exists(full))
return full;
pfree(full);
--
2.43.0
From a68a6a865bf69083e83679fecc6f8c825c69af61 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:20:01 +0900
Subject: [PATCH v7 2/5] Add backend facility for injection points
This adds a set of routines allowing developers to attach, detach and
run custom code based on arbitrary code paths set with a centralized
macro called INJECTION_POINT(). Injection points are registered in a
shared hash table. Processes also use a local cache to over loading
callbacks more than necessary, cleaning up their cache if a callback has
found to be removed.
---
src/include/pg_config.h.in | 3 +
src/include/utils/injection_point.h | 37 ++
src/backend/storage/ipc/ipci.c | 3 +
src/backend/storage/lmgr/lwlocknames.txt | 1 +
.../utils/activity/wait_event_names.txt | 1 +
src/backend/utils/misc/Makefile | 1 +
src/backend/utils/misc/injection_point.c | 334 ++++++++++++++++++
src/backend/utils/misc/meson.build | 1 +
doc/src/sgml/installation.sgml | 30 ++
doc/src/sgml/xfunc.sgml | 54 +++
src/makefiles/meson.build | 1 +
configure | 34 ++
configure.ac | 7 +
meson.build | 1 +
meson_options.txt | 3 +
src/Makefile.global.in | 1 +
src/tools/pgindent/typedefs.list | 2 +
17 files changed, 514 insertions(+)
create mode 100644 src/include/utils/injection_point.h
create mode 100644 src/backend/utils/misc/injection_point.c
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 5f16918243..288bb9cb42 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -698,6 +698,9 @@
/* Define to build with ICU support. (--with-icu) */
#undef USE_ICU
+/* Define to 1 to build with injection points. (--enable-injection-points) */
+#undef USE_INJECTION_POINTS
+
/* Define to 1 to build with LDAP support. (--with-ldap) */
#undef USE_LDAP
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
new file mode 100644
index 0000000000..55524b568f
--- /dev/null
+++ b/src/include/utils/injection_point.h
@@ -0,0 +1,37 @@
+/*-------------------------------------------------------------------------
+ * injection_point.h
+ * Definitions related to injection points.
+ *
+ * Copyright (c) 2001-2024, PostgreSQL Global Development Group
+ *
+ * src/include/utils/injection_point.h
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef INJECTION_POINT_H
+#define INJECTION_POINT_H
+
+/*
+ * Injections points require --enable-injection-points.
+ */
+#ifdef USE_INJECTION_POINTS
+#define INJECTION_POINT(name) InjectionPointRun(name)
+#else
+#define INJECTION_POINT(name) ((void) name)
+#endif
+
+/*
+ * Typedef for callback function launched by an injection point.
+ */
+typedef void (*InjectionPointCallback) (const char *name);
+
+extern Size InjectionPointShmemSize(void);
+extern void InjectionPointShmemInit(void);
+
+extern void InjectionPointAttach(const char *name,
+ const char *library,
+ const char *function);
+extern void InjectionPointRun(const char *name);
+extern void InjectionPointDetach(const char *name);
+
+#endif /* INJECTION_POINT_H */
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index e5119ed55d..6b9dcd8057 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -50,6 +50,7 @@
#include "storage/sinvaladt.h"
#include "storage/spin.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/snapmgr.h"
#include "utils/wait_event.h"
@@ -149,6 +150,7 @@ CalculateShmemSize(int *num_semaphores)
size = add_size(size, AsyncShmemSize());
size = add_size(size, StatsShmemSize());
size = add_size(size, WaitEventExtensionShmemSize());
+ size = add_size(size, InjectionPointShmemSize());
#ifdef EXEC_BACKEND
size = add_size(size, ShmemBackendArraySize());
#endif
@@ -351,6 +353,7 @@ CreateOrAttachShmemStructs(void)
AsyncShmemInit();
StatsShmemInit();
WaitEventExtensionShmemInit();
+ InjectionPointShmemInit();
}
/*
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index d621f5507f..0a0f02e055 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -55,3 +55,4 @@ WrapLimitsVacuumLock 46
NotifyQueueTailLock 47
WaitEventExtensionLock 48
WALSummarizerLock 49
+InjectionPointLock 50
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 088eb977d4..5862eecf16 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -325,6 +325,7 @@ WrapLimitsVacuum "Waiting to update limits on transaction id and multixact consu
NotifyQueueTail "Waiting to update limit on <command>NOTIFY</command> message storage."
WaitEventExtension "Waiting to read or update custom wait events information for extensions."
WALSummarizer "Waiting to read or update WAL summarization state."
+InjectionPoint "Waiting to read or update information related to injection points."
XactBuffer "Waiting for I/O on a transaction status SLRU buffer."
CommitTsBuffer "Waiting for I/O on a commit timestamp SLRU buffer."
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index c2971c7678..d9f59785b9 100644
--- a/src/backend/utils/misc/Makefile
+++ b/src/backend/utils/misc/Makefile
@@ -21,6 +21,7 @@ OBJS = \
guc_funcs.o \
guc_tables.o \
help_config.o \
+ injection_point.o \
pg_config.o \
pg_controldata.o \
pg_rusage.o \
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
new file mode 100644
index 0000000000..205dcfa31d
--- /dev/null
+++ b/src/backend/utils/misc/injection_point.c
@@ -0,0 +1,334 @@
+/*-------------------------------------------------------------------------
+ *
+ * injection_point.c
+ * Routines to control and run injection points in the code.
+ *
+ * Injection points can be used to call arbitrary callbacks in specific
+ * places of the code, attaching callbacks that would be run in the code
+ * paths where a named injection point exists.
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/utils/misc/injection_point.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include <sys/stat.h>
+
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "port/pg_bitutils.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/hsearch.h"
+#include "utils/injection_point.h"
+#include "utils/memutils.h"
+
+#ifdef USE_INJECTION_POINTS
+
+/*
+ * Hash table for storing injection points.
+ *
+ * InjectionPointHash is used to find an injection point by name.
+ */
+static HTAB *InjectionPointHash; /* find points from names */
+
+/* Field sizes */
+#define INJ_NAME_MAXLEN 64
+#define INJ_LIB_MAXLEN 128
+#define INJ_FUNC_MAXLEN 128
+
+/* Single injection point stored in InjectionPointHash */
+typedef struct InjectionPointEntry
+{
+ char name[INJ_NAME_MAXLEN]; /* hash key */
+ char library[INJ_LIB_MAXLEN]; /* library */
+ char function[INJ_FUNC_MAXLEN]; /* function */
+} InjectionPointEntry;
+
+#define INJECTION_POINT_HASH_INIT_SIZE 16
+#define INJECTION_POINT_HASH_MAX_SIZE 128
+
+/*
+ * Local cache of injection callbacks already loaded, stored in
+ * TopMemoryContext.
+ */
+typedef struct InjectionPointCacheEntry
+{
+ char name[INJ_NAME_MAXLEN];
+ InjectionPointCallback callback;
+} InjectionPointCacheEntry;
+
+static HTAB *InjectionPointCache = NULL;
+
+/*
+ * injection_point_cache_add
+ *
+ * Add an injection to the local cache.
+ */
+static void
+injection_point_cache_add(const char *name,
+ InjectionPointCallback callback)
+{
+ InjectionPointCacheEntry *entry;
+ bool found;
+
+ /* If first time, initialize */
+ if (InjectionPointCache == NULL)
+ {
+ HASHCTL hash_ctl;
+
+ hash_ctl.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+ hash_ctl.entrysize = sizeof(InjectionPointCacheEntry);
+ hash_ctl.hcxt = TopMemoryContext;
+
+ InjectionPointCache = hash_create("InjectionPoint cache hash",
+ INJECTION_POINT_HASH_MAX_SIZE,
+ &hash_ctl,
+ HASH_ELEM | HASH_STRINGS | HASH_CONTEXT);
+ }
+
+ entry = (InjectionPointCacheEntry *)
+ hash_search(InjectionPointCache, name, HASH_ENTER, &found);
+
+ Assert(!found);
+ memcpy(entry->name, name, strlen(name));
+ entry->callback = callback;
+}
+
+/*
+ * injection_point_cache_remove
+ *
+ * Remove entry from the local cache. Note that this leaks a callback
+ * loaded but removed later on, which should have no consequence from
+ * a testing perspective.
+ */
+static void
+injection_point_cache_remove(const char *name)
+{
+ /* Leave if no cache */
+ if (InjectionPointCache == NULL)
+ return;
+
+ (void) hash_search(InjectionPointCache, name, HASH_REMOVE, NULL);
+}
+
+/*
+ * injection_point_cache_get
+ *
+ * Retrieve an injection point from the local cache, if any.
+ */
+static InjectionPointCallback
+injection_point_cache_get(const char *name)
+{
+ bool found;
+ InjectionPointCacheEntry *entry;
+
+ /* no callback if no cache yet */
+ if (InjectionPointCache == NULL)
+ return NULL;
+
+ entry = (InjectionPointCacheEntry *)
+ hash_search(InjectionPointCache, name, HASH_FIND, &found);
+
+ if (found)
+ return entry->callback;
+
+ return NULL;
+}
+#endif /* USE_INJECTION_POINTS */
+
+/*
+ * Return the space for dynamic shared hash table.
+ */
+Size
+InjectionPointShmemSize(void)
+{
+#ifdef USE_INJECTION_POINTS
+ Size sz = 0;
+
+ sz = add_size(sz, hash_estimate_size(INJECTION_POINT_HASH_MAX_SIZE,
+ sizeof(InjectionPointEntry)));
+ return sz;
+#else
+ return 0;
+#endif
+}
+
+/*
+ * Allocate shmem space for dynamic shared hash.
+ */
+void
+InjectionPointShmemInit(void)
+{
+#ifdef USE_INJECTION_POINTS
+ HASHCTL info;
+
+ /* key is a NULL-terminated string */
+ info.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+ info.entrysize = sizeof(InjectionPointEntry);
+ InjectionPointHash = ShmemInitHash("InjectionPoint hash",
+ INJECTION_POINT_HASH_INIT_SIZE,
+ INJECTION_POINT_HASH_MAX_SIZE,
+ &info,
+ HASH_ELEM | HASH_FIXED_SIZE | HASH_STRINGS);
+#endif
+}
+
+#ifdef USE_INJECTION_POINTS
+static bool
+file_exists(const char *name)
+{
+ struct stat st;
+
+ Assert(name != NULL);
+ if (stat(name, &st) == 0)
+ return !S_ISDIR(st.st_mode);
+ else if (!(errno == ENOENT || errno == ENOTDIR))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not access file \"%s\": %m", name)));
+ return false;
+}
+#endif
+
+/*
+ * Attach a new injection point.
+ */
+void
+InjectionPointAttach(const char *name,
+ const char *library,
+ const char *function)
+{
+#ifdef USE_INJECTION_POINTS
+ InjectionPointEntry *entry_by_name;
+ bool found;
+
+ if (strlen(name) >= INJ_NAME_MAXLEN)
+ elog(ERROR, "injection point name %s too long (maximum of %u)",
+ name, INJ_NAME_MAXLEN);
+ if (strlen(library) >= INJ_LIB_MAXLEN)
+ elog(ERROR, "injection point library %s too long (maximum of %u)",
+ library, INJ_LIB_MAXLEN);
+ if (strlen(function) >= INJ_FUNC_MAXLEN)
+ elog(ERROR, "injection point function %s too long (maximum of %u)",
+ function, INJ_FUNC_MAXLEN);
+
+ /*
+ * Allocate and register a new injection point. A new point should not
+ * exist. For testing purposes this should be fine.
+ */
+ LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+ entry_by_name = (InjectionPointEntry *)
+ hash_search(InjectionPointHash, name,
+ HASH_ENTER, &found);
+ if (found)
+ {
+ LWLockRelease(InjectionPointLock);
+ elog(ERROR, "injection point \"%s\" already defined", name);
+ }
+
+ /* Save the entry */
+ memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
+ entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
+ memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
+ entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
+ memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
+ entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
+
+ LWLockRelease(InjectionPointLock);
+
+#else
+ elog(ERROR, "injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Detach an existing injection point.
+ */
+void
+InjectionPointDetach(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+ bool found;
+
+ LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+ hash_search(InjectionPointHash, name, HASH_REMOVE, &found);
+ LWLockRelease(InjectionPointLock);
+
+ if (!found)
+ elog(ERROR, "injection point \"%s\" not found", name);
+
+#else
+ elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Execute an injection point, if defined.
+ *
+ * Check first the shared hash table, and adapt the local cache depending
+ * on that as it could be possible that an entry to run has been removed.
+ */
+void
+InjectionPointRun(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+ InjectionPointEntry *entry_by_name;
+ bool found;
+ InjectionPointCallback injection_callback;
+
+ LWLockAcquire(InjectionPointLock, LW_SHARED);
+ entry_by_name = (InjectionPointEntry *)
+ hash_search(InjectionPointHash, name,
+ HASH_FIND, &found);
+ LWLockRelease(InjectionPointLock);
+
+ /*
+ * If not found, do nothing and remove it from the local cache if it
+ * existed there.
+ */
+ if (!found)
+ {
+ injection_point_cache_remove(name);
+ return;
+ }
+
+ /*
+ * Check if the callback exists in the local cache, to avoid unnecessary
+ * external loads.
+ */
+ injection_callback = injection_point_cache_get(name);
+ if (injection_callback == NULL)
+ {
+ char path[MAXPGPATH];
+
+ /* Not found in local cache, so load and register */
+ snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+ entry_by_name->library, DLSUFFIX);
+
+ if (!file_exists(path))
+ elog(ERROR, "could not find library \"%s\" for injection point \"%s\"",
+ path, name);
+
+ injection_callback = (InjectionPointCallback)
+ load_external_function(path, entry_by_name->function, true, NULL);
+
+ if (injection_callback == NULL)
+ elog(ERROR, "could not find function \"%s\" in library \"%s\" for injection point \"%s\"",
+ name, entry_by_name->function, path);
+
+ /* add it to the local cache when found */
+ injection_point_cache_add(name, injection_callback);
+ }
+
+ injection_callback(name);
+#else
+ elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
diff --git a/src/backend/utils/misc/meson.build b/src/backend/utils/misc/meson.build
index 581724f254..6669502205 100644
--- a/src/backend/utils/misc/meson.build
+++ b/src/backend/utils/misc/meson.build
@@ -6,6 +6,7 @@ backend_sources += files(
'guc_funcs.c',
'guc_tables.c',
'help_config.c',
+ 'injection_point.c',
'pg_config.c',
'pg_controldata.c',
'pg_rusage.c',
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index bb55695300..51830fc078 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -1656,6 +1656,21 @@ build-postgresql:
</listitem>
</varlistentry>
+ <varlistentry id="configure-option-enable-injection-points">
+ <term><option>--enable-injection-points</option></term>
+ <listitem>
+ <para>
+ Compiles <productname>PostgreSQL</productname> with support for
+ injection points in the server. This is valuable to inject
+ user-defined code to force specific conditions to happen on the
+ server in pre-defined code paths. This option is disabled by default.
+ See <xref linkend="xfunc-addin-injection-points"/> for more details.
+ This option is only for developers to test specific concurrency
+ scenarios.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="configure-option-with-segsize-blocks">
<term><option>--with-segsize-blocks=SEGSIZE_BLOCKS</option></term>
<listitem>
@@ -3160,6 +3175,21 @@ ninja install
</listitem>
</varlistentry>
+ <varlistentry id="configure-injection-points-meson">
+ <term><option>-Dinjection_points={ true | false }</option></term>
+ <listitem>
+ <para>
+ Compiles <productname>PostgreSQL</productname> with support for
+ injection points in the server. This is valuable to inject
+ user-defined code to force specific conditions to happen on the
+ server in pre-defined code paths. This option is disabled by default.
+ See <xref linkend="xfunc-addin-injection-points"/> for more details.
+ This option is only for developers to test specific concurrency
+ scenarios.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="configure-segsize-blocks-meson">
<term><option>-Dsegsize_blocks=SEGSIZE_BLOCKS</option></term>
<listitem>
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 89116ae74c..9dd9eafd22 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3510,6 +3510,60 @@ uint32 WaitEventExtensionNew(const char *wait_event_name)
</para>
</sect2>
+ <sect2 id="xfunc-addin-injection-points">
+ <title>Injection Points</title>
+
+ <para>
+ Add-ins can define injection points, that would run user-defined
+ code when going through a specific code path. This requires the use
+ of the following macro:
+<programlisting>
+INJECTION_POINT(name);
+</programlisting>
+ </para>
+
+ <para>
+ Callbacks can be attached to an injection point by calling:
+<programlisting>
+extern void InjectionPointAttach(const char *name,
+ const char *library,
+ const char *function);
+</programlisting>
+
+ <literal>name</literal> is the name of the injection point, that
+ will execute the <literal>function</literal> loaded from
+ <literal>library</literal>.
+ Injection points are saved in a hash table in shared memory, and
+ last until the server is shut down.
+ </para>
+
+ <para>
+ Here is an example of callback for
+ <literal>InjectionPointCallback</literal>:
+<programlisting>
+static void
+custom_injection_callback(const char *name)
+{
+ elog(NOTICE, "%s: executed custom callback", name);
+}
+</programlisting>
+ </para>
+
+ <para>
+ Optionally, it is possible to detach an injection point by calling:
+<programlisting>
+extern void InjectionPointDetach(const char *name);
+</programlisting>
+ </para>
+
+ <para>
+ Enabling injections points requires
+ <option>--enable-injection-points</option> with
+ <command>configure</command> or <option>-Dinjection_points=true</option>
+ with <application>Meson</application>.
+ </para>
+ </sect2>
+
<sect2 id="extend-cpp">
<title>Using C++ for Extensibility</title>
diff --git a/src/makefiles/meson.build b/src/makefiles/meson.build
index 034b26efa5..b0f4178b3d 100644
--- a/src/makefiles/meson.build
+++ b/src/makefiles/meson.build
@@ -54,6 +54,7 @@ pgxs_kv = {
'enable_rpath': get_option('rpath') ? 'yes' : 'no',
'enable_nls': libintl.found() ? 'yes' : 'no',
+ 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
'enable_tap_tests': tap_tests_enabled ? 'yes' : 'no',
'enable_debug': get_option('debug') ? 'yes' : 'no',
'enable_coverage': 'no',
diff --git a/configure b/configure
index 819ca26a7a..70a1968003 100755
--- a/configure
+++ b/configure
@@ -759,6 +759,7 @@ CPPFLAGS
LDFLAGS
CFLAGS
CC
+enable_injection_points
enable_tap_tests
enable_dtrace
DTRACEFLAGS
@@ -839,6 +840,7 @@ enable_profiling
enable_coverage
enable_dtrace
enable_tap_tests
+enable_injection_points
with_blocksize
with_segsize
with_segsize_blocks
@@ -1532,6 +1534,8 @@ Optional Features:
--enable-coverage build with coverage testing instrumentation
--enable-dtrace build with DTrace support
--enable-tap-tests enable TAP tests (requires Perl and IPC::Run)
+ --enable-injection-points
+ enable injection points (for testing)
--enable-depend turn on automatic dependency tracking
--enable-cassert enable assertion checks (for debugging)
--disable-largefile omit support for large files
@@ -3682,6 +3686,36 @@ fi
+#
+# Injection points
+#
+
+
+# Check whether --enable-injection-points was given.
+if test "${enable_injection_points+set}" = set; then :
+ enableval=$enable_injection_points;
+ case $enableval in
+ yes)
+
+$as_echo "#define USE_INJECTION_POINTS 1" >>confdefs.h
+
+ ;;
+ no)
+ :
+ ;;
+ *)
+ as_fn_error $? "no argument expected for --enable-injection-points option" "$LINENO" 5
+ ;;
+ esac
+
+else
+ enable_injection_points=no
+
+fi
+
+
+
+
#
# Block size
#
diff --git a/configure.ac b/configure.ac
index 5bf3c82cf5..52fd7af446 100644
--- a/configure.ac
+++ b/configure.ac
@@ -250,6 +250,13 @@ PGAC_ARG_BOOL(enable, tap-tests, no,
[enable TAP tests (requires Perl and IPC::Run)])
AC_SUBST(enable_tap_tests)
+#
+# Injection points
+#
+PGAC_ARG_BOOL(enable, injection-points, no, [enable injection points (for testing)],
+ [AC_DEFINE([USE_INJECTION_POINTS], 1, [Define to 1 to build with injection points. (--enable-injection-points)])])
+AC_SUBST(enable_injection_points)
+
#
# Block size
#
diff --git a/meson.build b/meson.build
index 57f9735feb..a67140a239 100644
--- a/meson.build
+++ b/meson.build
@@ -431,6 +431,7 @@ meson_bin = find_program(meson_binpath, native: true)
###############################################################
cdata.set('USE_ASSERT_CHECKING', get_option('cassert') ? 1 : false)
+cdata.set('USE_INJECTION_POINTS', get_option('injection_points') ? 1 : false)
blocksize = get_option('blocksize').to_int() * 1024
diff --git a/meson_options.txt b/meson_options.txt
index ee5d60b36e..249ecc5ffd 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -43,6 +43,9 @@ option('cassert', type: 'boolean', value: false,
option('tap_tests', type: 'feature', value: 'auto',
description: 'Enable TAP tests')
+option('injection_points', type: 'boolean', value: false,
+ description: 'Enable injection points')
+
option('PG_TEST_EXTRA', type: 'string', value: '',
description: 'Enable selected extra tests')
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index f8e461cbad..6f7de20527 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -203,6 +203,7 @@ enable_nls = @enable_nls@
enable_debug = @enable_debug@
enable_dtrace = @enable_dtrace@
enable_coverage = @enable_coverage@
+enable_injection_points = @enable_injection_points@
enable_tap_tests = @enable_tap_tests@
python_includespec = @python_includespec@
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 5fd46b7bd1..575604d2a7 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1145,6 +1145,8 @@ IdentLine
IdentifierLookup
IdentifySystemCmd
IfStackElem
+InjectionPointCacheEntry
+InjectionPointEntry
ImportForeignSchemaStmt
ImportForeignSchemaType
ImportForeignSchema_function
--
2.43.0
From 1ffd6e1f8c20d0f9d0d28c85fc70128599ee3f5c Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:25:43 +0900
Subject: [PATCH v7 3/5] Add test module injection_points
This is a test facility aimed at providing basic coverage for the code
routines of injection points, while providing some callbacks that can be
used for other tests. This will be extended with more tests.
---
src/test/modules/Makefile | 7 ++
src/test/modules/injection_points/.gitignore | 4 +
src/test/modules/injection_points/Makefile | 22 ++++
.../expected/injection_points.out | 118 ++++++++++++++++++
.../injection_points--1.0.sql | 36 ++++++
.../injection_points/injection_points.c | 95 ++++++++++++++
.../injection_points/injection_points.control | 4 +
src/test/modules/injection_points/meson.build | 37 ++++++
.../injection_points/sql/injection_points.sql | 34 +++++
src/test/modules/meson.build | 1 +
10 files changed, 358 insertions(+)
create mode 100644 src/test/modules/injection_points/.gitignore
create mode 100644 src/test/modules/injection_points/Makefile
create mode 100644 src/test/modules/injection_points/expected/injection_points.out
create mode 100644 src/test/modules/injection_points/injection_points--1.0.sql
create mode 100644 src/test/modules/injection_points/injection_points.c
create mode 100644 src/test/modules/injection_points/injection_points.control
create mode 100644 src/test/modules/injection_points/meson.build
create mode 100644 src/test/modules/injection_points/sql/injection_points.sql
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5d33fa6a9a..5bb7f848fe 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -37,6 +37,13 @@ SUBDIRS = \
worker_spi \
xid_wraparound
+
+ifeq ($(enable_injection_points),yes)
+SUBDIRS += injection_points
+else
+ALWAYS_SUBDIRS += injection_points
+endif
+
ifeq ($(with_ssl),openssl)
SUBDIRS += ssl_passphrase_callback
else
diff --git a/src/test/modules/injection_points/.gitignore b/src/test/modules/injection_points/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/injection_points/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
new file mode 100644
index 0000000000..fdd3b8e6da
--- /dev/null
+++ b/src/test/modules/injection_points/Makefile
@@ -0,0 +1,22 @@
+# src/test/modules/injection_points/Makefile
+
+MODULE_big = injection_points
+OBJS = \
+ $(WIN32RES) \
+ injection_points.o
+PGFILEDESC = "injection_points - facility for injection points"
+
+EXTENSION = injection_points
+DATA = injection_points--1.0.sql
+REGRESS = injection_points
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/injection_points
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/injection_points/expected/injection_points.out b/src/test/modules/injection_points/expected/injection_points.out
new file mode 100644
index 0000000000..2566af5359
--- /dev/null
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -0,0 +1,118 @@
+CREATE EXTENSION injection_points;
+SELECT injection_points_attach('TestInjectionBooh', 'booh');
+ERROR: incorrect mode "booh" for injection point creation
+SELECT injection_points_attach('TestInjectionError', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_attach('TestInjectionLog', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_attach('TestInjectionLog2', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionBooh'); -- nothing
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionError'); -- error
+ERROR: error triggered for injection point TestInjectionError
+-- Re-load cache and run again.
+\c
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionError'); -- error
+ERROR: error triggered for injection point TestInjectionError
+-- Remove one entry and check the remaining entries.
+SELECT injection_points_detach('TestInjectionError'); -- ok
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionError'); -- nothing
+ injection_points_run
+----------------------
+
+(1 row)
+
+-- More entries removed, letting TestInjectionLog2 to check the same
+-- callback used in more than one point.
+SELECT injection_points_detach('TestInjectionLog'); -- ok
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog'); -- nothing
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionError'); -- nothing
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_detach('TestInjectionLog'); -- fails
+ERROR: injection point "TestInjectionLog" not found
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ injection_points_run
+----------------------
+
+(1 row)
+
+DROP EXTENSION injection_points;
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
new file mode 100644
index 0000000000..64ff69bd7f
--- /dev/null
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -0,0 +1,36 @@
+/* src/test/modules/injection_points/injection_points--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION injection_points" to load this file. \quit
+
+--
+-- injection_points_attach()
+--
+-- Attaches an injection point using callbacks from one of the predefined
+-- modes.
+--
+CREATE FUNCTION injection_points_attach(IN point_name TEXT,
+ IN mode text)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_attach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- injection_points_run()
+--
+-- Executes an injection point.
+--
+CREATE FUNCTION injection_points_run(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_run'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- injection_points_detach()
+--
+-- Detaches an injection point.
+--
+CREATE FUNCTION injection_points_detach(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_detach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
new file mode 100644
index 0000000000..47e7522faf
--- /dev/null
+++ b/src/test/modules/injection_points/injection_points.c
@@ -0,0 +1,95 @@
+/*--------------------------------------------------------------------------
+ *
+ * injection_points.c
+ * Code for testing injection points.
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ * src/test/modules/injection_points/injection_points.c
+ *
+ * Injection points are able to trigger user-defined callbacks in pre-defined
+ * code paths.
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/builtins.h"
+#include "utils/injection_point.h"
+#include "utils/wait_event.h"
+
+PG_MODULE_MAGIC;
+
+extern PGDLLEXPORT void injection_error(const char *name);
+extern PGDLLEXPORT void injection_notice(const char *name);
+
+
+/* Set of callbacks available at point creation */
+void
+injection_error(const char *name)
+{
+ elog(ERROR, "error triggered for injection point %s", name);
+}
+
+void
+injection_notice(const char *name)
+{
+ elog(NOTICE, "notice triggered for injection point %s", name);
+}
+
+/*
+ * SQL function for creating an injection point.
+ */
+PG_FUNCTION_INFO_V1(injection_points_attach);
+Datum
+injection_points_attach(PG_FUNCTION_ARGS)
+{
+ char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ char *mode = text_to_cstring(PG_GETARG_TEXT_PP(1));
+ char *function;
+
+ if (strcmp(mode, "error") == 0)
+ function = "injection_error";
+ else if (strcmp(mode, "notice") == 0)
+ function = "injection_notice";
+ else
+ elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
+
+ InjectionPointAttach(name, "injection_points", function);
+
+ PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for triggering an injection point.
+ */
+PG_FUNCTION_INFO_V1(injection_points_run);
+Datum
+injection_points_run(PG_FUNCTION_ARGS)
+{
+ char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+ INJECTION_POINT(name);
+
+ PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for dropping an injection point.
+ */
+PG_FUNCTION_INFO_V1(injection_points_detach);
+Datum
+injection_points_detach(PG_FUNCTION_ARGS)
+{
+ char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+ InjectionPointDetach(name);
+
+ PG_RETURN_VOID();
+}
diff --git a/src/test/modules/injection_points/injection_points.control b/src/test/modules/injection_points/injection_points.control
new file mode 100644
index 0000000000..b4214f3bdc
--- /dev/null
+++ b/src/test/modules/injection_points/injection_points.control
@@ -0,0 +1,4 @@
+comment = 'Test code for injection points'
+default_version = '1.0'
+module_pathname = '$libdir/injection_points'
+relocatable = true
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
new file mode 100644
index 0000000000..ee37573f2a
--- /dev/null
+++ b/src/test/modules/injection_points/meson.build
@@ -0,0 +1,37 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+injection_points_sources = files(
+ 'injection_points.c',
+)
+
+if host_system == 'windows'
+ injection_points_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+ '--NAME', 'injection_points',
+ '--FILEDESC', 'injection_points - facility for injection points',])
+endif
+
+injection_points = shared_module('injection_points',
+ injection_points_sources,
+ kwargs: pg_test_mod_args,
+)
+test_install_libs += injection_points
+
+test_install_data += files(
+ 'injection_points.control',
+ 'injection_points--1.0.sql',
+)
+
+tests += {
+ 'name': 'injection_points',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'injection_points',
+ ],
+ },
+}
diff --git a/src/test/modules/injection_points/sql/injection_points.sql b/src/test/modules/injection_points/sql/injection_points.sql
new file mode 100644
index 0000000000..23c7e435ad
--- /dev/null
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -0,0 +1,34 @@
+CREATE EXTENSION injection_points;
+
+SELECT injection_points_attach('TestInjectionBooh', 'booh');
+SELECT injection_points_attach('TestInjectionError', 'error');
+SELECT injection_points_attach('TestInjectionLog', 'notice');
+SELECT injection_points_attach('TestInjectionLog2', 'notice');
+
+SELECT injection_points_run('TestInjectionBooh'); -- nothing
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+SELECT injection_points_run('TestInjectionLog'); -- notice
+SELECT injection_points_run('TestInjectionError'); -- error
+
+-- Re-load cache and run again.
+\c
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+SELECT injection_points_run('TestInjectionLog'); -- notice
+SELECT injection_points_run('TestInjectionError'); -- error
+
+-- Remove one entry and check the remaining entries.
+SELECT injection_points_detach('TestInjectionError'); -- ok
+SELECT injection_points_run('TestInjectionLog'); -- notice
+SELECT injection_points_run('TestInjectionError'); -- nothing
+-- More entries removed, letting TestInjectionLog2 to check the same
+-- callback used in more than one point.
+SELECT injection_points_detach('TestInjectionLog'); -- ok
+SELECT injection_points_run('TestInjectionLog'); -- nothing
+SELECT injection_points_run('TestInjectionError'); -- nothing
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+
+SELECT injection_points_detach('TestInjectionLog'); -- fails
+
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+
+DROP EXTENSION injection_points;
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 00ff1d77d1..ef7bd62c85 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -5,6 +5,7 @@ subdir('commit_ts')
subdir('delay_execution')
subdir('dummy_index_am')
subdir('dummy_seclabel')
+subdir('injection_points')
subdir('ldap_password_func')
subdir('libpq_pipeline')
subdir('plsample')
--
2.43.0
From b2dd7f5b0f19513132ffe8cfc2a17310d56b2648 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:30:56 +0900
Subject: [PATCH v7 4/5] Add regression test to show snapbuild consistency
Reverting 409f9ca44713 causes the test to fail. The test added here
relies on the existing callbacks in injection_points.
---
src/backend/replication/logical/snapbuild.c | 3 ++
src/test/recovery/Makefile | 7 ++-
src/test/recovery/meson.build | 4 ++
src/test/recovery/t/040_snapshot_status.pl | 51 +++++++++++++++++++++
4 files changed, 64 insertions(+), 1 deletion(-)
create mode 100644 src/test/recovery/t/040_snapshot_status.pl
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index a0b7947d2f..93048afc2f 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -141,6 +141,7 @@
#include "storage/procarray.h"
#include "storage/standby.h"
#include "utils/builtins.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/snapmgr.h"
#include "utils/snapshot.h"
@@ -654,6 +655,8 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
snap->xcnt = newxcnt;
snap->xip = newxip;
+ INJECTION_POINT("SnapBuildInitialSnapshot");
+
return snap;
}
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index 17ee353735..f57baba5e8 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -9,12 +9,17 @@
#
#-------------------------------------------------------------------------
-EXTRA_INSTALL=contrib/pg_prewarm contrib/pg_stat_statements contrib/test_decoding
+EXTRA_INSTALL=contrib/pg_prewarm \
+ contrib/pg_stat_statements \
+ contrib/test_decoding \
+ src/test/modules/injection_points
subdir = src/test/recovery
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+export enable_injection_points enable_injection_points
+
# required for 017_shm.pl and 027_stream_regress.pl
REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
export REGRESS_SHLIB
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index 88fb0306f5..43d7411a79 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -6,6 +6,9 @@ tests += {
'bd': meson.current_build_dir(),
'tap': {
'test_kwargs': {'priority': 40}, # recovery tests are slow, start early
+ 'env': {
+ 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
+ },
'tests': [
't/001_stream_rep.pl',
't/002_archiving.pl',
@@ -45,6 +48,7 @@ tests += {
't/037_invalid_database.pl',
't/038_save_logical_slots_shutdown.pl',
't/039_end_of_wal.pl',
+ 't/040_snapshot_status.pl',
],
},
}
diff --git a/src/test/recovery/t/040_snapshot_status.pl b/src/test/recovery/t/040_snapshot_status.pl
new file mode 100644
index 0000000000..e77b138144
--- /dev/null
+++ b/src/test/recovery/t/040_snapshot_status.pl
@@ -0,0 +1,51 @@
+# Test consistent of initial snapshot data.
+
+# This requires a node with wal_level=logical combined with an injection
+# point that forces a failure when a snapshot is initially built with a
+# logical slot created.
+#
+# See bug https://postgr.es/m/CAFiTN-s0zA1Kj0ozGHwkYkHwa5U0zUE94RSc_g81WrpcETB5=w...@mail.gmail.com.
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('SnapBuildInitialSnapshot', 'error');");
+
+my $node_host = $node->host;
+my $node_port = $node->port;
+my $connstr_common = "host=$node_host port=$node_port";
+my $connstr_db = "$connstr_common replication=database dbname=postgres";
+
+# This requires a single session, with two commands.
+my $psql_session =
+ $node->background_psql('postgres', on_error_stop => 0,
+ extra_params => [ '-d', $connstr_db ]);
+my ($output, $ret) = $psql_session->query(
+ 'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+ok($ret != 0, "First CREATE_REPLICATION_SLOT fails on injected error");
+
+# Now remove the injected error and check that the second command works.
+$node->safe_psql('postgres',
+ "SELECT injection_points_detach('SnapBuildInitialSnapshot');");
+
+($output, $ret) = $psql_session->query(
+ 'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+ok(substr($output, 0, 4) eq 'slot',
+ "Second CREATE_REPLICATION_SLOT passes");
+
+done_testing();
--
2.43.0
From 968e591966c72e316f4b1322deb6bfabea43a99e Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:36:53 +0900
Subject: [PATCH v7 5/5] Add basic test for promotion and restart race
condition
This test fails after 7863ee4def65 is reverted. test_injection_points
is extended so as it is possible to add condition variables to wait for
in the point callbacks, with a SQL function to broadcast condition
variables that may be sleeping.
I guess that this should be extended so as there is more than one
condition variable stored in shmem for this module, controlling which
variable to wait for directly in the callback itself, but that's not
really necessary now.
---
src/backend/access/transam/xlog.c | 7 +
.../injection_points--1.0.sql | 10 ++
.../injection_points/injection_points.c | 69 +++++++++
src/test/recovery/meson.build | 1 +
.../t/041_invalid_checkpoint_after_promote.pl | 138 ++++++++++++++++++
5 files changed, 225 insertions(+)
create mode 100644 src/test/recovery/t/041_invalid_checkpoint_after_promote.pl
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 478377c4a2..29e8f798f8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -100,6 +100,7 @@
#include "storage/sync.h"
#include "utils/guc_hooks.h"
#include "utils/guc_tables.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
#include "utils/relmapper.h"
@@ -7428,6 +7429,12 @@ CreateRestartPoint(int flags)
CheckPointGuts(lastCheckPoint.redo, flags);
+ /*
+ * This location is important to be after CheckPointGuts() to ensure
+ * that some work has happened.
+ */
+ INJECTION_POINT("CreateRestartPoint");
+
/*
* Remember the prior checkpoint's redo ptr for
* UpdateCheckPointDistanceEstimate()
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 64ff69bd7f..09ff14e24d 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -25,6 +25,16 @@ RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_run'
LANGUAGE C STRICT PARALLEL UNSAFE;
+--
+-- injection_points_wake()
+--
+-- Wakes a condition variable executed in an injection point.
+--
+CREATE FUNCTION injection_points_wake()
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_wake'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
--
-- injection_points_detach()
--
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 47e7522faf..599ced22a6 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -18,6 +18,7 @@
#include "postgres.h"
#include "fmgr.h"
+#include "storage/condition_variable.h"
#include "storage/lwlock.h"
#include "storage/shmem.h"
#include "utils/builtins.h"
@@ -26,10 +27,48 @@
PG_MODULE_MAGIC;
+/* Shared state information for injection points. */
+typedef struct TestInjectionPointSharedState
+{
+ /*
+ * Wait variable that can be registered at a given point, and that can be
+ * awakened via SQL.
+ */
+ ConditionVariable wait_point;
+} TestInjectionPointSharedState;
+
+/* Pointer to shared-memory state. */
+static TestInjectionPointSharedState *inj_state = NULL;
+
+/* Wait event when waiting on condition variable */
+static uint32 injection_wait_event = 0;
+
extern PGDLLEXPORT void injection_error(const char *name);
extern PGDLLEXPORT void injection_notice(const char *name);
+extern PGDLLEXPORT void injection_wait(const char *name);
+static void
+injection_init_shmem(void)
+{
+ bool found;
+
+ if (inj_state != NULL)
+ return;
+
+ LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+ inj_state = ShmemInitStruct("injection_points",
+ sizeof(TestInjectionPointSharedState),
+ &found);
+ if (!found)
+ {
+ /* First time through ... */
+ MemSet(inj_state, 0, sizeof(TestInjectionPointSharedState));
+ ConditionVariableInit(&inj_state->wait_point);
+ }
+ LWLockRelease(AddinShmemInitLock);
+}
+
/* Set of callbacks available at point creation */
void
injection_error(const char *name)
@@ -43,6 +82,20 @@ injection_notice(const char *name)
elog(NOTICE, "notice triggered for injection point %s", name);
}
+void
+injection_wait(const char *name)
+{
+ if (inj_state == NULL)
+ injection_init_shmem();
+ if (injection_wait_event == 0)
+ injection_wait_event = WaitEventExtensionNew("injection_wait");
+
+ /* And sleep.. */
+ ConditionVariablePrepareToSleep(&inj_state->wait_point);
+ ConditionVariableSleep(&inj_state->wait_point, injection_wait_event);
+ ConditionVariableCancelSleep();
+}
+
/*
* SQL function for creating an injection point.
*/
@@ -58,6 +111,8 @@ injection_points_attach(PG_FUNCTION_ARGS)
function = "injection_error";
else if (strcmp(mode, "notice") == 0)
function = "injection_notice";
+ else if (strcmp(mode, "wait") == 0)
+ function = "injection_wait";
else
elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
@@ -80,6 +135,20 @@ injection_points_run(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+/*
+ * SQL function for waking a condition variable.
+ */
+PG_FUNCTION_INFO_V1(injection_points_wake);
+Datum
+injection_points_wake(PG_FUNCTION_ARGS)
+{
+ if (inj_state == NULL)
+ injection_init_shmem();
+
+ ConditionVariableBroadcast(&inj_state->wait_point);
+ PG_RETURN_VOID();
+}
+
/*
* SQL function for dropping an injection point.
*/
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index 43d7411a79..c12b7fcf97 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -49,6 +49,7 @@ tests += {
't/038_save_logical_slots_shutdown.pl',
't/039_end_of_wal.pl',
't/040_snapshot_status.pl',
+ 't/041_invalid_checkpoint_after_promote.pl',
],
},
}
diff --git a/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl
new file mode 100644
index 0000000000..af4ab03604
--- /dev/null
+++ b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl
@@ -0,0 +1,138 @@
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep nanosleep);
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+# initialize primary node
+my $node_primary = PostgreSQL::Test::Cluster->new('master');
+$node_primary->init(allows_streaming => 1);
+$node_primary->append_conf(
+ 'postgresql.conf', q[
+checkpoint_timeout = 30s
+log_checkpoints = on
+restart_after_crash = on
+]);
+$node_primary->start;
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+
+# setup a standby
+my $node_standby = PostgreSQL::Test::Cluster->new('standby1');
+$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1);
+$node_standby->start;
+
+# dummy table for the upcoming tests.
+$node_primary->safe_psql('postgres', 'checkpoint');
+$node_primary->safe_psql('postgres', 'CREATE TABLE prim_tab (a int);');
+
+# Register a injection point on the standby so as the follow-up
+# restart point running on it will wait.
+$node_primary->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+# Wait until the extension has been created on the standby
+$node_primary->wait_for_replay_catchup($node_standby);
+# This causes a restartpoint to wait on a standby.
+$node_standby->safe_psql('postgres',
+ "SELECT injection_points_attach('CreateRestartPoint', 'wait');");
+
+# Execute a restart point on the standby, that will be waited on.
+# This needs to be in the background as we'll wait on it.
+my $logstart = -s $node_standby->logfile;
+my $psql_session =
+ $node_standby->background_psql('postgres', on_error_stop => 0);
+$psql_session->query_until(qr/starting_checkpoint/, q(
+ \echo starting_checkpoint
+ CHECKPOINT;
+));
+
+# Switch one WAL segment to make the restartpoint remove it.
+$node_primary->safe_psql('postgres', 'INSERT INTO prim_tab VALUES (1);');
+$node_primary->safe_psql('postgres', 'SELECT pg_switch_wal();');
+$node_primary->wait_for_replay_catchup($node_standby);
+
+# Wait until the checkpointer is in the middle of the restartpoint
+# processing.
+ok( $node_standby->poll_query_until(
+ 'postgres',
+ qq[SELECT count(*) FROM pg_stat_activity
+ WHERE backend_type = 'checkpointer' AND wait_event = 'injection_wait' ;],
+ '1'),
+ 'checkpointer is waiting at restart point'
+ ) or die "Timed out while waiting for checkpointer to run restartpoint";
+
+
+# Restartpoint should have started on standby.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+ $checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');
+
+# promote during restartpoint
+$node_primary->stop;
+$node_standby->promote;
+
+# Update the start position before waking up the checkpointer!
+$logstart = -s $node_standby->logfile;
+
+# Now wake up the checkpointer
+$node_standby->safe_psql('postgres',
+ "SELECT injection_points_wake();");
+
+# wait until checkpoint completes on the newly-promoted standby.
+my $checkpoint_complete = 0;
+for (my $i = 0; $i < 3000; $i++)
+{
+ my $log = slurp_file($node_standby->logfile, $logstart);
+ if ($log =~ m/restartpoint complete/)
+ {
+ $checkpoint_complete = 1;
+ last;
+ }
+ usleep(100_000);
+}
+is($checkpoint_complete, 1, 'restartpoint has completed');
+
+# kill SIGKILL a backend, and all backend will restart. Note that previous
+# checkpoint has not completed.
+my $psql_timeout = IPC::Run::timer(3600);
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+ [ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_standby->connstr('postgres') ],
+ '<',
+ \$killme_stdin,
+ '>',
+ \$killme_stdout,
+ '2>',
+ \$killme_stderr,
+ $psql_timeout);
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/;
+my $pid = $killme_stdout;
+chomp($pid);
+my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid);
+is($ret, 0, 'killed process with KILL');
+my $stdout;
+my $stderr;
+
+# After recovery, the server should be able to start.
+for (my $i = 0; $i < 30; $i++)
+{
+ ($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1');
+ last if $ret == 0;
+ sleep(1);
+}
+is($ret, 0, "psql connect success");
+is($stdout, 1, "psql select 1");
+
+done_testing();
--
2.43.0
signature.asc
Description: PGP signature
