Subject: Re: Adding facility for injection points (or probe points?) for more advanced tests

From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 13:39:06 +0900

(Compiled two separate review emails into a single one)

On Tue, Jan 02, 2024 at 03:36:12PM +0530, Ashutosh Bapat wrote:
> This discussion has not been addressed in v6. I think the interface
> needs to be documented in the order below
> INJECTION_POINT - this declares an injection point - i.e. a place in
> code where an external code can be injected (and run).
> InjectionPointAttach() - this is used to associate ("attach") external
> code to an injection point.
> InjectionPointDetach() - this is used to disassociate ("detach")
> external code from an injection point.
>
> [arguments about doc organization]
>
> Even if an INJECTION_POINT is not "declared" attach would succeed but
> doesn't do anything. I think this needs to be made clear in the
> documentation. Better if we could somehow make Attach() fail if the
> specified injection point is not "declared" using INJECTION_POINT. Of
> course we don't want to bloat the hash table with all "declared"
> injection points even if they aren't being attached to and hence not
> used.

Okay, I can see your point.  I have reorganized the docs in the
following order:
- INJECTION_POINT
- Attach
- Detach

> I think, exposing the current injection point strings as
> #defines and encouraging users to use these macros instead of string
> literals will be a good start.

Nah, I disagree with this one actually.  It is easy to grep for the
macro INJECTION_POINT to be able to achieve the same research job, and
this would make the code more inconsistent when callbacks are run
within extensions which don't care about a #define in a backend
header.

> With the current implementation it's possible to "declare" injection
> point with same name at multiple places. It's useful but is it
> intended?

Yes.  I would recommend not doing that, but I don't see why there
would be a point in restricting that, either.

> /* Field sizes */
> #define INJ_NAME_MAXLEN 64
> #define INJ_LIB_MAXLEN 128
> #define INJ_FUNC_MAXLEN 128
> I think these limits should be either documented or specified in the
> error messages for users to fix their code in case of
> errors/unexpected behaviour.

Adding them to the error messages when attaching is a good idea.
Done.

> This is not an array entry anymore. I think we should rename
> InjectionPointEntry as SharedInjectionPointEntry and InjectionPointArrayEntry
> as LocalInjectionPointEntry.

Yep, fixed.

> +/* utilities to handle the local array cache */
> +static void
> +injection_point_cache_add(const char *name,
> + InjectionPointCallback callback)
> +{
> ... snip ...
> +
> + entry = (InjectionPointCacheEntry *)
> + hash_search(InjectionPointCache, name, HASH_ENTER, &found);
> +
> + if (!found)
> 
> The function is called only when the injection point is not found in the local
> cache. So this condition will always be true. An Assert will help to make it
> clear and also prevent an unintended callback replacement.

Right, as coded that seems pointless to make the found conditional.  I
think that I coded it this way when doing some earlier work with this
code, and finished with a simpler thing.

> +#ifdef USE_INJECTION_POINTS
> +static bool
> +file_exists(const char *name)
> 
> There's similar function in jit.c and dfmgr.c. Can we not reuse that code?

This has been mentioned in a different comment.  Refactored as of
0001, but there is something here related to EACCES for the JIT path.
Seems weird to me that we would not fail if the JIT library cannot be
accessed when stat() fails.

> + /* Save the entry */
> + memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
> + entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
> + memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
> + entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
> + memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
> + entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
> 
> Most of the code is using strncpy instead of memcpy. Why is this code different?

strncpy() is less used in the backend code.  It comes to a matter of
taste, IMO.

> + injection_callback = injection_point_cache_get(name);
> + if (injection_callback == NULL)
> + {
> + char path[MAXPGPATH];
> +
> + /* Found, so just run the callback registered */
> 
> The condition indicates that the callback was not found. Comment looks wrong.

Fixed.

> Consider case
> 
> Backend 2
> InjectionPointAttach("xyz", "abc", "pqr");
> 
> Backend 1
> INJECTION_POINT("xyz");
> 
> Backend 2
> InjectionPointDetach("xyz");
> InjectionPointAttach("xyz", "uvw", "lmn");
> 
> Backend 1
> INJECTION_POINT("xyz");
> 
> IIUC, the last INJECTION_POINT would run abc.pqr instead of uvw.lmn.
> Am I correct?

Yeah, that's an intended design choice to keep the code simpler and
faster as there is no need to track the library and function names in
the local caches or implement something similar to invalidation
messages for this facility because it would impact performance anyway
in the call paths.  In short, just don't do that, or use two distinct
points.

On Thu, Jan 04, 2024 at 06:22:35PM +0530, Ashutosh Bapat wrote:
> One more comment on 0001
> InjectionPointAttach() doesn't test whether the given function exists
> in the given library. Even if InjectionPointAttach() succeeds,
> INJECTION_POINT might throw error because the function doesn't exist.
> This can be seen as an unwanted behaviour. I think
> InjectionPointAttach() should test whether the function exists and
> possibly load it as well by adding it to the local cache.

This has the disadvantage of filling the local cache but that may not
be necessary with an extra load_external_function() in the attach
path.  I agree to make things safer, but I would do that when
attempting to run the callback instead.  Perhaps there's an argument
for the case of somebody replacing a library on-the-fly.  I don't
really buy it, but people like doing fancy things sometimes.

> 0002 comments
> --- /dev/null
> +++ b/src/test/modules/test_injection_points/expected/test_injection_points.out
> 
> When built without injection point support, this test fails. We should
> add an alternate output file for such a build so that the behaviour
> with and without injection point support is tested. Or set up things
> such that the test is not run under make check in that directory. I
> will prefer the first option.

src/test/modules/Makefile has a safeguard for ./configure, and there's
one in test_injection_points/meson.build for Meson.  The test is not
run when the switches are not used, rather than using an alternate
output file.  There was a different issue when moving the tests to
src/test/recovery/, though, where we need to make the execution of the
tests conditional on get_option('injection_points').

> +
> +SELECT test_injection_points_run('TestInjectionError'); -- error
> +ERROR: error triggered for injection point TestInjectionError
> +-- Re-load and run again.
> 
> What's getting Re-loaded here? \c will create a new connection and
> thus a new backend. Maybe the comment should say "test in a fresh
> backend" or something of that sort?

The local cache is reloaded.  Reworded.

> Looks confusing to me, we are testing the one removed as well. Am I
> missing something?
> [...]
> We aren't removing all entries TestInjectionLog2 is still there. Am I
> missing something?

Reworded all that.

> +# after recovery, the server will not start, and log PANIC: could not
> locate a valid checkpoint record
> 
> IIUC the comment describes the behaviour with 7863ee4def65 reverted.
> But the test after this comment is written for the behaviour with
> 7863ee4def65. That's confusing. Is the intent to describe both
> behaviours in the comment?

This came from the original test case posted on the thread that
treated this bug.  There's more that bugs me for this script that I
would like to polish.  Let's focus on 0001 and 0002 for now..

> According to the prologue of ConditionVariableSleep(), that function
> should be called in a loop checking for the desired condition. All the
> callers that I examined follow that pattern. I think we need to follow
> that pattern here as well.
> 
> Below comment from ConditionVariableTimedSleep() makes me think that
> the caller of ConditionVariableSleep() can be woken up even if the
> condition variable was not signaled. That's why the while() loop
> around ConditionVariableSleep().

That's the thing here, we don't have an extra condition to check
after.  The variable sleep is what triggers the stop.  :)
Perhaps this could be made smarter or with something else, I'm OK to
revisit that with the polishing for 0003 I'm planning.  We could use a
separate shared state, for example, but that does not improve the test
readability, either.

Attached is a v7 series.  What do you think?  0004 and 0005 for the
extra tests still need more discussion and much more polishing, IMO.
--
Michael
From 1aea179dd07e4551a676c3c0a3bbf2a1bffc9fec Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 10:32:29 +0900
Subject: [PATCH v7 1/5] Refactor code to check file file existence

jit.c and dfgr.c had a copy of the same code to check if a file exists
or not.  This refactored routine will be used by an upcoming patch.

Note that this adds a check on EACCES for the JIT path.
---
 src/include/storage/fd.h       |  1 +
 src/backend/jit/jit.c          | 20 +-------------------
 src/backend/storage/file/fd.c  | 22 ++++++++++++++++++++++
 src/backend/utils/fmgr/dfmgr.c | 25 ++++---------------------
 4 files changed, 28 insertions(+), 40 deletions(-)

diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index c4c60bc0a8..60bba5c970 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -182,6 +182,7 @@ extern int	pg_fsync(int fd);
 extern int	pg_fsync_no_writethrough(int fd);
 extern int	pg_fsync_writethrough(int fd);
 extern int	pg_fdatasync(int fd);
+extern bool pg_file_exists(const char *fname);
 extern void pg_flush_data(int fd, off_t offset, off_t nbytes);
 extern int	pg_truncate(const char *path, off_t length);
 extern void fsync_fname(const char *fname, bool isdir);
diff --git a/src/backend/jit/jit.c b/src/backend/jit/jit.c
index 3f9848e726..d323c199ea 100644
--- a/src/backend/jit/jit.c
+++ b/src/backend/jit/jit.c
@@ -45,7 +45,6 @@ static bool provider_failed_loading = false;

 static bool provider_init(void);
-static bool file_exists(const char *name);

 /*
@@ -89,7 +88,7 @@ provider_init(void)
 	 */
 	snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path, jit_provider, DLSUFFIX);
 	elog(DEBUG1, "probing availability of JIT provider at %s", path);
