On Mon, May 26, 2025 at 01:17:46PM +0530, Rahila Syed wrote: > It appears that Github CI is reporting failures with > injection_points/002_data_persist > failing across all OSes.
I am not sure to see what you are referring to here, based on the following reports: https://commitfest.postgresql.org/patch/5731/ https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5731 http://cfbot.cputube.org/highlights/all.html The last failure reported that I know of was due to the addition of the runtime arguments to the macro INJECTION_POINT(). > I wonder if it would be better to include a check for the return value of > `durable_rename` to > manage potential failure scenarios. Or we can be smarter and make this call an ERROR. > 2. + > + file = AllocateFile(INJ_DUMP_FILE ".tmp", PG_BINARY_W); > > Could we perhaps add a brief comment clarifying the rationale behind the > use of a > temporary file in this context? Sure. This is about avoiding the load of incomplete files if there's a crash in the middle of a flush call, for example. > It seems that `buf` should be null-terminated before being passed to > `pstrdup(buf)`. > Otherwise, `strlen` might not accurately determine the length. This seems > to be > consistent with the handling of `fread` calls in `snapmgr.c`. Hmm, good point. That would be safer, even if the write part should already push the null termination. -- Michael
From fe86d0066c009affbe312bc528673e5d032bdfad Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 22 Apr 2025 12:59:30 +0900
Subject: [PATCH v3 1/3] Add InjectionPointList() to retrieve list of injection
points
This hides the internals of the shmem array lookup, allocating the
result in a palloc'd array usable by the caller.
---
src/include/utils/injection_point.h | 16 +++++++++
src/backend/utils/misc/injection_point.c | 46 ++++++++++++++++++++++++
src/tools/pgindent/typedefs.list | 1 +
3 files changed, 63 insertions(+)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index a37958e1835f..fd5bc061b7bd 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -11,6 +11,19 @@
#ifndef INJECTION_POINT_H
#define INJECTION_POINT_H
+#include "nodes/pg_list.h"
+
+/*
+ * Injection point data, used when retrieving a list of all the attached
+ * injection points.
+ */
+typedef struct InjectionPointData
+{
+ const char *name;
+ const char *library;
+ const char *function;
+} InjectionPointData;
+
/*
* Injection points require --enable-injection-points.
*/
@@ -47,6 +60,9 @@ extern void InjectionPointCached(const char *name, void *arg);
extern bool IsInjectionPointAttached(const char *name);
extern bool InjectionPointDetach(const char *name);
+/* Get the current set of injection points attached */
+extern List *InjectionPointList(void);
+
#ifdef EXEC_BACKEND
extern PGDLLIMPORT struct InjectionPointsCtl *ActiveInjectionPoints;
#endif
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index f58ebc8ee522..12570fba56e4 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -584,3 +584,49 @@ IsInjectionPointAttached(const char *name)
return false; /* silence compiler */
#endif
}
+
+/*
+ * Retrieve a list of all the injection points currently attached.
+ *
+ * This list is palloc'd in the current memory context.
+ */
+List *
+InjectionPointList(void)
+{
+#ifdef USE_INJECTION_POINTS
+ List *inj_points = NIL;
+ uint32 max_inuse;
+
+ LWLockAcquire(InjectionPointLock, LW_SHARED);
+
+ max_inuse = pg_atomic_read_u32(&ActiveInjectionPoints->max_inuse);
+
+ for (uint32 idx = 0; idx < max_inuse; idx++)
+ {
+ InjectionPointEntry *entry;
+ InjectionPointData *inj_point;
+ uint64 generation;
+
+ entry = &ActiveInjectionPoints->entries[idx];
+ generation = pg_atomic_read_u64(&entry->generation);
+
+ /* skip free slots */
+ if (generation % 2 == 0)
+ continue;
+
+ inj_point = (InjectionPointData *) palloc0(sizeof(InjectionPointData));
+ inj_point->name = pstrdup(entry->name);
+ inj_point->library = pstrdup(entry->library);
+ inj_point->function = pstrdup(entry->function);
+ inj_points = lappend(inj_points, inj_point);
+ }
+
+ LWLockRelease(InjectionPointLock);
+
+ return inj_points;
+
+#else
+ elog(ERROR, "Injection points are not supported by this build");
+ return NIL; /* keep compiler quiet */
+#endif
+}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index a8346cda633a..4ad6fdb0d003 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1283,6 +1283,7 @@ InjectionPointCacheEntry
InjectionPointCallback
InjectionPointCondition
InjectionPointConditionType
+InjectionPointData
InjectionPointEntry
InjectionPointsCtl
InjectionPointSharedState
--
2.49.0
From bfb448567fdf3fce5cc7b459468ca379b40a0b04 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 27 May 2025 09:48:39 +0900
Subject: [PATCH v3 2/3] injection_points: Add function to flush injection
point data to disk
The function injection_points_flush() can be called in a session to
persist to disk all the injection points currently attached to the
cluster. These are saved in file called injection_points.data at the
root of PGDATA, and loaded by the cluster at startup with all the points
registered automatically attached.
This will be useful to test scenarios where injection points need to be
for example attached at very early stages of startup, before it is
possible to attach anything via SQL.
---
.../injection_points--1.0.sql | 10 ++
.../injection_points/injection_points.c | 170 ++++++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../injection_points/t/002_data_persist.pl | 53 ++++++
4 files changed, 234 insertions(+)
create mode 100644 src/test/modules/injection_points/t/002_data_persist.pl
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 cc76b1bf99ae..966e1342e4ae 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -3,6 +3,16 @@
-- 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_flush()
+--
+-- Flush to disk all the data of the injection points attached.
+--
+CREATE FUNCTION injection_points_flush()
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_flush'
+LANGUAGE C STRICT;
+
--
-- injection_points_attach()
--
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 3da0cbc10e08..ba039cfb0ca7 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -24,6 +24,7 @@
#include "nodes/value.h"
#include "storage/condition_variable.h"
#include "storage/dsm_registry.h"
+#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/lwlock.h"
#include "storage/shmem.h"
@@ -39,6 +40,14 @@ PG_MODULE_MAGIC;
#define INJ_MAX_WAIT 8
#define INJ_NAME_MAXLEN 64
+/* Location of injection point data files, if flush has been requested */
+#define INJ_DUMP_FILE "injection_points.data"
+#define INJ_DUMP_FILE_TMP INJ_DUMP_FILE ".tmp"
+
+/* Magic number identifying the injection file */
+static const uint32 INJ_FILE_HEADER = 0xFF345678;
+
+
/*
* Conditions related to injection points. This tracks in shared memory the
* runtime conditions under which an injection point is allowed to run,
@@ -151,6 +160,9 @@ static void
injection_shmem_startup(void)
{
bool found;
+ int32 num_inj_points;
+ uint32 header;
+ FILE *file;
if (prev_shmem_startup_hook)
prev_shmem_startup_hook();
@@ -172,6 +184,87 @@ injection_shmem_startup(void)
}
LWLockRelease(AddinShmemInitLock);
+
+ /*
+ * Done if some other process already completed the initialization.
+ */
+ if (found)
+ return;
+
+ /*
+ * Note: there should be no need to bother with locks here, because there
+ * should be no other processes running when this code is reached.
+ */
+
+ /* Load injection point data, if any has been found while starting up */
+ file = AllocateFile(INJ_DUMP_FILE, PG_BINARY_R);
+
+ if (file == NULL)
+ {
+ if (errno != ENOENT)
+ goto error;
+
+ /* No file? We are done. */
+ return;
+ }
+
+ if (fread(&header, sizeof(uint32), 1, file) != 1 ||
+ fread(&num_inj_points, sizeof(int32), 1, file) != 1)
+ goto error;
+
+ if (header != INJ_FILE_HEADER)
+ goto error;
+
+ for (int i = 0; i < num_inj_points; i++)
+ {
+ const char *name;
+ const char *library;
+ const char *function;
+ uint32 len;
+ char buf[1024];
+
+ if (fread(&len, sizeof(uint32), 1, file) != 1)
+ goto error;
+ if (fread(buf, 1, len + 1, file) != len + 1)
+ goto error;
+ buf[len] = '\0';
+ name = pstrdup(buf);
+
+ if (fread(&len, sizeof(uint32), 1, file) != 1)
+ goto error;
+ if (fread(buf, 1, len + 1, file) != len + 1)
+ goto error;
+ buf[len] = '\0';
+ library = pstrdup(buf);
+
+ if (fread(&len, sizeof(uint32), 1, file) != 1)
+ goto error;
+ if (fread(buf, 1, len + 1, file) != len + 1)
+ goto error;
+ buf[len] = '\0';
+ function = pstrdup(buf);
+
+ /* No private data handled here */
+ InjectionPointAttach(name, library, function, NULL, 0);
+ }
+
+ /*
+ * Remove the persisted injection point file, we do not need it anymore.
+ */
+ unlink(INJ_DUMP_FILE);
+ FreeFile(file);
+
+ return;
+
+error:
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m",
+ INJ_DUMP_FILE)));
+ if (file)
+ FreeFile(file);
+
+ unlink(INJ_DUMP_FILE);
}
/*
@@ -343,6 +436,83 @@ injection_wait(const char *name, const void *private_data, void *arg)
SpinLockRelease(&inj_state->lock);
}
+/*
+ * SQL function for flushing injection point data to disk.
+ */
+PG_FUNCTION_INFO_V1(injection_points_flush);
+Datum
+injection_points_flush(PG_FUNCTION_ARGS)
+{
+ FILE *file = NULL;
+ List *inj_points = NIL;
+ ListCell *lc;
+ int32 num_inj_points;
+
+ inj_points = InjectionPointList();
+ if (inj_points == NIL)
+ PG_RETURN_VOID();
+
+ num_inj_points = list_length(inj_points);
+
+ /*
+ * The injection point data is written to a temporary file renamed to a
+ * final file to avoid incomplete files that could be loaded by backends.
+ */
+ file = AllocateFile(INJ_DUMP_FILE ".tmp", PG_BINARY_W);
+ if (file == NULL)
+ goto error;
+
+ if (fwrite(&INJ_FILE_HEADER, sizeof(uint32), 1, file) != 1)
+ goto error;
+
+ if (fwrite(&num_inj_points, sizeof(int32), 1, file) != 1)
+ goto error;
+
+ foreach(lc, inj_points)
+ {
+ InjectionPointData *inj_point = lfirst(lc);
+ uint32 len;
+
+ len = strlen(inj_point->name);
+ if (fwrite(&len, sizeof(uint32), 1, file) != 1 ||
+ fwrite(inj_point->name, 1, len + 1, file) != len + 1)
+ goto error;
+
+ len = strlen(inj_point->library);
+ if (fwrite(&len, sizeof(uint32), 1, file) != 1 ||
+ fwrite(inj_point->library, 1, len + 1, file) != len + 1)
+ goto error;
+
+ len = strlen(inj_point->function);
+ if (fwrite(&len, sizeof(uint32), 1, file) != 1 ||
+ fwrite(inj_point->function, 1, len + 1, file) != len + 1)
+ goto error;
+ }
+
+ if (FreeFile(file))
+ {
+ file = NULL;
+ goto error;
+ }
+
+ /*
+ * Rename file into place, so we atomically replace any old one.
+ */
+ durable_rename(INJ_DUMP_FILE_TMP, INJ_DUMP_FILE, ERROR);
+
+ PG_RETURN_VOID();
+
+error:
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not write file \"%s\": %m",
+ INJ_DUMP_FILE_TMP)));
+ if (file)
+ FreeFile(file);
+ unlink(INJ_DUMP_FILE_TMP);
+ PG_RETURN_VOID();
+}
+
/*
* SQL function for creating an injection point.
*/
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index d61149712fd7..b994f8d413d3 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -56,6 +56,7 @@ tests += {
},
'tests': [
't/001_stats.pl',
+ 't/002_data_persist.pl',
],
},
}
diff --git a/src/test/modules/injection_points/t/002_data_persist.pl b/src/test/modules/injection_points/t/002_data_persist.pl
new file mode 100644
index 000000000000..9ecb05230931
--- /dev/null
+++ b/src/test/modules/injection_points/t/002_data_persist.pl
@@ -0,0 +1,53 @@
+
+# Copyright (c) 2024-2025, PostgreSQL Global Development Group
+
+# Tests for persistence of injection point data.
+
+use strict;
+use warnings FATAL => 'all';
+use locale;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Test persistency of statistics generated for injection points.
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+# Node initialization
+my $node = PostgreSQL::Test::Cluster->new('master');
+$node->init;
+$node->append_conf(
+ 'postgresql.conf', qq(
+shared_preload_libraries = 'injection_points'
+));
+$node->start;
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+
+# Attach a couple of points, which are going to be made persistent.
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('persist-notice', 'notice');");
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('persist-error', 'error');");
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('persist-notice-2', 'notice');");
+
+# Flush and restart, the injection points still exist.
+$node->safe_psql('postgres', "SELECT injection_points_flush();");
+$node->restart;
+
+my ($result, $stdout, $stderr) =
+ $node->psql('postgres', "SELECT injection_points_run('persist-notice-2')");
+ok( $stderr =~
+ /NOTICE: notice triggered for injection point persist-notice-2/,
+ "injection point triggering NOTICE exists");
+
+($result, $stdout, $stderr) =
+ $node->psql('postgres', "SELECT injection_points_run('persist-error')");
+ok($stderr =~ /ERROR: error triggered for injection point persist-error/,
+ "injection point triggering ERROR exists");
+
+done_testing();
--
2.49.0
From c1556b972686dea8e748956220a0960516a4cbb3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 21 May 2025 17:15:19 +0900
Subject: [PATCH v3 3/3] Add regression test for 2PC visibility check on
standby
This adds some test coverage for a defect fixed in 2e57790836c6, where
the only reliable test back then was to use a hardcoded sleep().
This test relies on an injection point that is persisted across a node
restart, so as it is possible to cause the checkpointer to wait when
configuring the shared memory state of shared_preload_libraries at a
very early startup stage.
Reverting 2e57790836c6 causes the test to fail, and it passes on HEAD.
---
src/backend/replication/syncrep.c | 3 ++
src/test/recovery/t/009_twophase.pl | 60 +++++++++++++++++++++++++++++
2 files changed, 63 insertions(+)
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index cc35984ad008..e2be2603d1ce 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -84,6 +84,7 @@
#include "storage/proc.h"
#include "tcop/tcopprot.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/ps_status.h"
/* User-settable parameters for sync rep */
@@ -968,6 +969,8 @@ SyncRepUpdateSyncStandbysDefined(void)
if (sync_standbys_defined !=
((WalSndCtl->sync_standbys_status & SYNC_STANDBY_DEFINED) != 0))
{
+ INJECTION_POINT("checkpointer-syncrep-update", NULL);
+
LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
/*
diff --git a/src/test/recovery/t/009_twophase.pl b/src/test/recovery/t/009_twophase.pl
index 1a662ebe499d..f14da1549bac 100644
--- a/src/test/recovery/t/009_twophase.pl
+++ b/src/test/recovery/t/009_twophase.pl
@@ -51,6 +51,27 @@ $node_paris->append_conf(
));
$node_paris->start;
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+my $injection_points_supported =
+ $node_london->check_extension('injection_points');
+if ($injection_points_supported != 0)
+{
+ $node_london->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+
+ # Set shared_preload_libraries, to allow the injection points to persist
+ # across restarts.
+ $node_london->append_conf(
+ 'postgresql.conf', qq(
+ shared_preload_libraries = 'injection_points'
+ ));
+ $node_paris->append_conf(
+ 'postgresql.conf', qq(
+ shared_preload_libraries = 'injection_points'
+ ));
+}
+
# Switch to synchronous replication in both directions
configure_and_reload($node_london, "synchronous_standby_names = 'paris'");
configure_and_reload($node_paris, "synchronous_standby_names = 'london'");
@@ -327,6 +348,23 @@ $cur_primary->psql(
INSERT INTO t_009_tbl_standby_mvcc VALUES (2, 'issued to ${cur_primary_name}');
PREPARE TRANSACTION 'xact_009_standby_mvcc';
");
+
+# Attach an injection point to wait in the checkpointer when configuring
+# the shared memory state data related to synchronous_standby_names, then
+# persist the attached point to disk so as the follow-up restart will be able
+# to wait at the early stages of the checkpointer startup sequence.
+#
+# Note that as the checkpointer has already applied the
+# synchronous_standby_names configuration, this has no effect until the
+# next startup of the primary.
+if ($injection_points_supported != 0)
+{
+ $cur_primary->psql('postgres',
+ "SELECT injection_points_attach('checkpointer-syncrep-update', 'wait')"
+ );
+ $cur_primary->psql('postgres', "SELECT injection_points_flush()");
+}
+
$cur_primary->stop;
$cur_standby->restart;
@@ -341,6 +379,16 @@ is($psql_out, '0',
# Commit the transaction in primary
$cur_primary->start;
+
+# Make sure that the checkpointer is waiting before setting up the data of
+# synchronous_standby_names in shared memory. We want the checkpointer to be
+# stuck and make sure that the next COMMIT PREPARED is detected correctly on
+# the standby when remote_apply is set on the primary.
+if ($injection_points_supported != 0)
+{
+ $cur_primary->wait_for_event('checkpointer',
+ 'checkpointer-syncrep-update');
+}
$cur_primary->psql(
'postgres', "
SET synchronous_commit='remote_apply'; -- To ensure the standby is caught up
@@ -361,6 +409,18 @@ is($psql_out, '2',
"Committed prepared transaction is visible to new snapshot in standby");
$standby_session->quit;
+# Remove the injection point, the checkpointer now applies the configuration
+# related to synchronous_standby_names in shared memory.
+if ($injection_points_supported != 0)
+{
+ $cur_primary->psql('postgres',
+ "SELECT injection_points_wakeup('checkpointer-syncrep-update')");
+ $cur_primary->psql('postgres',
+ "SELECT injection_points_detach('checkpointer-syncrep-update')");
+}
+
+$cur_standby->restart;
+
###############################################################################
# Check for a lock conflict between prepared transaction with DDL inside and
# replay of XLOG_STANDBY_LOCK wal record.
--
2.49.0
signature.asc
Description: PGP signature
