On Wed, Nov 15, 2023 at 12:21:40PM +0100, Alvaro Herrera wrote: > On 2023-Nov-15, Michael Paquier wrote: > Oh, I think you're overthinking what my comment was. I was saying, just > name it "InjectionPointsHash". Since there appears to be no room for > another hash table for injection points, then there's no need to specify > that this one is the ByName hash. I couldn't think of any other way to > organize the injection points either.
Aha, OK. No problem, this was itching me as well but I didn't see an argument with changing these names, so I've renamed things a bit more. >> Yes, still not something that's required in the core APIs or an >> initial batch. > > I agree that we can do the easy thing first and build it up later. I > just hope we don't get too wedded on the current interface because of > lack of time in the current release that we get stuck with it. One thing that I assume we will need with more advanced testing is the possibility to pass down one (for a structure of data) or more arguments to the callback a point is attached to. For that, it is possible to add more macros, like INJECTION_POINT_1ARG, INJECTION_POINT_ARG(), etc. that use some (void *) pointers. It would be possible to add that even in stable branches, as need arises, changing the structure of the shmem hash table if required to control the way a callback is run. At the end, I suspect that it is one of these things where we'll need to adapt depending on what people want to do with this stuff. FWIW, I can do already quite a bit with this minimalistic design and an external extension. Custom wait events are also very handy to monitor a wait. >> I am not sure that it is a good idea to enforce a specific conditional >> logic in the backend core code. > > Agreed, let's get more experience on what other types of tests people > want to build, and how are things going to interact with each other. I've worked more on polishing the core facility today, with 0001 introducing the backend-side facility. One thing that I mentioned lacking is a local cache for processes so as they don't load an external callback more than once if run. So I have added this local cache. When a point is scanned but not found, a previous cache entry is cleared if any (this leaks but we don't have a way to unload stuff, and for testing that's not a big deal). I've renamed the routines to use attach and detach as terms, and adopted the name you've suggested for the macro. The names around the hash table and its entries have been changed to what you've suggested. You are right, that's more intuitive. 0002 is the test module for the basics, split into its own patch, with a couple of tests for the local cache part. 0003 and 0004 have been adjusted with the new SQL functions. At the end, I'd like to propose 0004 as it's been a PITA for me and I don't want to break this case again. It needs more work and can be cheaper. One more argument in favor of it is the addition of condition variables to wait and wake points (perhaps with even more variables?) in the test module. If there is interest for 0003, I'm OK to work more on it as well, but that's less important IMV. Thoughts and comments are welcome, with a v4 series attached. -- Michael
From 8febfda427ae773dd3c4c66ca9c932b3fe4cbc10 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 16 Nov 2023 14:41:41 +0900
Subject: [PATCH v4 1/4] 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 | 36 ++
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 | 349 ++++++++++++++++++
src/backend/utils/misc/meson.build | 1 +
doc/src/sgml/installation.sgml | 30 ++
doc/src/sgml/xfunc.sgml | 56 +++
configure | 34 ++
configure.ac | 7 +
meson.build | 1 +
meson_options.txt | 3 +
src/Makefile.global.in | 1 +
src/tools/pgindent/typedefs.list | 2 +
16 files changed, 529 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 d8a2985567..7a976821e5 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -701,6 +701,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..6335260fea
--- /dev/null
+++ b/src/include/utils/injection_point.h
@@ -0,0 +1,36 @@
+/*-------------------------------------------------------------------------
+ * injection_point.h
+ * Definitions related to injection points.
+ *
+ * Copyright (c) 2001-2023, 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 a3d8eacb8d..12b8a42fd9 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -48,6 +48,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"
@@ -143,6 +144,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
@@ -302,6 +304,7 @@ CreateSharedMemoryAndSemaphores(void)
AsyncShmemInit();
StatsShmemInit();
WaitEventExtensionShmemInit();
+ InjectionPointShmemInit();
#ifdef EXEC_BACKEND
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index f72f2906ce..42a048746d 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -54,3 +54,4 @@ XactTruncationLock 44
WrapLimitsVacuumLock 46
NotifyQueueTailLock 47
WaitEventExtensionLock 48
+InjectionPointLock 49
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index d7995931bd..5631d29138 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -319,6 +319,7 @@ XactTruncation "Waiting to execute <function>pg_xact_status</function> or update
WrapLimitsVacuum "Waiting to update limits on transaction id and multixact consumption."
NotifyQueueTail "Waiting to update limit on <command>NOTIFY</command> message storage."
WaitEventExtension "Waiting to read or update custom wait events information for extensions."
+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..31b84fe6ee
--- /dev/null
+++ b/src/backend/utils/misc/injection_point.c
@@ -0,0 +1,349 @@
+/*-------------------------------------------------------------------------
+ *
+ * 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, registering callbacks that would be run in the code
+ * paths where a named injection point exists.
+ *
+ * Portions Copyright (c) 1996-2023, 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
+
+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 InjectionPointArrayEntry
+{
+ char name[INJ_NAME_MAXLEN];
+ InjectionPointCallback callback;
+} InjectionPointArrayEntry;
+
+static InjectionPointArrayEntry *InjectionPointCacheArray = NULL;
+static int32 InjectionPointCacheArrayLen = 0; /* allocated length of above
+ * array */
+static int32 NextInjectionPointId = 0; /* number of entries used */
+
+/* utilities to handle the local array cache */
+static void
+injection_point_cache_add(const char *name,
+ InjectionPointCallback callback)
+{
+ /* If first time, initialize */
+ if (InjectionPointCacheArray == NULL)
+ {
+ InjectionPointCacheArray = (InjectionPointArrayEntry *)
+ MemoryContextAllocZero(TopMemoryContext,
+ 32 * sizeof(InjectionPointArrayEntry));
+ InjectionPointCacheArrayLen = 32;
+ NextInjectionPointId = 0;
+ }
+
+ /* If necessary, enlarge */
+ if (NextInjectionPointId >= InjectionPointCacheArrayLen)
+ {
+ int32 newlen = pg_nextpower2_32(InjectionPointCacheArrayLen + 1);
+
+ InjectionPointCacheArray = repalloc0_array(InjectionPointCacheArray,
+ InjectionPointArrayEntry,
+ InjectionPointCacheArrayLen,
+ newlen);
+ InjectionPointCacheArrayLen = newlen;
+ }
+
+ /* enough room has been made, so add it to the cache */
+ memcpy(InjectionPointCacheArray[NextInjectionPointId].name,
+ name, strlen(name));
+ InjectionPointCacheArray[NextInjectionPointId].callback = callback;
+ NextInjectionPointId++;
+}
+
+/*
+ * 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)
+{
+ int count = -1;
+
+ /* Leave if no cache */
+ if (InjectionPointCacheArray == NULL)
+ return;
+
+ /* find the entry we are looking for */
+ for (int i = 0; i < NextInjectionPointId; i++)
+ {
+ if (strcmp(name, InjectionPointCacheArray[i].name) == 0)
+ {
+ count = i;
+ break;
+ }
+ }
+
+ /* leave if no matching entry found */
+ if (count < 0)
+ return;
+ Assert(count < NextInjectionPointId);
+
+ /* now move entries one slot back */
+ for (int i = count; i < NextInjectionPointId - 1; i++)
+ {
+ memcpy(InjectionPointCacheArray[i].name,
+ InjectionPointCacheArray[i + 1].name,
+ INJ_NAME_MAXLEN);
+ InjectionPointCacheArray[i].callback =
+ InjectionPointCacheArray[i + 1].callback;
+ }
+
+ /* and reset the last entry */
+ memset(InjectionPointCacheArray[NextInjectionPointId].name,
+ 0, INJ_NAME_MAXLEN);
+ InjectionPointCacheArray[NextInjectionPointId].callback = NULL;
+ NextInjectionPointId--;
+}
+
+static InjectionPointCallback
+injection_point_cache_get(const char *name)
+{
+ /* no callback if no cache yet */
+ if (InjectionPointCacheArray == NULL)
+ return NULL;
+
+ for (int i = 0; i < NextInjectionPointId; i++)
+ {
+ if (strcmp(name, InjectionPointCacheArray[i].name) == 0)
+ return InjectionPointCacheArray[i].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 by name",
+ INJECTION_POINT_HASH_INIT_SIZE,
+ INJECTION_POINT_HASH_MAX_SIZE,
+ &info,
+ HASH_ELEM | 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", name);
+ if (strlen(library) >= INJ_LIB_MAXLEN)
+ elog(ERROR, "injection point library %s too long", library);
+ if (strlen(function) >= INJ_FUNC_MAXLEN)
+ elog(ERROR, "injection point function %s too long", function);
+
+ /*
+ * 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];
+
+ /* Found, so just run the callback registered */
+ snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+ entry_by_name->library, DLSUFFIX);
+
+ if (!file_exists(path))
+ elog(ERROR, "could not find injection library \"%s\"", path);
+
+ injection_callback = (InjectionPointCallback)
+ load_external_function(path, entry_by_name->function, true, NULL);
+
+ /* 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 f719c97c05..1438859b69 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 a3dc6eb855..d8840bce1f 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -1666,6 +1666,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>
@@ -3163,6 +3178,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..66cc94b03b 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3510,6 +3510,62 @@ 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 can register callbacks
+ to run user-defined code when going through a specific code path,
+ 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</library>.
+ 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>
+ Once an injection point is defined, running it requires to use
+ the following macro to trigger the callback given in a wanted code
+ path:
+<programlisting>
+INJECTION_POINT(name);
+</programlisting>
+ </para>
+
+ <para>
+ Optionally, it is possible to detach injection points by calling:
+<programlisting>
+extern void InjectionPointDetach(const char *name);
+</programlisting>
+ </para>
+
+ <para>
+ Enabling injections points requires
+ <option>--enable-injection-points</option> from
+ <command>configure</command> or <option>-Dinjection_points=true</option>
+ from <application>Meson</application>.
+ </para>
+ </sect2>
+
<sect2 id="extend-cpp">
<title>Using C++ for Extensibility</title>
diff --git a/configure b/configure
index c064115038..ce0ef0133d 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 f220b379b3..af13b6da69 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 47c8fcdc53..23f14de60e 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 d2f95cfec3..8d0d35636d 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 b3ca6392a6..ceb5895d1b 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 dba3498a13..a4acaa0666 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1137,6 +1137,8 @@ IdentLine
IdentifierLookup
IdentifySystemCmd
IfStackElem
+InjectionPointArrayEntry
+InjectionPointEntry
ImportForeignSchemaStmt
ImportForeignSchemaType
ImportForeignSchema_function
--
2.42.0
From 9e7120eb8f767478ec20e8bb93fca341fc3f2957 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 16 Nov 2023 14:00:48 +0900
Subject: [PATCH v4 2/4] Add test module test_injection_points
This is a test facility aimed at providing basic coverage for the code
routines of injection points. This will be extended with more tests.
---
src/test/modules/Makefile | 7 ++
src/test/modules/meson.build | 1 +
.../modules/test_injection_points/.gitignore | 4 +
.../modules/test_injection_points/Makefile | 22 ++++
.../expected/test_injection_points.out | 117 ++++++++++++++++++
.../modules/test_injection_points/meson.build | 37 ++++++
.../sql/test_injection_points.sql | 33 +++++
.../test_injection_points--1.0.sql | 36 ++++++
.../test_injection_points.c | 91 ++++++++++++++
.../test_injection_points.control | 4 +
10 files changed, 352 insertions(+)
create mode 100644 src/test/modules/test_injection_points/.gitignore
create mode 100644 src/test/modules/test_injection_points/Makefile
create mode 100644 src/test/modules/test_injection_points/expected/test_injection_points.out
create mode 100644 src/test/modules/test_injection_points/meson.build
create mode 100644 src/test/modules/test_injection_points/sql/test_injection_points.sql
create mode 100644 src/test/modules/test_injection_points/test_injection_points--1.0.sql
create mode 100644 src/test/modules/test_injection_points/test_injection_points.c
create mode 100644 src/test/modules/test_injection_points/test_injection_points.control
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index a18e4d28a0..7c4cefaedb 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -36,6 +36,13 @@ SUBDIRS = \
unsafe_tests \
worker_spi
+
+ifeq ($(enable_injection_points),yes)
+SUBDIRS += test_injection_points
+else
+ALWAYS_SUBDIRS += test_injection_points
+endif
+
ifeq ($(with_ssl),openssl)
SUBDIRS += ssl_passphrase_callback
else
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 4e83c0f8d7..1d6764e235 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -17,6 +17,7 @@ subdir('test_ddl_deparse')
subdir('test_dsa')
subdir('test_extensions')
subdir('test_ginpostinglist')
+subdir('test_injection_points')
subdir('test_integerset')
subdir('test_lfind')
subdir('test_misc')
diff --git a/src/test/modules/test_injection_points/.gitignore b/src/test/modules/test_injection_points/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_injection_points/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_injection_points/Makefile b/src/test/modules/test_injection_points/Makefile
new file mode 100644
index 0000000000..65bcdde782
--- /dev/null
+++ b/src/test/modules/test_injection_points/Makefile
@@ -0,0 +1,22 @@
+# src/test/modules/test_injection_points/Makefile
+
+MODULE_big = test_injection_points
+OBJS = \
+ $(WIN32RES) \
+ test_injection_points.o
+PGFILEDESC = "test_injection_points - test injection points"
+
+EXTENSION = test_injection_points
+DATA = test_injection_points--1.0.sql
+REGRESS = test_injection_points
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_injection_points
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_injection_points/expected/test_injection_points.out b/src/test/modules/test_injection_points/expected/test_injection_points.out
new file mode 100644
index 0000000000..a8ddae0aad
--- /dev/null
+++ b/src/test/modules/test_injection_points/expected/test_injection_points.out
@@ -0,0 +1,117 @@
+CREATE EXTENSION test_injection_points;
+SELECT test_injection_points_attach('TestInjectionBooh', 'booh');
+ERROR: incorrect mode "booh" for injection point creation
+SELECT test_injection_points_attach('TestInjectionError', 'error');
+ test_injection_points_attach
+------------------------------
+
+(1 row)
+
+SELECT test_injection_points_attach('TestInjectionLog', 'notice');
+ test_injection_points_attach
+------------------------------
+
+(1 row)
+
+SELECT test_injection_points_attach('TestInjectionLog2', 'notice');
+ test_injection_points_attach
+------------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionBooh'); -- nothing
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- error
+ERROR: error triggered for injection point TestInjectionError
+-- Re-load and run again.
+\c
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- error
+ERROR: error triggered for injection point TestInjectionError
+-- Remove one entry and check the other one.
+SELECT test_injection_points_detach('TestInjectionError'); -- ok
+ test_injection_points_detach
+------------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+-- All entries removed, nothing happens
+SELECT test_injection_points_detach('TestInjectionLog'); -- ok
+ test_injection_points_detach
+------------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- nothing
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+SELECT test_injection_points_detach('TestInjectionLog'); -- fails
+ERROR: injection point "TestInjectionLog" not found
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ test_injection_points_run
+---------------------------
+
+(1 row)
+
+DROP EXTENSION test_injection_points;
diff --git a/src/test/modules/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
new file mode 100644
index 0000000000..7509a102ef
--- /dev/null
+++ b/src/test/modules/test_injection_points/meson.build
@@ -0,0 +1,37 @@
+# Copyright (c) 2022-2023, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+test_injection_points_sources = files(
+ 'test_injection_points.c',
+)
+
+if host_system == 'windows'
+ test_injection_points_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+ '--NAME', 'test_injection_points',
+ '--FILEDESC', 'test_injection_points - test injection points',])
+endif
+
+test_injection_points = shared_module('test_injection_points',
+ test_injection_points_sources,
+ kwargs: pg_test_mod_args,
+)
+test_install_libs += test_injection_points
+
+test_install_data += files(
+ 'test_injection_points.control',
+ 'test_injection_points--1.0.sql',
+)
+
+tests += {
+ 'name': 'test_injection_points',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'test_injection_points',
+ ],
+ },
+}
diff --git a/src/test/modules/test_injection_points/sql/test_injection_points.sql b/src/test/modules/test_injection_points/sql/test_injection_points.sql
new file mode 100644
index 0000000000..8f23f4c044
--- /dev/null
+++ b/src/test/modules/test_injection_points/sql/test_injection_points.sql
@@ -0,0 +1,33 @@
+CREATE EXTENSION test_injection_points;
+
+SELECT test_injection_points_attach('TestInjectionBooh', 'booh');
+SELECT test_injection_points_attach('TestInjectionError', 'error');
+SELECT test_injection_points_attach('TestInjectionLog', 'notice');
+SELECT test_injection_points_attach('TestInjectionLog2', 'notice');
+
+SELECT test_injection_points_run('TestInjectionBooh'); -- nothing
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- error
+
+-- Re-load and run again.
+\c
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- error
+
+-- Remove one entry and check the other one.
+SELECT test_injection_points_detach('TestInjectionError'); -- ok
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+-- All entries removed, nothing happens
+SELECT test_injection_points_detach('TestInjectionLog'); -- ok
+SELECT test_injection_points_run('TestInjectionLog'); -- nothing
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+
+SELECT test_injection_points_detach('TestInjectionLog'); -- fails
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+
+DROP EXTENSION test_injection_points;
diff --git a/src/test/modules/test_injection_points/test_injection_points--1.0.sql b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
new file mode 100644
index 0000000000..1c0a689ae2
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
@@ -0,0 +1,36 @@
+/* src/test/modules/test_injection_points/test_injection_points--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_injection_points" to load this file. \quit
+
+--
+-- test_injection_points_attach()
+--
+-- Attaches an injection point using callbacks from one of the predefined
+-- modes.
+--
+CREATE FUNCTION test_injection_points_attach(IN point_name TEXT,
+ IN mode text)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_attach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- test_injection_points_run()
+--
+-- Executes an injection point.
+--
+CREATE FUNCTION test_injection_points_run(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_run'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- test_injection_points_detach()
+--
+-- Detaches an injection point.
+--
+CREATE FUNCTION test_injection_points_detach(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_detach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
diff --git a/src/test/modules/test_injection_points/test_injection_points.c b/src/test/modules/test_injection_points/test_injection_points.c
new file mode 100644
index 0000000000..efb2c74c47
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points.c
@@ -0,0 +1,91 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_injection_points.c
+ * Code for testing injection points.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_injection_points/test_injection_points.c
+ *
+ * Injection points are able to trigger user-defined callbacks in pre-defined
+ * code paths.
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "utils/builtins.h"
+#include "utils/injection_point.h"
+
+PG_MODULE_MAGIC;
+
+extern PGDLLEXPORT void test_injection_error(const char *name);
+extern PGDLLEXPORT void test_injection_notice(const char *name);
+
+/* Set of callbacks available at point creation */
+void
+test_injection_error(const char *name)
+{
+ elog(ERROR, "error triggered for injection point %s", name);
+}
+
+void
+test_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(test_injection_points_attach);
+Datum
+test_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 = "test_injection_error";
+ else if (strcmp(mode, "notice") == 0)
+ function = "test_injection_notice";
+ else
+ elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
+
+ InjectionPointAttach(name, "test_injection_points", function);
+
+ PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for triggering an injection point.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_run);
+Datum
+test_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(test_injection_points_detach);
+Datum
+test_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/test_injection_points/test_injection_points.control b/src/test/modules/test_injection_points/test_injection_points.control
new file mode 100644
index 0000000000..a13657cfc6
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points.control
@@ -0,0 +1,4 @@
+comment = 'Test code for injection points'
+default_version = '1.0'
+module_pathname = '$libdir/test_injection_points'
+relocatable = true
--
2.42.0
From 6601c25d6004ec348f107753e245aa22fac60123 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 16 Nov 2023 14:28:22 +0900
Subject: [PATCH v4 3/4] Add regression test to show snapbuild consistency
Reverting 409f9ca44713 causes the test to fail. The test added here
relies on the existing callbacks in test_injection_points.
---
src/backend/replication/logical/snapbuild.c | 3 ++
.../modules/test_injection_points/Makefile | 2 +
.../modules/test_injection_points/meson.build | 5 ++
.../t/001_snapshot_status.pl | 47 +++++++++++++++++++
4 files changed, 57 insertions(+)
create mode 100644 src/test/modules/test_injection_points/t/001_snapshot_status.pl
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index fec190a8b2..3491e5a872 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/modules/test_injection_points/Makefile b/src/test/modules/test_injection_points/Makefile
index 65bcdde782..4696c1b013 100644
--- a/src/test/modules/test_injection_points/Makefile
+++ b/src/test/modules/test_injection_points/Makefile
@@ -10,6 +10,8 @@ EXTENSION = test_injection_points
DATA = test_injection_points--1.0.sql
REGRESS = test_injection_points
+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
index 7509a102ef..6006b38f3d 100644
--- a/src/test/modules/test_injection_points/meson.build
+++ b/src/test/modules/test_injection_points/meson.build
@@ -34,4 +34,9 @@ tests += {
'test_injection_points',
],
},
+ 'tap': {
+ 'tests': [
+ 't/001_snapshot_status.pl',
+ ],
+ }
}
diff --git a/src/test/modules/test_injection_points/t/001_snapshot_status.pl b/src/test/modules/test_injection_points/t/001_snapshot_status.pl
new file mode 100644
index 0000000000..ca5c6cc7a4
--- /dev/null
+++ b/src/test/modules/test_injection_points/t/001_snapshot_status.pl
@@ -0,0 +1,47 @@
+# 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;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION test_injection_points;');
+$node->safe_psql('postgres',
+ "SELECT test_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 test_injection_points_detach('SnapBuildInitialSnapshot');");
+
+($output, $ret) = $psql_session->query(
+ 'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+print "BOO" . substr($output, 0, 4) . "\n";
+ok(substr($output, 0, 4) eq 'slot',
+ "Second CREATE_REPLICATION_SLOT passes");
+
+done_testing();
--
2.42.0
From a706447e46daa3fbb55538b9e96809c799f1b99a Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 16 Nov 2023 14:42:31 +0900
Subject: [PATCH v4 4/4] 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 +
.../modules/test_injection_points/meson.build | 1 +
.../t/002_invalid_checkpoint_after_promote.pl | 132 ++++++++++++++++++
.../test_injection_points--1.0.sql | 10 ++
.../test_injection_points.c | 73 ++++++++++
5 files changed, 223 insertions(+)
create mode 100644 src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1159dff1a6..d3828bcee3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -99,6 +99,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"
@@ -7329,6 +7330,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/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
index 6006b38f3d..6ebdc728b7 100644
--- a/src/test/modules/test_injection_points/meson.build
+++ b/src/test/modules/test_injection_points/meson.build
@@ -37,6 +37,7 @@ tests += {
'tap': {
'tests': [
't/001_snapshot_status.pl',
+ 't/002_invalid_checkpoint_after_promote.pl',
],
}
}
diff --git a/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl b/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl
new file mode 100644
index 0000000000..2da243e871
--- /dev/null
+++ b/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl
@@ -0,0 +1,132 @@
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep nanosleep);
+use Test::More;
+
+# 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 test_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 test_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 = 'test_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 test_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 will not start, and log PANIC: could not locate a valid checkpoint record
+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();
diff --git a/src/test/modules/test_injection_points/test_injection_points--1.0.sql b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
index 1c0a689ae2..05f97f0982 100644
--- a/src/test/modules/test_injection_points/test_injection_points--1.0.sql
+++ b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
@@ -25,6 +25,16 @@ RETURNS void
AS 'MODULE_PATHNAME', 'test_injection_points_run'
LANGUAGE C STRICT PARALLEL UNSAFE;
+--
+-- test_injection_points_wake()
+--
+-- Wakes a condition variable executed in an injection point.
+--
+CREATE FUNCTION test_injection_points_wake()
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_wake'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
--
-- test_injection_points_detach()
--
diff --git a/src/test/modules/test_injection_points/test_injection_points.c b/src/test/modules/test_injection_points/test_injection_points.c
index efb2c74c47..8b837d85d3 100644
--- a/src/test/modules/test_injection_points/test_injection_points.c
+++ b/src/test/modules/test_injection_points/test_injection_points.c
@@ -18,13 +18,56 @@
#include "postgres.h"
#include "fmgr.h"
+#include "storage/condition_variable.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;
+/* 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 test_injection_wait_event = 0;
+
extern PGDLLEXPORT void test_injection_error(const char *name);
extern PGDLLEXPORT void test_injection_notice(const char *name);
+extern PGDLLEXPORT void test_injection_wait(const char *name);
+
+
+static void
+test_injection_init_shmem(void)
+{
+ bool found;
+
+ if (inj_state != NULL)
+ return;
+
+ LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+ inj_state = ShmemInitStruct("test_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
@@ -39,6 +82,20 @@ test_injection_notice(const char *name)
elog(NOTICE, "notice triggered for injection point %s", name);
}
+void
+test_injection_wait(const char *name)
+{
+ if (inj_state == NULL)
+ test_injection_init_shmem();
+ if (test_injection_wait_event == 0)
+ test_injection_wait_event = WaitEventExtensionNew("test_injection_wait");
+
+ /* And sleep.. */
+ ConditionVariablePrepareToSleep(&inj_state->wait_point);
+ ConditionVariableSleep(&inj_state->wait_point, test_injection_wait_event);
+ ConditionVariableCancelSleep();
+}
+
/*
* SQL function for creating an injection point.
*/
@@ -54,6 +111,8 @@ test_injection_points_attach(PG_FUNCTION_ARGS)
function = "test_injection_error";
else if (strcmp(mode, "notice") == 0)
function = "test_injection_notice";
+ else if (strcmp(mode, "wait") == 0)
+ function = "test_injection_wait";
else
elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
@@ -76,6 +135,20 @@ test_injection_points_run(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+/*
+ * SQL function for waking a condition variable.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_wake);
+Datum
+test_injection_points_wake(PG_FUNCTION_ARGS)
+{
+ if (inj_state == NULL)
+ test_injection_init_shmem();
+
+ ConditionVariableBroadcast(&inj_state->wait_point);
+ PG_RETURN_VOID();
+}
+
/*
* SQL function for dropping an injection point.
*/
--
2.42.0
signature.asc
Description: PGP signature