-	if (!file_exists(path))
+	if (!pg_file_exists(path))
 	{
 		elog(DEBUG1,
 			 "provider not available, disabling JIT for current session");
@@ -188,20 +187,3 @@ InstrJitAgg(JitInstrumentation *dst, JitInstrumentation *add)
 	INSTR_TIME_ADD(dst->optimization_counter, add->optimization_counter);
 	INSTR_TIME_ADD(dst->emission_counter, add->emission_counter);
 }
-
-static bool
-file_exists(const char *name)
-{
-	struct stat st;
-
-	Assert(name != NULL);
-
-	if (stat(name, &st) == 0)
-		return !S_ISDIR(st.st_mode);
-	else if (!(errno == ENOENT || errno == ENOTDIR))
-		ereport(ERROR,
-				(errcode_for_file_access(),
-				 errmsg("could not access file \"%s\": %m", name)));
-
-	return false;
-}
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 8917c6004a..c3585ced34 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -493,6 +493,28 @@ retry:
 	return rc;
 }
 
+/*
+ * pg_file_exists -- check that a given file exists.
+ *
+ * This requires an absolute path to the file.
+ */
+bool
+pg_file_exists(const char *name)
+{
+	struct stat st;
+
+	Assert(name != NULL);
+
+	if (stat(name, &st) == 0)
+		return !S_ISDIR(st.st_mode);
+	else if (!(errno == ENOENT || errno == ENOTDIR || errno == EACCES))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not access file \"%s\": %m", name)));
+
+	return false;
+}
+
 /*
  * pg_flush_data --- advise OS that the described dirty data should be flushed
  *
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 638eddf19f..eafa0128ef 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -33,6 +33,7 @@
 #include "fmgr.h"
 #include "lib/stringinfo.h"
 #include "miscadmin.h"
+#include "storage/fd.h"
 #include "storage/shmem.h"
 #include "utils/hsearch.h"
 
@@ -78,7 +79,6 @@ char	   *Dynamic_library_path;
 static void *internal_load_library(const char *libname);
 static void incompatible_module_error(const char *libname,
 									  const Pg_magic_struct *module_magic_data) pg_attribute_noreturn();
-static bool file_exists(const char *name);
 static char *expand_dynamic_library_name(const char *name);
 static void check_restricted_library_name(const char *name);
 static char *substitute_libpath_macro(const char *name);
@@ -400,23 +400,6 @@ incompatible_module_error(const char *libname,
 			 errdetail_internal("%s", details.data)));
 }
 
-static bool
-file_exists(const char *name)
-{
-	struct stat st;
-
-	Assert(name != NULL);
-
-	if (stat(name, &st) == 0)
-		return !S_ISDIR(st.st_mode);
-	else if (!(errno == ENOENT || errno == ENOTDIR || errno == EACCES))
-		ereport(ERROR,
-				(errcode_for_file_access(),
-				 errmsg("could not access file \"%s\": %m", name)));
-
-	return false;
-}
-
 
 /*
  * If name contains a slash, check if the file exists, if so return
@@ -447,7 +430,7 @@ expand_dynamic_library_name(const char *name)
 	else
 	{
 		full = substitute_libpath_macro(name);
-		if (file_exists(full))
+		if (pg_file_exists(full))
 			return full;
 		pfree(full);
 	}
@@ -465,7 +448,7 @@ expand_dynamic_library_name(const char *name)
 	{
 		full = substitute_libpath_macro(new);
 		pfree(new);
-		if (file_exists(full))
+		if (pg_file_exists(full))
 			return full;
 		pfree(full);
 	}
@@ -582,7 +565,7 @@ find_in_dynamic_libpath(const char *basename)
 
 		elog(DEBUG3, "find_in_dynamic_libpath: trying \"%s\"", full);
 
-		if (file_exists(full))
+		if (pg_file_exists(full))
 			return full;
 
 		pfree(full);
-- 
2.43.0

From a68a6a865bf69083e83679fecc6f8c825c69af61 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 13:20:01 +0900
Subject: [PATCH v7 2/5] Add backend facility for injection points

This adds a set of routines allowing developers to attach, detach and
run custom code based on arbitrary code paths set with a centralized
macro called INJECTION_POINT().  Injection points are registered in a
shared hash table.  Processes also use a local cache to over loading
callbacks more than necessary, cleaning up their cache if a callback has
found to be removed.
---
 src/include/pg_config.h.in                    |   3 +
 src/include/utils/injection_point.h           |  37 ++
 src/backend/storage/ipc/ipci.c                |   3 +
 src/backend/storage/lmgr/lwlocknames.txt      |   1 +
 .../utils/activity/wait_event_names.txt       |   1 +
 src/backend/utils/misc/Makefile               |   1 +
 src/backend/utils/misc/injection_point.c      | 334 ++++++++++++++++++
 src/backend/utils/misc/meson.build            |   1 +
 doc/src/sgml/installation.sgml                |  30 ++
 doc/src/sgml/xfunc.sgml                       |  54 +++
 src/makefiles/meson.build                     |   1 +
 configure                                     |  34 ++
 configure.ac                                  |   7 +
 meson.build                                   |   1 +
 meson_options.txt                             |   3 +
 src/Makefile.global.in                        |   1 +
 src/tools/pgindent/typedefs.list              |   2 +
 17 files changed, 514 insertions(+)
 create mode 100644 src/include/utils/injection_point.h
 create mode 100644 src/backend/utils/misc/injection_point.c

diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 5f16918243..288bb9cb42 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -698,6 +698,9 @@
 /* Define to build with ICU support. (--with-icu) */
 #undef USE_ICU
 
+/* Define to 1 to build with injection points. (--enable-injection-points) */
+#undef USE_INJECTION_POINTS
+
 /* Define to 1 to build with LDAP support. (--with-ldap) */
 #undef USE_LDAP
 
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
new file mode 100644
index 0000000000..55524b568f
--- /dev/null
+++ b/src/include/utils/injection_point.h
@@ -0,0 +1,37 @@
+/*-------------------------------------------------------------------------
+ * injection_point.h
+ *	  Definitions related to injection points.
+ *
+ * Copyright (c) 2001-2024, PostgreSQL Global Development Group
+ *
+ * src/include/utils/injection_point.h
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef INJECTION_POINT_H
+#define INJECTION_POINT_H
+
+/*
+ * Injections points require --enable-injection-points.
+ */
+#ifdef USE_INJECTION_POINTS
+#define INJECTION_POINT(name) InjectionPointRun(name)
+#else
+#define INJECTION_POINT(name) ((void) name)
+#endif
+
+/*
+ * Typedef for callback function launched by an injection point.
+ */
+typedef void (*InjectionPointCallback) (const char *name);
+
+extern Size InjectionPointShmemSize(void);
+extern void InjectionPointShmemInit(void);
+
+extern void InjectionPointAttach(const char *name,
+								 const char *library,
+								 const char *function);
+extern void InjectionPointRun(const char *name);
+extern void InjectionPointDetach(const char *name);
+
+#endif							/* INJECTION_POINT_H */
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index e5119ed55d..6b9dcd8057 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -50,6 +50,7 @@
 #include "storage/sinvaladt.h"
 #include "storage/spin.h"
 #include "utils/guc.h"
