Hi Michael,
There is some overlap between Dtrace functionality and this
functionality. But I see differences too. E.g. injection points offer
deeper integration whereas dtrace provides more information to the
probe like callstack and argument values etc. We need to assess
whether these functionality can co-exist and whether we need both of
them. If the answer to both of these questions is yes, it will be good
to add documentation explaining the differences and similarities and
also some guidance on when to use what.
On Fri, Jan 12, 2024 at 9:56 AM Michael Paquier <mich...@paquier.xyz> wrote:
>
> On Fri, Jan 12, 2024 at 08:35:42AM +0900, Michael Paquier wrote:
> > 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/zunuzpimkzcov...@paquier.xyz
>
> The refactoring of 0001 has now been applied as of e72a37528dda, and
> the buildfarm looks stable (at least for now).
>
> Here is a rebased patch set of the rest.
+
+#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;
+}
Shouldn't this be removed now? The code should use one from fd.c
Other code changes look good. I think the documentation and comments
need some changes esp. considering the users point of view. Have
attached two patches (0003, and 0004) with those changes to be applied
on top of 0001 and 0002 respectively. Please review them. Might need
some wordsmithy and language correction. Attaching the whole patch set
to keep cibot happy.
This is review of 0001 and 0002 only. Once we take care of these
comments I think those patches will be ready for commit except one
point of contention mentioned in [1]. We haven't heard any third
opinion yet.
[1]
https://www.postgresql.org/message-id/CAExHW5sc_ar7=w9xccc9twyxzf71ghc6poq_+u4hxtxmnb7...@mail.gmail.com
--
Best Wishes,
Ashutosh Bapat
From 7a26f1c345e8a8dcf895dcd5d1d45012f46dd826 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Wed, 17 Jan 2024 16:59:03 +0530
Subject: [PATCH 2/6] Comment and documentation suggestions.
... reflecting a user's point of view.
Ashutosh Bapat
---
doc/src/sgml/installation.sgml | 24 ++++++++++++------------
doc/src/sgml/xfunc.sgml | 24 +++++++++++++++---------
src/backend/utils/misc/injection_point.c | 11 +++++------
3 files changed, 32 insertions(+), 27 deletions(-)
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index 51830fc078..f913b6b78f 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -1661,12 +1661,12 @@ build-postgresql:
<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.
+ injection points in the server. Injection points allow to run
+ user-defined code from within the server in pre-defined code paths.
+ This helps in testing and investigation of concurrency scenarios in a
+ controlled fashion. This option is disabled by default. See <xref
+ linkend="xfunc-addin-injection-points"/> for more details. This option
+ is inteded to be used only by developers for testing.
</para>
</listitem>
</varlistentry>
@@ -3180,12 +3180,12 @@ ninja install
<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.
+ injection points in the server. Injection points allow to run
+ user-defined code from within the server in pre-defined code paths.
+ This helps in testing and investigation of concurrency scenarios in a
+ controlled fashion. This option is disabled by default. See <xref
+ linkend="xfunc-addin-injection-points"/> for more details. This option
+ is inteded to be used only by developers for testing.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 9dd9eafd22..59dbe73c67 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3514,27 +3514,24 @@ uint32 WaitEventExtensionNew(const char *wait_event_name)
<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:
+ An injection point with a given name <literal>name</literal> is declared using macro:
<programlisting>
INJECTION_POINT(name);
</programlisting>
+ There are a few injection points already declared at strategic points within the server code. After adding a new injection point the code needs to be compiled in order for that injection point to be available in the binary. Add-ins written in C-language can declare injection point in their own code using the same macro.
</para>
<para>
- Callbacks can be attached to an injection point by calling:
+ Add-ins can attach callbacks to an already declared 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.
+ <literal>name</literal> is the name of the injection point, which when
+ reached during execution will execute the <literal>function</literal>
+ loaded from <literal>library</literal>.
</para>
<para>
@@ -3547,6 +3544,7 @@ custom_injection_callback(const char *name)
elog(NOTICE, "%s: executed custom callback", name);
}
</programlisting>
+ This callback simply prints a message to server error log with severity NOTCE. But callbacks may implement more complex logic.
</para>
<para>
@@ -3556,6 +3554,14 @@ extern void InjectionPointDetach(const char *name);
</programlisting>
</para>
+ <para>
+ A callback attached to an injection point is available across all the
+ backends including the backends started after
+ <literal>InjectionPointAttach</literal> is called. It remains attached till
+ server is running or it is detached using
+ <literal>InjectionPointDetach</literal>.
+ </para>
+
<para>
Enabling injections points requires
<option>--enable-injection-points</option> with
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 205dcfa31d..e9f326a1be 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -3,9 +3,8 @@
* 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.
+ * Injection points can be used to run arbitrary code by attaching callbacks
+ * that would be executed in place of the named injection point.
*
* Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
@@ -55,7 +54,7 @@ typedef struct InjectionPointEntry
#define INJECTION_POINT_HASH_MAX_SIZE 128
/*
- * Local cache of injection callbacks already loaded, stored in
+ * Backend local cache of injection callbacks already loaded, stored in
* TopMemoryContext.
*/
typedef struct InjectionPointCacheEntry
@@ -69,7 +68,7 @@ static HTAB *InjectionPointCache = NULL;
/*
* injection_point_cache_add
*
- * Add an injection to the local cache.
+ * Add an injection point to the local cache.
*/
static void
injection_point_cache_add(const char *name,
@@ -129,7 +128,7 @@ injection_point_cache_get(const char *name)
bool found;
InjectionPointCacheEntry *entry;
- /* no callback if no cache yet */
+ /* No callback if no cache yet. */
if (InjectionPointCache == NULL)
return NULL;
--
2.25.1
From 089d1a3a549d7694a6b093e4dc6321a711331dec Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Thu, 18 Jan 2024 10:32:29 +0530
Subject: [PATCH 4/6] Comment and documentation suggestions.
... reflecting a user's point of view.
Ashutosh Bapat
---
.../modules/injection_points/injection_points--1.0.sql | 9 ++++-----
src/test/modules/injection_points/injection_points.c | 10 +++++-----
2 files changed, 9 insertions(+), 10 deletions(-)
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..5944c41716 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -6,11 +6,10 @@
--
-- injection_points_attach()
--
--- Attaches an injection point using callbacks from one of the predefined
--- modes.
+-- Attaches the action to the given injection point.
--
CREATE FUNCTION injection_points_attach(IN point_name TEXT,
- IN mode text)
+ IN action text)
RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_attach'
LANGUAGE C STRICT PARALLEL UNSAFE;
@@ -18,7 +17,7 @@ LANGUAGE C STRICT PARALLEL UNSAFE;
--
-- injection_points_run()
--
--- Executes an injection point.
+-- Executes the action attached to the injection point.
--
CREATE FUNCTION injection_points_run(IN point_name TEXT)
RETURNS void
@@ -28,7 +27,7 @@ LANGUAGE C STRICT PARALLEL UNSAFE;
--
-- injection_points_detach()
--
--- Detaches an injection point.
+-- Detaches the current action, if any, from the given injection point.
--
CREATE FUNCTION injection_points_detach(IN point_name TEXT)
RETURNS void
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 47e7522faf..bf947de0b7 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -30,7 +30,7 @@ extern PGDLLEXPORT void injection_error(const char *name);
extern PGDLLEXPORT void injection_notice(const char *name);
-/* Set of callbacks available at point creation */
+/* Set of callbacks available to be attached to an injection point. */
void
injection_error(const char *name)
{
@@ -51,15 +51,15 @@ 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 *action = text_to_cstring(PG_GETARG_TEXT_PP(1));
char *function;
- if (strcmp(mode, "error") == 0)
+ if (strcmp(action, "error") == 0)
function = "injection_error";
- else if (strcmp(mode, "notice") == 0)
+ else if (strcmp(action, "notice") == 0)
function = "injection_notice";
else
- elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
+ elog(ERROR, "incorrect action \"%s\" for injection point creation", action);
InjectionPointAttach(name, "injection_points", function);
--
2.25.1
From 0778f3023f90fd4c1d6a9903519ae542eb61e253 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:25:43 +0900
Subject: [PATCH 3/6] 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.25.1
From a0c0fb31648a8dfaf9b5d2c453496d1942aa1c2f Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:20:01 +0900
Subject: [PATCH 1/6] 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.
---
configure | 34 ++
configure.ac | 7 +
doc/src/sgml/installation.sgml | 30 ++
doc/src/sgml/xfunc.sgml | 54 +++
meson.build | 1 +
meson_options.txt | 3 +
src/Makefile.global.in | 1 +
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 +
src/include/pg_config.h.in | 3 +
src/include/utils/injection_point.h | 37 ++
src/makefiles/meson.build | 1 +
src/tools/pgindent/typedefs.list | 2 +
17 files changed, 514 insertions(+)
create mode 100644 src/backend/utils/misc/injection_point.c
create mode 100644 src/include/utils/injection_point.h
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/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/meson.build b/meson.build
index c317144b6b..55184db248 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/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index e5119ed55d..6b9dcd8057 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -50,6 +50,7 @@
#include "storage/sinvaladt.h"
#include "storage/spin.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/snapmgr.h"
#include "utils/wait_event.h"
@@ -149,6 +150,7 @@ CalculateShmemSize(int *num_semaphores)
size = add_size(size, AsyncShmemSize());
size = add_size(size, StatsShmemSize());
size = add_size(size, WaitEventExtensionShmemSize());
+ size = add_size(size, InjectionPointShmemSize());
#ifdef EXEC_BACKEND
size = add_size(size, ShmemBackendArraySize());
#endif
@@ -351,6 +353,7 @@ CreateOrAttachShmemStructs(void)
AsyncShmemInit();
StatsShmemInit();
WaitEventExtensionShmemInit();
+ InjectionPointShmemInit();
}
/*
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index d621f5507f..0a0f02e055 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -55,3 +55,4 @@ WrapLimitsVacuumLock 46
NotifyQueueTailLock 47
WaitEventExtensionLock 48
WALSummarizerLock 49
+InjectionPointLock 50
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index f625473ad4..09aeb51bc8 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -329,6 +329,7 @@ WrapLimitsVacuum "Waiting to update limits on transaction id and multixact consu
NotifyQueueTail "Waiting to update limit on <command>NOTIFY</command> message storage."
WaitEventExtension "Waiting to read or update custom wait events information for extensions."
WALSummarizer "Waiting to read or update WAL summarization state."
+InjectionPoint "Waiting to read or update information related to injection points."
#
# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index c2971c7678..d9f59785b9 100644
--- a/src/backend/utils/misc/Makefile
+++ b/src/backend/utils/misc/Makefile
@@ -21,6 +21,7 @@ OBJS = \
guc_funcs.o \
guc_tables.o \
help_config.o \
+ injection_point.o \
pg_config.o \
pg_controldata.o \
pg_rusage.o \
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
new file mode 100644
index 0000000000..205dcfa31d
--- /dev/null
+++ b/src/backend/utils/misc/injection_point.c
@@ -0,0 +1,334 @@
+/*-------------------------------------------------------------------------
+ *
+ * injection_point.c
+ * Routines to control and run injection points in the code.
+ *
+ * Injection points can be used to call arbitrary callbacks in specific
+ * places of the code, attaching callbacks that would be run in the code
+ * paths where a named injection point exists.
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/utils/misc/injection_point.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include <sys/stat.h>
+
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "port/pg_bitutils.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/hsearch.h"
+#include "utils/injection_point.h"
+#include "utils/memutils.h"
+
+#ifdef USE_INJECTION_POINTS
+
+/*
+ * Hash table for storing injection points.
+ *
+ * InjectionPointHash is used to find an injection point by name.
+ */
+static HTAB *InjectionPointHash; /* find points from names */
+
+/* Field sizes */
+#define INJ_NAME_MAXLEN 64
+#define INJ_LIB_MAXLEN 128
+#define INJ_FUNC_MAXLEN 128
+
+/* Single injection point stored in InjectionPointHash */
+typedef struct InjectionPointEntry
+{
+ char name[INJ_NAME_MAXLEN]; /* hash key */
+ char library[INJ_LIB_MAXLEN]; /* library */
+ char function[INJ_FUNC_MAXLEN]; /* function */
+} InjectionPointEntry;
+
+#define INJECTION_POINT_HASH_INIT_SIZE 16
+#define INJECTION_POINT_HASH_MAX_SIZE 128
+
+/*
+ * Local cache of injection callbacks already loaded, stored in
+ * TopMemoryContext.
+ */
+typedef struct InjectionPointCacheEntry
+{
+ char name[INJ_NAME_MAXLEN];
+ InjectionPointCallback callback;
+} InjectionPointCacheEntry;
+
+static HTAB *InjectionPointCache = NULL;
+
+/*
+ * injection_point_cache_add
+ *
+ * Add an injection to the local cache.
+ */
+static void
+injection_point_cache_add(const char *name,
+ InjectionPointCallback callback)
+{
+ InjectionPointCacheEntry *entry;
+ bool found;
+
+ /* If first time, initialize */
+ if (InjectionPointCache == NULL)
+ {
+ HASHCTL hash_ctl;
+
+ hash_ctl.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+ hash_ctl.entrysize = sizeof(InjectionPointCacheEntry);
+ hash_ctl.hcxt = TopMemoryContext;
+
+ InjectionPointCache = hash_create("InjectionPoint cache hash",
+ INJECTION_POINT_HASH_MAX_SIZE,
+ &hash_ctl,
+ HASH_ELEM | HASH_STRINGS | HASH_CONTEXT);
+ }
+
+ entry = (InjectionPointCacheEntry *)
+ hash_search(InjectionPointCache, name, HASH_ENTER, &found);
+
+ Assert(!found);
+ memcpy(entry->name, name, strlen(name));
+ entry->callback = callback;
+}
+
+/*
+ * injection_point_cache_remove
+ *
+ * Remove entry from the local cache. Note that this leaks a callback
+ * loaded but removed later on, which should have no consequence from
+ * a testing perspective.
+ */
+static void
+injection_point_cache_remove(const char *name)
+{
+ /* Leave if no cache */
+ if (InjectionPointCache == NULL)
+ return;
+
+ (void) hash_search(InjectionPointCache, name, HASH_REMOVE, NULL);
+}
+
+/*
+ * injection_point_cache_get
+ *
+ * Retrieve an injection point from the local cache, if any.
+ */
+static InjectionPointCallback
+injection_point_cache_get(const char *name)
+{
+ bool found;
+ InjectionPointCacheEntry *entry;
+
+ /* no callback if no cache yet */
+ if (InjectionPointCache == NULL)
+ return NULL;
+
+ entry = (InjectionPointCacheEntry *)
+ hash_search(InjectionPointCache, name, HASH_FIND, &found);
+
+ if (found)
+ return entry->callback;
+
+ return NULL;
+}
+#endif /* USE_INJECTION_POINTS */
+
+/*
+ * Return the space for dynamic shared hash table.
+ */
+Size
+InjectionPointShmemSize(void)
+{
+#ifdef USE_INJECTION_POINTS
+ Size sz = 0;
+
+ sz = add_size(sz, hash_estimate_size(INJECTION_POINT_HASH_MAX_SIZE,
+ sizeof(InjectionPointEntry)));
+ return sz;
+#else
+ return 0;
+#endif
+}
+
+/*
+ * Allocate shmem space for dynamic shared hash.
+ */
+void
+InjectionPointShmemInit(void)
+{
+#ifdef USE_INJECTION_POINTS
+ HASHCTL info;
+
+ /* key is a NULL-terminated string */
+ info.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+ info.entrysize = sizeof(InjectionPointEntry);
+ InjectionPointHash = ShmemInitHash("InjectionPoint hash",
+ INJECTION_POINT_HASH_INIT_SIZE,
+ INJECTION_POINT_HASH_MAX_SIZE,
+ &info,
+ HASH_ELEM | HASH_FIXED_SIZE | HASH_STRINGS);
+#endif
+}
+
+#ifdef USE_INJECTION_POINTS
+static bool
+file_exists(const char *name)
+{
+ struct stat st;
+
+ Assert(name != NULL);
+ if (stat(name, &st) == 0)
+ return !S_ISDIR(st.st_mode);
+ else if (!(errno == ENOENT || errno == ENOTDIR))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not access file \"%s\": %m", name)));
+ return false;
+}
+#endif
+
+/*
+ * Attach a new injection point.
+ */
+void
+InjectionPointAttach(const char *name,
+ const char *library,
+ const char *function)
+{
+#ifdef USE_INJECTION_POINTS
+ InjectionPointEntry *entry_by_name;
+ bool found;
+
+ if (strlen(name) >= INJ_NAME_MAXLEN)
+ elog(ERROR, "injection point name %s too long (maximum of %u)",
+ name, INJ_NAME_MAXLEN);
+ if (strlen(library) >= INJ_LIB_MAXLEN)
+ elog(ERROR, "injection point library %s too long (maximum of %u)",
+ library, INJ_LIB_MAXLEN);
+ if (strlen(function) >= INJ_FUNC_MAXLEN)
+ elog(ERROR, "injection point function %s too long (maximum of %u)",
+ function, INJ_FUNC_MAXLEN);
+
+ /*
+ * Allocate and register a new injection point. A new point should not
+ * exist. For testing purposes this should be fine.
+ */
+ LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+ entry_by_name = (InjectionPointEntry *)
+ hash_search(InjectionPointHash, name,
+ HASH_ENTER, &found);
+ if (found)
+ {
+ LWLockRelease(InjectionPointLock);
+ elog(ERROR, "injection point \"%s\" already defined", name);
+ }
+
+ /* Save the entry */
+ memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
+ entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
+ memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
+ entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
+ memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
+ entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
+
+ LWLockRelease(InjectionPointLock);
+
+#else
+ elog(ERROR, "injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Detach an existing injection point.
+ */
+void
+InjectionPointDetach(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+ bool found;
+
+ LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+ hash_search(InjectionPointHash, name, HASH_REMOVE, &found);
+ LWLockRelease(InjectionPointLock);
+
+ if (!found)
+ elog(ERROR, "injection point \"%s\" not found", name);
+
+#else
+ elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Execute an injection point, if defined.
+ *
+ * Check first the shared hash table, and adapt the local cache depending
+ * on that as it could be possible that an entry to run has been removed.
+ */
+void
+InjectionPointRun(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+ InjectionPointEntry *entry_by_name;
+ bool found;
+ InjectionPointCallback injection_callback;
+
+ LWLockAcquire(InjectionPointLock, LW_SHARED);
+ entry_by_name = (InjectionPointEntry *)
+ hash_search(InjectionPointHash, name,
+ HASH_FIND, &found);
+ LWLockRelease(InjectionPointLock);
+
+ /*
+ * If not found, do nothing and remove it from the local cache if it
+ * existed there.
+ */
+ if (!found)
+ {
+ injection_point_cache_remove(name);
+ return;
+ }
+
+ /*
+ * Check if the callback exists in the local cache, to avoid unnecessary
+ * external loads.
+ */
+ injection_callback = injection_point_cache_get(name);
+ if (injection_callback == NULL)
+ {
+ char path[MAXPGPATH];
+
+ /* Not found in local cache, so load and register */
+ snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+ entry_by_name->library, DLSUFFIX);
+
+ if (!file_exists(path))
+ elog(ERROR, "could not find library \"%s\" for injection point \"%s\"",
+ path, name);
+
+ injection_callback = (InjectionPointCallback)
+ load_external_function(path, entry_by_name->function, true, NULL);
+
+ if (injection_callback == NULL)
+ elog(ERROR, "could not find function \"%s\" in library \"%s\" for injection point \"%s\"",
+ name, entry_by_name->function, path);
+
+ /* add it to the local cache when found */
+ injection_point_cache_add(name, injection_callback);
+ }
+
+ injection_callback(name);
+#else
+ elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
diff --git a/src/backend/utils/misc/meson.build b/src/backend/utils/misc/meson.build
index 581724f254..6669502205 100644
--- a/src/backend/utils/misc/meson.build
+++ b/src/backend/utils/misc/meson.build
@@ -6,6 +6,7 @@ backend_sources += files(
'guc_funcs.c',
'guc_tables.c',
'help_config.c',
+ 'injection_point.c',
'pg_config.c',
'pg_controldata.c',
'pg_rusage.c',
diff --git a/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/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/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 29fd1cae64..41f5ff8aa1 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.25.1
From 0b5fbe8368a5d35b0504d804b27675e8fa56dcd8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 9 Jan 2024 13:30:56 +0900
Subject: [PATCH 5/6] Add regression test to show snapbuild consistency
Reverting 409f9ca44713 causes the test to fail. The test added here
relies on the existing callbacks in injection_points.
---
src/backend/replication/logical/snapbuild.c | 3 ++
src/test/recovery/Makefile | 7 ++-
src/test/recovery/meson.build | 4 ++
src/test/recovery/t/040_snapshot_status.pl | 51 +++++++++++++++++++++
4 files changed, 64 insertions(+), 1 deletion(-)
create mode 100644 src/test/recovery/t/040_snapshot_status.pl
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index a0b7947d2f..93048afc2f 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -141,6 +141,7 @@
#include "storage/procarray.h"
#include "storage/standby.h"
#include "utils/builtins.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/snapmgr.h"
#include "utils/snapshot.h"
@@ -654,6 +655,8 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
snap->xcnt = newxcnt;
snap->xip = newxip;
+ INJECTION_POINT("SnapBuildInitialSnapshot");
+
return snap;
}
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index 17ee353735..f57baba5e8 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -9,12 +9,17 @@
#
#-------------------------------------------------------------------------
-EXTRA_INSTALL=contrib/pg_prewarm contrib/pg_stat_statements contrib/test_decoding
+EXTRA_INSTALL=contrib/pg_prewarm \
+ contrib/pg_stat_statements \
+ contrib/test_decoding \
+ src/test/modules/injection_points
subdir = src/test/recovery
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+export enable_injection_points enable_injection_points
+
# required for 017_shm.pl and 027_stream_regress.pl
REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
export REGRESS_SHLIB
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index 88fb0306f5..43d7411a79 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -6,6 +6,9 @@ tests += {
'bd': meson.current_build_dir(),
'tap': {
'test_kwargs': {'priority': 40}, # recovery tests are slow, start early
+ 'env': {
+ 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
+ },
'tests': [
't/001_stream_rep.pl',
't/002_archiving.pl',
@@ -45,6 +48,7 @@ tests += {
't/037_invalid_database.pl',
't/038_save_logical_slots_shutdown.pl',
't/039_end_of_wal.pl',
+ 't/040_snapshot_status.pl',
],
},
}
diff --git a/src/test/recovery/t/040_snapshot_status.pl b/src/test/recovery/t/040_snapshot_status.pl
new file mode 100644
index 0000000000..e77b138144
--- /dev/null
+++ b/src/test/recovery/t/040_snapshot_status.pl
@@ -0,0 +1,51 @@
+# Test consistent of initial snapshot data.
+
+# This requires a node with wal_level=logical combined with an injection
+# point that forces a failure when a snapshot is initially built with a
+# logical slot created.
+#
+# See bug https://postgr.es/m/CAFiTN-s0zA1Kj0ozGHwkYkHwa5U0zUE94RSc_g81WrpcETB5=w...@mail.gmail.com.
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('SnapBuildInitialSnapshot', 'error');");
+
+my $node_host = $node->host;
+my $node_port = $node->port;
+my $connstr_common = "host=$node_host port=$node_port";
+my $connstr_db = "$connstr_common replication=database dbname=postgres";
+
+# This requires a single session, with two commands.
+my $psql_session =
+ $node->background_psql('postgres', on_error_stop => 0,
+ extra_params => [ '-d', $connstr_db ]);
+my ($output, $ret) = $psql_session->query(
+ 'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+ok($ret != 0, "First CREATE_REPLICATION_SLOT fails on injected error");
+
+# Now remove the injected error and check that the second command works.
+$node->safe_psql('postgres',
+ "SELECT injection_points_detach('SnapBuildInitialSnapshot');");
+
+($output, $ret) = $psql_session->query(
+ 'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+ok(substr($output, 0, 4) eq 'slot',
+ "Second CREATE_REPLICATION_SLOT passes");
+
+done_testing();
--
2.25.1
From 16255c9531d6b59a5232ef07de3836e2ed0a36c8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 11 Jan 2024 13:10:52 +0900
Subject: [PATCH 6/6] 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 5944c41716..221937b24e 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -24,6 +24,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 bf947de0b7..9846a1a3b3 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,9 +27,47 @@
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 to be attached to an injection point. */
void
@@ -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(action, "notice") == 0)
function = "injection_notice";
+ else if (strcmp(mode, "wait") == 0)
+ function = "injection_wait";
else
elog(ERROR, "incorrect action \"%s\" for injection point creation", action);
@@ -80,6 +135,20 @@ injection_points_run(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+/*
+ * SQL function for waking a condition variable.
+ */
+PG_FUNCTION_INFO_V1(injection_points_wake);
+Datum
+injection_points_wake(PG_FUNCTION_ARGS)
+{
+ if (inj_state == NULL)
+ injection_init_shmem();
+
+ ConditionVariableBroadcast(&inj_state->wait_point);
+ PG_RETURN_VOID();
+}
+
/*
* SQL function for dropping an injection point.
*/
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index 43d7411a79..c12b7fcf97 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -49,6 +49,7 @@ tests += {
't/038_save_logical_slots_shutdown.pl',
't/039_end_of_wal.pl',
't/040_snapshot_status.pl',
+ 't/041_invalid_checkpoint_after_promote.pl',
],
},
}
diff --git a/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl
new file mode 100644
index 0000000000..886454d50c
--- /dev/null
+++ b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl
@@ -0,0 +1,138 @@
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep);
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+# initialize primary node
+my $node_primary = PostgreSQL::Test::Cluster->new('master');
+$node_primary->init(allows_streaming => 1);
+$node_primary->append_conf(
+ 'postgresql.conf', q[
+checkpoint_timeout = 30s
+log_checkpoints = on
+restart_after_crash = on
+]);
+$node_primary->start;
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+
+# setup a standby
+my $node_standby = PostgreSQL::Test::Cluster->new('standby1');
+$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1);
+$node_standby->start;
+
+# dummy table for the upcoming tests.
+$node_primary->safe_psql('postgres', 'checkpoint');
+$node_primary->safe_psql('postgres', 'CREATE TABLE prim_tab (a int);');
+
+# Register a injection point on the standby so as the follow-up
+# restart point running on it will wait.
+$node_primary->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+# Wait until the extension has been created on the standby
+$node_primary->wait_for_replay_catchup($node_standby);
+# This causes a restartpoint to wait on a standby.
+$node_standby->safe_psql('postgres',
+ "SELECT injection_points_attach('CreateRestartPoint', 'wait');");
+
+# Execute a restart point on the standby, that will be waited on.
+# This needs to be in the background as we'll wait on it.
+my $logstart = -s $node_standby->logfile;
+my $psql_session =
+ $node_standby->background_psql('postgres', on_error_stop => 0);
+$psql_session->query_until(qr/starting_checkpoint/, q(
+ \echo starting_checkpoint
+ CHECKPOINT;
+));
+
+# Switch one WAL segment to make the restartpoint remove it.
+$node_primary->safe_psql('postgres', 'INSERT INTO prim_tab VALUES (1);');
+$node_primary->safe_psql('postgres', 'SELECT pg_switch_wal();');
+$node_primary->wait_for_replay_catchup($node_standby);
+
+# Wait until the checkpointer is in the middle of the restartpoint
+# processing.
+ok( $node_standby->poll_query_until(
+ 'postgres',
+ qq[SELECT count(*) FROM pg_stat_activity
+ WHERE backend_type = 'checkpointer' AND wait_event = 'injection_wait' ;],
+ '1'),
+ 'checkpointer is waiting at restart point'
+ ) or die "Timed out while waiting for checkpointer to run restartpoint";
+
+
+# Restartpoint should have started on standby.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+ $checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');
+
+# promote during restartpoint
+$node_primary->stop;
+$node_standby->promote;
+
+# Update the start position before waking up the checkpointer!
+$logstart = -s $node_standby->logfile;
+
+# Now wake up the checkpointer
+$node_standby->safe_psql('postgres',
+ "SELECT injection_points_wake();");
+
+# wait until checkpoint completes on the newly-promoted standby.
+my $checkpoint_complete = 0;
+for (my $i = 0; $i < 3000; $i++)
+{
+ my $log = slurp_file($node_standby->logfile, $logstart);
+ if ($log =~ m/restartpoint complete/)
+ {
+ $checkpoint_complete = 1;
+ last;
+ }
+ usleep(100_000);
+}
+is($checkpoint_complete, 1, 'restartpoint has completed');
+
+# kill SIGKILL a backend, and all backend will restart. Note that previous
+# checkpoint has not completed.
+my $psql_timeout = IPC::Run::timer(3600);
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+ [ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_standby->connstr('postgres') ],
+ '<',
+ \$killme_stdin,
+ '>',
+ \$killme_stdout,
+ '2>',
+ \$killme_stderr,
+ $psql_timeout);
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/;
+my $pid = $killme_stdout;
+chomp($pid);
+my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid);
+is($ret, 0, 'killed process with KILL');
+my $stdout;
+my $stderr;
+
+# After recovery, the server should be able to start.
+for (my $i = 0; $i < 30; $i++)
+{
+ ($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1');
+ last if $ret == 0;
+ sleep(1);
+}
+is($ret, 0, "psql connect success");
+is($stdout, 1, "psql select 1");
+
+done_testing();
--
2.25.1
