On Wed, Jan 10, 2024 at 03:21:03PM +0530, Ashutosh Bapat wrote: > On Tue, Jan 9, 2024 at 10:09 AM Michael Paquier <mich...@paquier.xyz> wrote: >>> +#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. > > I agree with this change to jit. Without having search permissions on > every directory in the path, the function can not determine if the > file exists or not. So throwing an error is better than just returning > false which means that > the file does not exist.
I was looking at the original set of threads related to JIT, and this has been mentioned nowhere. I think that I'm going to give it a shot and see how the buildfarm reacts. If that finishes with red, we could always revert this part of the patch in jit.c still keep the refactored routine. >> 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. > > In practice the InjectionPointDetach() and InjectionPointAttach() > calls may not be close by and the user may not be able to figure out > why the injection points are behaviour weird. It may impact > performance but unexpected behaviour should be avoided, IMO. > > If nothing else this should be documented. In all the infrastructures I've looked at, folks did not really care about having an invalidation for the callbacks loaded. Still I'm OK to add something in the documentation about that, say among the lines of an extra sentence like: "The callbacks loaded by a process are cached within each process. There is no invalidation facility for the callbacks attached to injection points, hence updating a callback for an injection point requires a restart of the process to release its cache and the previous callbacks attached to it." > I am ok with not populating the cache but checking with just > load_external_function(). This is again another ease of use scenario > where a silly mistake by user is caught earlier making user's life > easier. That at least should be the goal of the first cut. I don't really aim for complicated here, just useful. > With v6 I could run the test when built with enable_injection_point > false. I just ran make check in that folder. Didn't test meson build. The CI has been failing because 041_invalid_checkpoint_after_promote was loading Time::HiRes::nanosleep and Windows does not support it. > Yeah, I think we have to use another shared state. If the waiting > backend moves ahead without test_injection_point_wake() being called, > that could lead to unexpected and very weird behaviour. > > It looks like ConditionVariable just remembers the processes that need > to be woken up during broadcast or signal. But by itself it doesn't > guarantee desired condition when woken up. Yeah, I'm not sure yet about how to do that in the most elegant way. But this part could always happen after 0001~0003. >> 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. > > Generally I think the 0001 and 0002 are in good shape. However, I > would like them to be more easy to use - like catching simple user > errors that can be easily caught. That saves a lot of frustration > because of unexpected behaviour. I will review 0001 and 0002 from v7 > in detail again, but it might take a few days. Thanks again for the reviews. I still intend to focus solely on 0001, 0002 and 0003 for the current commit fest to have something able to enforce error states in backends, at least. There have been quite a few bugs that could have coverage thanks for that. -- Michael
From 11c6e12750b2fc86c16a7ee5554e864fbc4438d3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 10:32:29 +0900
Subject: [PATCH v8 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 60035e582818dbc23c81458d7511846ecc597ab3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:20:01 +0900
Subject: [PATCH v8 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 f625473ad4..09aeb51bc8 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -329,6 +329,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."
#
# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
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 012287e17a6e19d88e0ffd6651acfa3ad2e8a906 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:25:43 +0900
Subject: [PATCH v8 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 22a5c58665c10002b1d7226de2d2bb0b1842afb5 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:30:56 +0900
Subject: [PATCH v8 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 3f934801bad3d65d466e431c813d14faf617a988 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 11 Jan 2024 13:10:52 +0900
Subject: [PATCH v8 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..886454d50c
--- /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);
+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