+#include "utils/injection_point.h"
 #include "utils/snapmgr.h"
 #include "utils/wait_event.h"
 
@@ -149,6 +150,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, AsyncShmemSize());
 	size = add_size(size, StatsShmemSize());
 	size = add_size(size, WaitEventExtensionShmemSize());
+	size = add_size(size, InjectionPointShmemSize());
 #ifdef EXEC_BACKEND
 	size = add_size(size, ShmemBackendArraySize());
 #endif
@@ -351,6 +353,7 @@ CreateOrAttachShmemStructs(void)
 	AsyncShmemInit();
 	StatsShmemInit();
 	WaitEventExtensionShmemInit();
+	InjectionPointShmemInit();
 }
 
 /*
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index d621f5507f..0a0f02e055 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -55,3 +55,4 @@ WrapLimitsVacuumLock				46
 NotifyQueueTailLock					47
 WaitEventExtensionLock				48
 WALSummarizerLock					49
+InjectionPointLock				50
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 088eb977d4..5862eecf16 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -325,6 +325,7 @@ WrapLimitsVacuum	"Waiting to update limits on transaction id and multixact consu
 NotifyQueueTail	"Waiting to update limit on <command>NOTIFY</command> message storage."
 WaitEventExtension	"Waiting to read or update custom wait events information for extensions."
 WALSummarizer	"Waiting to read or update WAL summarization state."
+InjectionPoint	"Waiting to read or update information related to injection points."
 
 XactBuffer	"Waiting for I/O on a transaction status SLRU buffer."
 CommitTsBuffer	"Waiting for I/O on a commit timestamp SLRU buffer."
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index c2971c7678..d9f59785b9 100644
--- a/src/backend/utils/misc/Makefile
+++ b/src/backend/utils/misc/Makefile
@@ -21,6 +21,7 @@ OBJS = \
 	guc_funcs.o \
 	guc_tables.o \
 	help_config.o \
+	injection_point.o \
 	pg_config.o \
 	pg_controldata.o \
 	pg_rusage.o \
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
new file mode 100644
index 0000000000..205dcfa31d
--- /dev/null
+++ b/src/backend/utils/misc/injection_point.c
@@ -0,0 +1,334 @@
+/*-------------------------------------------------------------------------
+ *
+ * injection_point.c
+ *	  Routines to control and run injection points in the code.
+ *
+ * Injection points can be used to call arbitrary callbacks in specific
+ * places of the code, attaching callbacks that would be run in the code
+ * paths where a named injection point exists.
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/backend/utils/misc/injection_point.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include <sys/stat.h>
+
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "port/pg_bitutils.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/hsearch.h"
+#include "utils/injection_point.h"
+#include "utils/memutils.h"
+
+#ifdef USE_INJECTION_POINTS
+
+/*
+ * Hash table for storing injection points.
+ *
+ * InjectionPointHash is used to find an injection point by name.
+ */
+static HTAB *InjectionPointHash;	/* find points from names */
+
+/* Field sizes */
+#define INJ_NAME_MAXLEN		64
+#define INJ_LIB_MAXLEN		128
+#define INJ_FUNC_MAXLEN		128
+
+/* Single injection point stored in InjectionPointHash */
+typedef struct InjectionPointEntry
+{
+	char		name[INJ_NAME_MAXLEN];	/* hash key */
+	char		library[INJ_LIB_MAXLEN];	/* library */
+	char		function[INJ_FUNC_MAXLEN];	/* function */
+} InjectionPointEntry;
+
+#define INJECTION_POINT_HASH_INIT_SIZE	16
+#define INJECTION_POINT_HASH_MAX_SIZE	128
+
+/*
+ * Local cache of injection callbacks already loaded, stored in
+ * TopMemoryContext.
+ */
+typedef struct InjectionPointCacheEntry
+{
+	char		name[INJ_NAME_MAXLEN];
+	InjectionPointCallback callback;
+} InjectionPointCacheEntry;
+
+static HTAB *InjectionPointCache = NULL;
+
+/*
+ * injection_point_cache_add
+ *
+ * Add an injection to the local cache.
+ */
+static void
+injection_point_cache_add(const char *name,
+						  InjectionPointCallback callback)
+{
+	InjectionPointCacheEntry *entry;
+	bool		found;
+
+	/* If first time, initialize */
+	if (InjectionPointCache == NULL)
+	{
+		HASHCTL		hash_ctl;
+
+		hash_ctl.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+		hash_ctl.entrysize = sizeof(InjectionPointCacheEntry);
+		hash_ctl.hcxt = TopMemoryContext;
+
+		InjectionPointCache = hash_create("InjectionPoint cache hash",
+										  INJECTION_POINT_HASH_MAX_SIZE,
+										  &hash_ctl,
+										  HASH_ELEM | HASH_STRINGS | HASH_CONTEXT);
+	}
+
+	entry = (InjectionPointCacheEntry *)
+		hash_search(InjectionPointCache, name, HASH_ENTER, &found);
+
+	Assert(!found);
+	memcpy(entry->name, name, strlen(name));
+	entry->callback = callback;
+}
+
+/*
+ * injection_point_cache_remove
+ *
+ * Remove entry from the local cache.  Note that this leaks a callback
+ * loaded but removed later on, which should have no consequence from
+ * a testing perspective.
+ */
+static void
+injection_point_cache_remove(const char *name)
+{
+	/* Leave if no cache */
+	if (InjectionPointCache == NULL)
+		return;
+
+	(void) hash_search(InjectionPointCache, name, HASH_REMOVE, NULL);
+}
+
+/*
+ * injection_point_cache_get
+ *
+ * Retrieve an injection point from the local cache, if any.
+ */
+static InjectionPointCallback
+injection_point_cache_get(const char *name)
+{
+	bool		found;
+	InjectionPointCacheEntry *entry;
+
+	/* no callback if no cache yet */
+	if (InjectionPointCache == NULL)
+		return NULL;
+
+	entry = (InjectionPointCacheEntry *)
+		hash_search(InjectionPointCache, name, HASH_FIND, &found);
+
+	if (found)
+		return entry->callback;
+
+	return NULL;
+}
+#endif							/* USE_INJECTION_POINTS */
+
+/*
+ * Return the space for dynamic shared hash table.
+ */
+Size
+InjectionPointShmemSize(void)
+{
+#ifdef USE_INJECTION_POINTS
+	Size		sz = 0;
+
+	sz = add_size(sz, hash_estimate_size(INJECTION_POINT_HASH_MAX_SIZE,
+										 sizeof(InjectionPointEntry)));
+	return sz;
+#else
+	return 0;
+#endif
+}
+
+/*
+ * Allocate shmem space for dynamic shared hash.
+ */
+void
+InjectionPointShmemInit(void)
+{
+#ifdef USE_INJECTION_POINTS
+	HASHCTL		info;
+
+	/* key is a NULL-terminated string */
+	info.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+	info.entrysize = sizeof(InjectionPointEntry);
+	InjectionPointHash = ShmemInitHash("InjectionPoint hash",
+									   INJECTION_POINT_HASH_INIT_SIZE,
+									   INJECTION_POINT_HASH_MAX_SIZE,
+									   &info,
+									   HASH_ELEM | HASH_FIXED_SIZE | HASH_STRINGS);
+#endif
+}
+
+#ifdef USE_INJECTION_POINTS
+static bool
+file_exists(const char *name)
+{
+	struct stat st;
+
+	Assert(name != NULL);
+	if (stat(name, &st) == 0)
+		return !S_ISDIR(st.st_mode);
+	else if (!(errno == ENOENT || errno == ENOTDIR))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not access file \"%s\": %m", name)));
+	return false;
+}
+#endif
+
+/*
+ * Attach a new injection point.
+ */
+void
+InjectionPointAttach(const char *name,
+					 const char *library,
+					 const char *function)
+{
+#ifdef USE_INJECTION_POINTS
+	InjectionPointEntry *entry_by_name;
+	bool		found;
+
+	if (strlen(name) >= INJ_NAME_MAXLEN)
+		elog(ERROR, "injection point name %s too long (maximum of %u)",
+			 name, INJ_NAME_MAXLEN);
+	if (strlen(library) >= INJ_LIB_MAXLEN)
+		elog(ERROR, "injection point library %s too long (maximum of %u)",
+			 library, INJ_LIB_MAXLEN);
+	if (strlen(function) >= INJ_FUNC_MAXLEN)
+		elog(ERROR, "injection point function %s too long (maximum of %u)",
+			 function, INJ_FUNC_MAXLEN);
+
+	/*
+	 * Allocate and register a new injection point.  A new point should not
+	 * exist.  For testing purposes this should be fine.
+	 */
+	LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+	entry_by_name = (InjectionPointEntry *)
+		hash_search(InjectionPointHash, name,
+					HASH_ENTER, &found);
+	if (found)
+	{
+		LWLockRelease(InjectionPointLock);
+		elog(ERROR, "injection point \"%s\" already defined", name);
+	}
+
+	/* Save the entry */
+	memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
+	entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
+	memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
+	entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
+	memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
+	entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
+
+	LWLockRelease(InjectionPointLock);
+
+#else
+	elog(ERROR, "injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Detach an existing injection point.
+ */
+void
+InjectionPointDetach(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+	bool		found;
+
+	LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+	hash_search(InjectionPointHash, name, HASH_REMOVE, &found);
+	LWLockRelease(InjectionPointLock);
+
+	if (!found)
+		elog(ERROR, "injection point \"%s\" not found", name);
+
+#else
+	elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Execute an injection point, if defined.
+ *
+ * Check first the shared hash table, and adapt the local cache depending
+ * on that as it could be possible that an entry to run has been removed.
+ */
+void
+InjectionPointRun(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+	InjectionPointEntry *entry_by_name;
+	bool		found;
+	InjectionPointCallback injection_callback;
+
+	LWLockAcquire(InjectionPointLock, LW_SHARED);
+	entry_by_name = (InjectionPointEntry *)
+		hash_search(InjectionPointHash, name,
+					HASH_FIND, &found);
+	LWLockRelease(InjectionPointLock);
+
+	/*
+	 * If not found, do nothing and remove it from the local cache if it
+	 * existed there.
+	 */
+	if (!found)
+	{
+		injection_point_cache_remove(name);
+		return;
+	}
+
+	/*
+	 * Check if the callback exists in the local cache, to avoid unnecessary
+	 * external loads.
+	 */
+	injection_callback = injection_point_cache_get(name);
+	if (injection_callback == NULL)
+	{
+		char		path[MAXPGPATH];
+
+		/* Not found in local cache, so load and register */
+		snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+				 entry_by_name->library, DLSUFFIX);
+
+		if (!file_exists(path))
+			elog(ERROR, "could not find library \"%s\" for injection point \"%s\"",
+				 path, name);
+
+		injection_callback = (InjectionPointCallback)
+			load_external_function(path, entry_by_name->function, true, NULL);
+
+		if (injection_callback == NULL)
+			elog(ERROR, "could not find function \"%s\" in library \"%s\" for injection point \"%s\"",
+				 name, entry_by_name->function, path);
+
+		/* add it to the local cache when found */
+		injection_point_cache_add(name, injection_callback);
+	}
+
+	injection_callback(name);
+#else
+	elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
diff --git a/src/backend/utils/misc/meson.build b/src/backend/utils/misc/meson.build
index 581724f254..6669502205 100644
--- a/src/backend/utils/misc/meson.build
+++ b/src/backend/utils/misc/meson.build
@@ -6,6 +6,7 @@ backend_sources += files(
   'guc_funcs.c',
   'guc_tables.c',
   'help_config.c',
+  'injection_point.c',
   'pg_config.c',
   'pg_controldata.c',
   'pg_rusage.c',
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index bb55695300..51830fc078 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -1656,6 +1656,21 @@ build-postgresql:
        </listitem>
       </varlistentry>
 
+      <varlistentry id="configure-option-enable-injection-points">
+       <term><option>--enable-injection-points</option></term>
+       <listitem>
+        <para>
+        Compiles <productname>PostgreSQL</productname> with support for
+        injection points in the server.  This is valuable to inject
+        user-defined code to force specific conditions to happen on the
+        server in pre-defined code paths.  This option is disabled by default.
+        See <xref linkend="xfunc-addin-injection-points"/> for more details.
+        This option is only for developers to test specific concurrency
+        scenarios.
+        </para>
+       </listitem>
+      </varlistentry>
+
       <varlistentry id="configure-option-with-segsize-blocks">
        <term><option>--with-segsize-blocks=SEGSIZE_BLOCKS</option></term>
        <listitem>
@@ -3160,6 +3175,21 @@ ninja install
       </listitem>
      </varlistentry>
 
+     <varlistentry id="configure-injection-points-meson">
+      <term><option>-Dinjection_points={ true | false }</option></term>
+      <listitem>
+       <para>
+        Compiles <productname>PostgreSQL</productname> with support for
+        injection points in the server.  This is valuable to inject
+        user-defined code to force specific conditions to happen on the
+        server in pre-defined code paths.  This option is disabled by default.
+        See <xref linkend="xfunc-addin-injection-points"/> for more details.
+        This option is only for developers to test specific concurrency
+        scenarios.
+       </para>
+      </listitem>
+     </varlistentry>
+
       <varlistentry id="configure-segsize-blocks-meson">
        <term><option>-Dsegsize_blocks=SEGSIZE_BLOCKS</option></term>
        <listitem>
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 89116ae74c..9dd9eafd22 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3510,6 +3510,60 @@ uint32 WaitEventExtensionNew(const char *wait_event_name)
     </para>
    </sect2>
 
+   <sect2 id="xfunc-addin-injection-points">
+    <title>Injection Points</title>
+
+    <para>
+     Add-ins can define injection points, that would run user-defined
+     code when going through a specific code path. This requires the use
+     of the following macro:
+<programlisting>
+INJECTION_POINT(name);
+</programlisting>
+    </para>
+
+    <para>
+     Callbacks can be attached to an injection point by calling:
+<programlisting>
+extern void InjectionPointAttach(const char *name,
+                                 const char *library,
+                                 const char *function);
+</programlisting>
+
+     <literal>name</literal> is the name of the injection point, that
+     will execute the <literal>function</literal> loaded from
+     <literal>library</literal>.
+     Injection points are saved in a hash table in shared memory, and
+     last until the server is shut down.
+    </para>
+
+    <para>
+     Here is an example of callback for
+     <literal>InjectionPointCallback</literal>:
+<programlisting>
+static void
+custom_injection_callback(const char *name)
+{
+    elog(NOTICE, "%s: executed custom callback", name);
+}
+</programlisting>
+    </para>
+
+    <para>
+     Optionally, it is possible to detach an injection point by calling:
+<programlisting>
+extern void InjectionPointDetach(const char *name);
+</programlisting>
+    </para>
+
+    <para>
+     Enabling injections points requires
+     <option>--enable-injection-points</option> with
+     <command>configure</command> or <option>-Dinjection_points=true</option>
+     with <application>Meson</application>.
+    </para>
+   </sect2>
+
    <sect2 id="extend-cpp">
     <title>Using C++ for Extensibility</title>
 
diff --git a/src/makefiles/meson.build b/src/makefiles/meson.build
index 034b26efa5..b0f4178b3d 100644
--- a/src/makefiles/meson.build
+++ b/src/makefiles/meson.build
@@ -54,6 +54,7 @@ pgxs_kv = {
 
   'enable_rpath': get_option('rpath') ? 'yes' : 'no',
   'enable_nls': libintl.found() ? 'yes' : 'no',
+  'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
   'enable_tap_tests': tap_tests_enabled ? 'yes' : 'no',
   'enable_debug': get_option('debug') ? 'yes' : 'no',
   'enable_coverage': 'no',
diff --git a/configure b/configure
index 819ca26a7a..70a1968003 100755
--- a/configure
+++ b/configure
@@ -759,6 +759,7 @@ CPPFLAGS
 LDFLAGS
 CFLAGS
 CC
+enable_injection_points
 enable_tap_tests
 enable_dtrace
 DTRACEFLAGS
@@ -839,6 +840,7 @@ enable_profiling
 enable_coverage
 enable_dtrace
 enable_tap_tests
+enable_injection_points
 with_blocksize
 with_segsize
 with_segsize_blocks
@@ -1532,6 +1534,8 @@ Optional Features:
   --enable-coverage       build with coverage testing instrumentation
   --enable-dtrace         build with DTrace support
   --enable-tap-tests      enable TAP tests (requires Perl and IPC::Run)
+  --enable-injection-points
+                          enable injection points (for testing)
   --enable-depend         turn on automatic dependency tracking
   --enable-cassert        enable assertion checks (for debugging)
   --disable-largefile     omit support for large files
@@ -3682,6 +3686,36 @@ fi

+#
+# Injection points
+#
+
+
+# Check whether --enable-injection-points was given.
+if test "${enable_injection_points+set}" = set; then :
+  enableval=$enable_injection_points;
+  case $enableval in
+    yes)
+
+$as_echo "#define USE_INJECTION_POINTS 1" >>confdefs.h
+
+      ;;
+    no)
+      :
+      ;;
+    *)
+      as_fn_error $? "no argument expected for --enable-injection-points option" "$LINENO" 5
+      ;;
+  esac
+
+else
+  enable_injection_points=no
+
+fi
+
+
+
+
 #
 # Block size
 #
diff --git a/configure.ac b/configure.ac
index 5bf3c82cf5..52fd7af446 100644
--- a/configure.ac
+++ b/configure.ac
@@ -250,6 +250,13 @@ PGAC_ARG_BOOL(enable, tap-tests, no,
               [enable TAP tests (requires Perl and IPC::Run)])
 AC_SUBST(enable_tap_tests)
 
+#
+# Injection points
+#
+PGAC_ARG_BOOL(enable, injection-points, no, [enable injection points (for testing)],
+              [AC_DEFINE([USE_INJECTION_POINTS], 1, [Define to 1 to build with injection points. (--enable-injection-points)])])
+AC_SUBST(enable_injection_points)
+
 #
 # Block size
 #
diff --git a/meson.build b/meson.build
index 57f9735feb..a67140a239 100644
--- a/meson.build
+++ b/meson.build
@@ -431,6 +431,7 @@ meson_bin = find_program(meson_binpath, native: true)
 ###############################################################
 
 cdata.set('USE_ASSERT_CHECKING', get_option('cassert') ? 1 : false)
+cdata.set('USE_INJECTION_POINTS', get_option('injection_points') ? 1 : false)
 
 blocksize = get_option('blocksize').to_int() * 1024
 
diff --git a/meson_options.txt b/meson_options.txt
index ee5d60b36e..249ecc5ffd 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -43,6 +43,9 @@ option('cassert', type: 'boolean', value: false,
 option('tap_tests', type: 'feature', value: 'auto',
   description: 'Enable TAP tests')
 
+option('injection_points', type: 'boolean', value: false,
+  description: 'Enable injection points')
+
 option('PG_TEST_EXTRA', type: 'string', value: '',
   description: 'Enable selected extra tests')
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index f8e461cbad..6f7de20527 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -203,6 +203,7 @@ enable_nls	= @enable_nls@
 enable_debug	= @enable_debug@
 enable_dtrace	= @enable_dtrace@
 enable_coverage	= @enable_coverage@
+enable_injection_points = @enable_injection_points@
 enable_tap_tests	= @enable_tap_tests@
 
 python_includespec	= @python_includespec@
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 5fd46b7bd1..575604d2a7 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1145,6 +1145,8 @@ IdentLine
 IdentifierLookup
 IdentifySystemCmd
 IfStackElem
+InjectionPointCacheEntry
+InjectionPointEntry
 ImportForeignSchemaStmt
 ImportForeignSchemaType
 ImportForeignSchema_function
-- 
2.43.0

From 1ffd6e1f8c20d0f9d0d28c85fc70128599ee3f5c Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 13:25:43 +0900
Subject: [PATCH v7 3/5] Add test module injection_points

This is a test facility aimed at providing basic coverage for the code
routines of injection points, while providing some callbacks that can be
used for other tests.  This will be extended with more tests.
---
 src/test/modules/Makefile                     |   7 ++
 src/test/modules/injection_points/.gitignore  |   4 +
 src/test/modules/injection_points/Makefile    |  22 ++++
 .../expected/injection_points.out             | 118 ++++++++++++++++++
 .../injection_points--1.0.sql                 |  36 ++++++
 .../injection_points/injection_points.c       |  95 ++++++++++++++
 .../injection_points/injection_points.control |   4 +
 src/test/modules/injection_points/meson.build |  37 ++++++
 .../injection_points/sql/injection_points.sql |  34 +++++
 src/test/modules/meson.build                  |   1 +
 10 files changed, 358 insertions(+)
 create mode 100644 src/test/modules/injection_points/.gitignore
 create mode 100644 src/test/modules/injection_points/Makefile
 create mode 100644 src/test/modules/injection_points/expected/injection_points.out
 create mode 100644 src/test/modules/injection_points/injection_points--1.0.sql
 create mode 100644 src/test/modules/injection_points/injection_points.c
 create mode 100644 src/test/modules/injection_points/injection_points.control
 create mode 100644 src/test/modules/injection_points/meson.build
 create mode 100644 src/test/modules/injection_points/sql/injection_points.sql

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5d33fa6a9a..5bb7f848fe 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -37,6 +37,13 @@ SUBDIRS = \
 		  worker_spi \
 		  xid_wraparound
 
+
+ifeq ($(enable_injection_points),yes)
+SUBDIRS += injection_points
+else
+ALWAYS_SUBDIRS += injection_points
+endif
+
 ifeq ($(with_ssl),openssl)
 SUBDIRS += ssl_passphrase_callback
 else
diff --git a/src/test/modules/injection_points/.gitignore b/src/test/modules/injection_points/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/injection_points/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
new file mode 100644
index 0000000000..fdd3b8e6da
--- /dev/null
+++ b/src/test/modules/injection_points/Makefile
@@ -0,0 +1,22 @@
+# src/test/modules/injection_points/Makefile
+
+MODULE_big = injection_points
+OBJS = \
+	$(WIN32RES) \
+	injection_points.o
+PGFILEDESC = "injection_points - facility for injection points"
+
+EXTENSION = injection_points
+DATA = injection_points--1.0.sql
+REGRESS = injection_points
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/injection_points
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/injection_points/expected/injection_points.out b/src/test/modules/injection_points/expected/injection_points.out
new file mode 100644
index 0000000000..2566af5359
--- /dev/null
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -0,0 +1,118 @@
+CREATE EXTENSION injection_points;
+SELECT injection_points_attach('TestInjectionBooh', 'booh');
+ERROR:  incorrect mode "booh" for injection point creation
+SELECT injection_points_attach('TestInjectionError', 'error');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_attach('TestInjectionLog', 'notice');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_attach('TestInjectionLog2', 'notice');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionBooh'); -- nothing
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionError'); -- error
+ERROR:  error triggered for injection point TestInjectionError
+-- Re-load cache and run again.
+\c
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionError'); -- error
+ERROR:  error triggered for injection point TestInjectionError
+-- Remove one entry and check the remaining entries.
+SELECT injection_points_detach('TestInjectionError'); -- ok
+ injection_points_detach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionError'); -- nothing
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+-- More entries removed, letting TestInjectionLog2 to check the same
+-- callback used in more than one point.
+SELECT injection_points_detach('TestInjectionLog'); -- ok
+ injection_points_detach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog'); -- nothing
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionError'); -- nothing
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+SELECT injection_points_detach('TestInjectionLog'); -- fails
+ERROR:  injection point "TestInjectionLog" not found
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ injection_points_run 
+----------------------
+ 
+(1 row)
+
+DROP EXTENSION injection_points;
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
new file mode 100644
index 0000000000..64ff69bd7f
--- /dev/null
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -0,0 +1,36 @@
+/* src/test/modules/injection_points/injection_points--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION injection_points" to load this file. \quit
+
+--
+-- injection_points_attach()
+--
+-- Attaches an injection point using callbacks from one of the predefined
+-- modes.
+--
+CREATE FUNCTION injection_points_attach(IN point_name TEXT,
+    IN mode text)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_attach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- injection_points_run()
+--
+-- Executes an injection point.
+--
+CREATE FUNCTION injection_points_run(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_run'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- injection_points_detach()
+--
+-- Detaches an injection point.
+--
+CREATE FUNCTION injection_points_detach(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_detach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
new file mode 100644
index 0000000000..47e7522faf
--- /dev/null
+++ b/src/test/modules/injection_points/injection_points.c
@@ -0,0 +1,95 @@
+/*--------------------------------------------------------------------------
+ *
+ * injection_points.c
+ *		Code for testing injection points.
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *		src/test/modules/injection_points/injection_points.c
+ *
+ * Injection points are able to trigger user-defined callbacks in pre-defined
+ * code paths.
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/builtins.h"
+#include "utils/injection_point.h"
+#include "utils/wait_event.h"
+
+PG_MODULE_MAGIC;
+
+extern PGDLLEXPORT void injection_error(const char *name);
+extern PGDLLEXPORT void injection_notice(const char *name);
+
+
+/* Set of callbacks available at point creation */
+void
+injection_error(const char *name)
+{
+	elog(ERROR, "error triggered for injection point %s", name);
+}
+
+void
+injection_notice(const char *name)
+{
+	elog(NOTICE, "notice triggered for injection point %s", name);
+}
+
+/*
+ * SQL function for creating an injection point.
+ */
+PG_FUNCTION_INFO_V1(injection_points_attach);
+Datum
+injection_points_attach(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+	char	   *mode = text_to_cstring(PG_GETARG_TEXT_PP(1));
+	char	   *function;
+
+	if (strcmp(mode, "error") == 0)
+		function = "injection_error";
+	else if (strcmp(mode, "notice") == 0)
+		function = "injection_notice";
+	else
+		elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
+
+	InjectionPointAttach(name, "injection_points", function);
+
+	PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for triggering an injection point.
+ */
+PG_FUNCTION_INFO_V1(injection_points_run);
+Datum
+injection_points_run(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+	INJECTION_POINT(name);
+
+	PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for dropping an injection point.
+ */
+PG_FUNCTION_INFO_V1(injection_points_detach);
+Datum
+injection_points_detach(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+	InjectionPointDetach(name);
+
+	PG_RETURN_VOID();
+}
diff --git a/src/test/modules/injection_points/injection_points.control b/src/test/modules/injection_points/injection_points.control
new file mode 100644
index 0000000000..b4214f3bdc
--- /dev/null
+++ b/src/test/modules/injection_points/injection_points.control
@@ -0,0 +1,4 @@
+comment = 'Test code for injection points'
+default_version = '1.0'
+module_pathname = '$libdir/injection_points'
+relocatable = true
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
new file mode 100644
index 0000000000..ee37573f2a
--- /dev/null
+++ b/src/test/modules/injection_points/meson.build
@@ -0,0 +1,37 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+  subdir_done()
+endif
+
+injection_points_sources = files(
+  'injection_points.c',
+)
+
+if host_system == 'windows'
+  injection_points_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'injection_points',
+    '--FILEDESC', 'injection_points - facility for injection points',])
+endif
+
+injection_points = shared_module('injection_points',
+  injection_points_sources,
+  kwargs: pg_test_mod_args,
+)
+test_install_libs += injection_points
+
+test_install_data += files(
+  'injection_points.control',
+  'injection_points--1.0.sql',
+)
+
+tests += {
+  'name': 'injection_points',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'regress': {
+    'sql': [
+      'injection_points',
+    ],
+  },
+}
diff --git a/src/test/modules/injection_points/sql/injection_points.sql b/src/test/modules/injection_points/sql/injection_points.sql
new file mode 100644
index 0000000000..23c7e435ad
--- /dev/null
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -0,0 +1,34 @@
+CREATE EXTENSION injection_points;
+
+SELECT injection_points_attach('TestInjectionBooh', 'booh');
+SELECT injection_points_attach('TestInjectionError', 'error');
+SELECT injection_points_attach('TestInjectionLog', 'notice');
+SELECT injection_points_attach('TestInjectionLog2', 'notice');
+
+SELECT injection_points_run('TestInjectionBooh'); -- nothing
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+SELECT injection_points_run('TestInjectionLog'); -- notice
+SELECT injection_points_run('TestInjectionError'); -- error
+
+-- Re-load cache and run again.
+\c
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+SELECT injection_points_run('TestInjectionLog'); -- notice
+SELECT injection_points_run('TestInjectionError'); -- error
+
+-- Remove one entry and check the remaining entries.
+SELECT injection_points_detach('TestInjectionError'); -- ok
+SELECT injection_points_run('TestInjectionLog'); -- notice
+SELECT injection_points_run('TestInjectionError'); -- nothing
+-- More entries removed, letting TestInjectionLog2 to check the same
+-- callback used in more than one point.
+SELECT injection_points_detach('TestInjectionLog'); -- ok
+SELECT injection_points_run('TestInjectionLog'); -- nothing
+SELECT injection_points_run('TestInjectionError'); -- nothing
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+
+SELECT injection_points_detach('TestInjectionLog'); -- fails
+
+SELECT injection_points_run('TestInjectionLog2'); -- notice
+
+DROP EXTENSION injection_points;
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 00ff1d77d1..ef7bd62c85 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -5,6 +5,7 @@ subdir('commit_ts')
 subdir('delay_execution')
 subdir('dummy_index_am')
 subdir('dummy_seclabel')
+subdir('injection_points')
 subdir('ldap_password_func')
 subdir('libpq_pipeline')
 subdir('plsample')
-- 
2.43.0

From b2dd7f5b0f19513132ffe8cfc2a17310d56b2648 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 13:30:56 +0900
Subject: [PATCH v7 4/5] Add regression test to show snapbuild consistency

Reverting 409f9ca44713 causes the test to fail.  The test added here
relies on the existing callbacks in injection_points.
---
 src/backend/replication/logical/snapbuild.c |  3 ++
 src/test/recovery/Makefile                  |  7 ++-
 src/test/recovery/meson.build               |  4 ++
 src/test/recovery/t/040_snapshot_status.pl  | 51 +++++++++++++++++++++
 4 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 src/test/recovery/t/040_snapshot_status.pl

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index a0b7947d2f..93048afc2f 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -141,6 +141,7 @@
 #include "storage/procarray.h"
 #include "storage/standby.h"
 #include "utils/builtins.h"
+#include "utils/injection_point.h"
 #include "utils/memutils.h"
 #include "utils/snapmgr.h"
 #include "utils/snapshot.h"
@@ -654,6 +655,8 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	snap->xcnt = newxcnt;
 	snap->xip = newxip;
 
+	INJECTION_POINT("SnapBuildInitialSnapshot");
+
 	return snap;
 }
 
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index 17ee353735..f57baba5e8 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -9,12 +9,17 @@
 #
 #-------------------------------------------------------------------------
 
-EXTRA_INSTALL=contrib/pg_prewarm contrib/pg_stat_statements contrib/test_decoding
+EXTRA_INSTALL=contrib/pg_prewarm \
+	contrib/pg_stat_statements \
+	contrib/test_decoding \
+	src/test/modules/injection_points
 
 subdir = src/test/recovery
 top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global
 
+export enable_injection_points enable_injection_points
+
 # required for 017_shm.pl and 027_stream_regress.pl
 REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
 export REGRESS_SHLIB
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index 88fb0306f5..43d7411a79 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -6,6 +6,9 @@ tests += {
   'bd': meson.current_build_dir(),
   'tap': {
     'test_kwargs': {'priority': 40}, # recovery tests are slow, start early
+    'env': {
+      'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
+    },
     'tests': [
       't/001_stream_rep.pl',
       't/002_archiving.pl',
@@ -45,6 +48,7 @@ tests += {
       't/037_invalid_database.pl',
       't/038_save_logical_slots_shutdown.pl',
       't/039_end_of_wal.pl',
+      't/040_snapshot_status.pl',
     ],
   },
 }
diff --git a/src/test/recovery/t/040_snapshot_status.pl b/src/test/recovery/t/040_snapshot_status.pl
new file mode 100644
index 0000000000..e77b138144
--- /dev/null
+++ b/src/test/recovery/t/040_snapshot_status.pl
@@ -0,0 +1,51 @@
+# Test consistent of initial snapshot data.
+
+# This requires a node with wal_level=logical combined with an injection
+# point that forces a failure when a snapshot is initially built with a
+# logical slot created.
+#
+# See bug https://postgr.es/m/CAFiTN-s0zA1Kj0ozGHwkYkHwa5U0zUE94RSc_g81WrpcETB5=[email-masked].
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+$node->safe_psql('postgres',
+  "SELECT injection_points_attach('SnapBuildInitialSnapshot', 'error');");
+
+my $node_host = $node->host;
+my $node_port = $node->port;
+my $connstr_common = "host=$node_host port=$node_port";
+my $connstr_db = "$connstr_common replication=database dbname=postgres";
+
+# This requires a single session, with two commands.
+my $psql_session =
+  $node->background_psql('postgres', on_error_stop => 0,
+			 extra_params => [ '-d', $connstr_db ]);
+my ($output, $ret) = $psql_session->query(
+    'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+ok($ret != 0, "First CREATE_REPLICATION_SLOT fails on injected error");
+
+# Now remove the injected error and check that the second command works.
+$node->safe_psql('postgres',
+  "SELECT injection_points_detach('SnapBuildInitialSnapshot');");
+
+($output, $ret) = $psql_session->query(
+    'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+ok(substr($output, 0, 4) eq 'slot',
+   "Second CREATE_REPLICATION_SLOT passes");
+
+done_testing();
-- 
2.43.0

From 968e591966c72e316f4b1322deb6bfabea43a99e Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 13:36:53 +0900
Subject: [PATCH v7 5/5] Add basic test for promotion and restart race
 condition

This test fails after 7863ee4def65 is reverted.  test_injection_points
is extended so as it is possible to add condition variables to wait for
in the point callbacks, with a SQL function to broadcast condition
variables that may be sleeping.

I guess that this should be extended so as there is more than one
condition variable stored in shmem for this module, controlling which
variable to wait for directly in the callback itself, but that's not
really necessary now.
---
 src/backend/access/transam/xlog.c             |   7 +
 .../injection_points--1.0.sql                 |  10 ++
 .../injection_points/injection_points.c       |  69 +++++++++
 src/test/recovery/meson.build                 |   1 +
 .../t/041_invalid_checkpoint_after_promote.pl | 138 ++++++++++++++++++
 5 files changed, 225 insertions(+)
 create mode 100644 src/test/recovery/t/041_invalid_checkpoint_after_promote.pl

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 478377c4a2..29e8f798f8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -100,6 +100,7 @@
 #include "storage/sync.h"
 #include "utils/guc_hooks.h"
 #include "utils/guc_tables.h"
+#include "utils/injection_point.h"
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
 #include "utils/relmapper.h"
@@ -7428,6 +7429,12 @@ CreateRestartPoint(int flags)
 
 	CheckPointGuts(lastCheckPoint.redo, flags);
 
+	/*
+	 * This location is important to be after CheckPointGuts() to ensure
+	 * that some work has happened.
+	 */
+	INJECTION_POINT("CreateRestartPoint");
+
 	/*
 	 * Remember the prior checkpoint's redo ptr for
 	 * UpdateCheckPointDistanceEstimate()
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
index 64ff69bd7f..09ff14e24d 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -25,6 +25,16 @@ RETURNS void
 AS 'MODULE_PATHNAME', 'injection_points_run'
 LANGUAGE C STRICT PARALLEL UNSAFE;
 
+--
+-- injection_points_wake()
+--
+-- Wakes a condition variable executed in an injection point.
+--
+CREATE FUNCTION injection_points_wake()
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_wake'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
 --
 -- injection_points_detach()
 --
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 47e7522faf..599ced22a6 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -18,6 +18,7 @@
 #include "postgres.h"
 
 #include "fmgr.h"
+#include "storage/condition_variable.h"
 #include "storage/lwlock.h"
 #include "storage/shmem.h"
 #include "utils/builtins.h"
@@ -26,10 +27,48 @@
 
 PG_MODULE_MAGIC;
 
+/* Shared state information for injection points. */
+typedef struct TestInjectionPointSharedState
+{
+	/*
+	 * Wait variable that can be registered at a given point, and that can be
+	 * awakened via SQL.
+	 */
+	ConditionVariable	wait_point;
+} TestInjectionPointSharedState;
+
+/* Pointer to shared-memory state. */
+static TestInjectionPointSharedState *inj_state = NULL;
+
+/* Wait event when waiting on condition variable */
+static uint32 injection_wait_event = 0;
+
 extern PGDLLEXPORT void injection_error(const char *name);
 extern PGDLLEXPORT void injection_notice(const char *name);
+extern PGDLLEXPORT void injection_wait(const char *name);

+static void
+injection_init_shmem(void)
+{
+	bool		found;
+
+	if (inj_state != NULL)
+		return;
+
+	LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+	inj_state = ShmemInitStruct("injection_points",
+								sizeof(TestInjectionPointSharedState),
+								&found);
+	if (!found)
+	{
+		/* First time through ... */
+		MemSet(inj_state, 0, sizeof(TestInjectionPointSharedState));
+		ConditionVariableInit(&inj_state->wait_point);
+	}
+	LWLockRelease(AddinShmemInitLock);
+}
+
 /* Set of callbacks available at point creation */
 void
 injection_error(const char *name)
@@ -43,6 +82,20 @@ injection_notice(const char *name)
 	elog(NOTICE, "notice triggered for injection point %s", name);
 }
 
+void
+injection_wait(const char *name)
+{
+	if (inj_state == NULL)
+		injection_init_shmem();
+	if (injection_wait_event == 0)
+		injection_wait_event = WaitEventExtensionNew("injection_wait");
+
+	/* And sleep.. */
+	ConditionVariablePrepareToSleep(&inj_state->wait_point);
+	ConditionVariableSleep(&inj_state->wait_point, injection_wait_event);
+	ConditionVariableCancelSleep();
+}
+
 /*
  * SQL function for creating an injection point.
  */
@@ -58,6 +111,8 @@ injection_points_attach(PG_FUNCTION_ARGS)
 		function = "injection_error";
 	else if (strcmp(mode, "notice") == 0)
 		function = "injection_notice";
+	else if (strcmp(mode, "wait") == 0)
+		function = "injection_wait";
 	else
 		elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
 
@@ -80,6 +135,20 @@ injection_points_run(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
 
+/*
+ * SQL function for waking a condition variable.
+ */
+PG_FUNCTION_INFO_V1(injection_points_wake);
+Datum
+injection_points_wake(PG_FUNCTION_ARGS)
+{
+	if (inj_state == NULL)
+		injection_init_shmem();
+
+	ConditionVariableBroadcast(&inj_state->wait_point);
+	PG_RETURN_VOID();
+}
+
 /*
  * SQL function for dropping an injection point.
  */
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index 43d7411a79..c12b7fcf97 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -49,6 +49,7 @@ tests += {
       't/038_save_logical_slots_shutdown.pl',
       't/039_end_of_wal.pl',
       't/040_snapshot_status.pl',
+      't/041_invalid_checkpoint_after_promote.pl',
     ],
   },
 }
diff --git a/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl
new file mode 100644
index 0000000000..af4ab03604
--- /dev/null
+++ b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl
@@ -0,0 +1,138 @@
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep nanosleep);
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+# initialize primary node
+my $node_primary = PostgreSQL::Test::Cluster->new('master');
+$node_primary->init(allows_streaming => 1);
+$node_primary->append_conf(
+	'postgresql.conf', q[
+checkpoint_timeout = 30s
+log_checkpoints = on
+restart_after_crash = on
+]);
+$node_primary->start;
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+
+# setup a standby
+my $node_standby = PostgreSQL::Test::Cluster->new('standby1');
+$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1);
+$node_standby->start;
+
+# dummy table for the upcoming tests.
+$node_primary->safe_psql('postgres', 'checkpoint');
+$node_primary->safe_psql('postgres', 'CREATE TABLE prim_tab (a int);');
+
+# Register a injection point on the standby so as the follow-up
+# restart point running on it will wait.
+$node_primary->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+# Wait until the extension has been created on the standby
+$node_primary->wait_for_replay_catchup($node_standby);
+# This causes a restartpoint to wait on a standby.
+$node_standby->safe_psql('postgres',
+  "SELECT injection_points_attach('CreateRestartPoint', 'wait');");
+
+# Execute a restart point on the standby, that will be waited on.
+# This needs to be in the background as we'll wait on it.
+my $logstart = -s $node_standby->logfile;
+my $psql_session =
+  $node_standby->background_psql('postgres', on_error_stop => 0);
+$psql_session->query_until(qr/starting_checkpoint/, q(
+   \echo starting_checkpoint
+   CHECKPOINT;
+));
+
+# Switch one WAL segment to make the restartpoint remove it.
+$node_primary->safe_psql('postgres', 'INSERT INTO prim_tab VALUES (1);');
+$node_primary->safe_psql('postgres', 'SELECT pg_switch_wal();');
+$node_primary->wait_for_replay_catchup($node_standby);
+
+# Wait until the checkpointer is in the middle of the restartpoint
+# processing.
+ok( $node_standby->poll_query_until(
+	'postgres',
+	qq[SELECT count(*) FROM pg_stat_activity
+           WHERE backend_type = 'checkpointer' AND wait_event = 'injection_wait' ;],
+	'1'),
+    'checkpointer is waiting at restart point'
+    ) or die "Timed out while waiting for checkpointer to run restartpoint";
+
+
+# Restartpoint should have started on standby.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+	$checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');
+
+# promote during restartpoint
+$node_primary->stop;
+$node_standby->promote;
+
+# Update the start position before waking up the checkpointer!
+$logstart = -s $node_standby->logfile;
+
+# Now wake up the checkpointer
+$node_standby->safe_psql('postgres',
+  "SELECT injection_points_wake();");
+
+# wait until checkpoint completes on the newly-promoted standby.
+my $checkpoint_complete = 0;
+for (my $i = 0; $i < 3000; $i++)
+{
+	my $log = slurp_file($node_standby->logfile, $logstart);
+	if ($log =~ m/restartpoint complete/)
+	{
+		$checkpoint_complete = 1;
+		last;
+	}
+	usleep(100_000);
+}
+is($checkpoint_complete, 1, 'restartpoint has completed');
+
+# kill SIGKILL a backend, and all backend will restart. Note that previous
+# checkpoint has not completed.
+my $psql_timeout = IPC::Run::timer(3600);
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+	[ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_standby->connstr('postgres') ],
+	'<',
+	\$killme_stdin,
+	'>',
+	\$killme_stdout,
+	'2>',
+	\$killme_stderr,
+	$psql_timeout);
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/;
+my $pid = $killme_stdout;
+chomp($pid);
+my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid);
+is($ret, 0, 'killed process with KILL');
+my $stdout;
+my $stderr;
+
+# After recovery, the server should be able to start.
+for (my $i = 0; $i < 30; $i++)
+{
+    ($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1');
+    last if $ret == 0;
+	sleep(1);
+}
+is($ret, 0, "psql connect success");
+is($stdout, 1, "psql select 1");
+
+done_testing();
-- 
2.43.0

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 10 Jan 2024 15:21:03 +0530

> Okay, I can see your point.  I have reorganized the docs in the

Thanks. This looks better. Needs further wordsmithy. But that can wait
till the code has been reviewed.

> within extensions which don't care about a #define in a backend

The macros should be in extension facing header, ofc. But I take back
this suggestion since defining these macros is extra work (every time
a new injection point is declared) and your suggestion to grep
practically works. We can improve things as needed.

> would be a point in restricting that, either.

Since an unintentional misspelling might trigger an unintended
injection point. But we will see how much of that happens in practice.

> 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.

> strncpy() is less used in the backend code.  It comes to a matter of

To me using memcpy implies that the contents of the memory being
copied can be non-character. For a buffer containing a character
string I would prefer strncpy. But I wouldn't argue furher..

> in the call paths.  In short, just don't do that, or use two distinct

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.

> really buy it, but people like doing fancy things sometimes.

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.

> output file.

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.

>  There was a different issue when moving the tests to

> The local cache is reloaded.  Reworded.

We are starting a new backend not "re"loading a cache in an existing
backend per say.

> separate shared state, for example, but that does not improve the test

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.

> 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.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Michael Paquier <[email-masked]>
Date: Thu, 11 Jan 2024 13:11:44 +0900

> file exists or not. So throwing an error is better than just returning

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.

> 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."

> easier. That at least should be the goal of the first cut.

I don't really aim for complicated here, just useful.

> 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.

> 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.

> 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 <[email-masked]>
Date: Tue, 9 Jan 2024 10:32:29 +0900
Subject: [PATCH v8 1/5] Refactor code to check file file existence

 /*
 
+/*

-static bool
 
 /*

-		if (file_exists(full))
 
 		pfree(full);
-- 
2.43.0

From 60035e582818dbc23c81458d7511846ecc597ab3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 13:20:01 +0900
Subject: [PATCH v8 2/5] Add backend facility for injection points

 

 
 /*
index f625473ad4..09aeb51bc8 100644
@@ -329,6 +329,7 @@ WrapLimitsVacuum	"Waiting to update limits on transaction id and multixact consu
 
 #
 # END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)

 

 
+#
+# Injection points
+#
+
+
 
+#
+# Injection points
+#

 

 

From 012287e17a6e19d88e0ffd6651acfa3ad2e8a906 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 13:25:43 +0900
Subject: [PATCH v8 3/5] Add test module injection_points

 
+

From 22a5c58665c10002b1d7226de2d2bb0b1842afb5 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 9 Jan 2024 13:30:56 +0900
Subject: [PATCH v8 4/5] Add regression test to show snapbuild consistency

 

 

From 3f934801bad3d65d466e431c813d14faf617a988 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Thu, 11 Jan 2024 13:10:52 +0900
Subject: [PATCH v8 5/5] Add basic test for promotion and restart race
 condition

 
 
+	/*
 
+--
 
 #include "fmgr.h"
 
 PG_MODULE_MAGIC;

+static void
 
+void

+/*
index 0000000000..886454d50c
+use Time::HiRes qw(usleep);

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 11 Jan 2024 14:47:27 +0530

> previous callbacks attached to it."

It doesn't behave exactly like that either. If the INJECTION_POINT is
run after detach (but before Attach), the local cache will be updated.
A subsequent attach and INJECTION_POINT call would fetch the new
callback.

> I don't really aim for complicated here, just useful.

It isn't complicated. Such simple error check improve user's
confidence on the feature and better be part of the 1st cut.

> was loading Time::HiRes::nanosleep and Windows does not support it.

Some miscommunication here. The SQL test under injection_point module
can be run in a build without injection_point and it fails. I think
it's better to have an alternate output for the same or prohibit the
test running itself.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Michael Paquier <[email-masked]>
Date: Fri, 12 Jan 2024 08:35:42 +0900

> confidence on the feature and better be part of the 1st cut.

I'm really not sure about that, because it does not impact the scope
of the facility even with all the use cases I've seen where injection
points could be used.  It could always be added later if there's a
strong push for it.  For testing, I'm biased about attempting to load
callbacks in the process attaching them.

> it's better to have an alternate output for the same or prohibit the

The same problem exists if you try to run the SSL tests in
src/test/ssl/ without support build for them.  Protections at the
upper levels are good enough for the CI and the buildfarm, while
making the overall maintenance cheaper, so I'm happy with just these.

It also seems like you've missed this message, where this has been
mentioned (spoiler: first version of the patch used an alternate
output):
https://www.postgresql.org/message-id/[email-masked] 
--
Michael