On Mon, Aug 10, 2026 at 05:31:00PM -0500, Sami Imseih wrote: > The facilities that this will be useful for are those in which the > injection_point_set_local() > cannot be used and thus we cannot clearly determine if the injection > point is reached > at the correct condition. [0] was one example, which led to this > thread, but we dealt > with this by using wait_for_log to ensure that we reached the correct code > path.
0001 feels overall OK. I just have a few comments.
+-- injection point condition string matching.
+SELECT injection_points_attach('TestConditionString', 'notice', 'MyString');
+SELECT injection_points_run('TestConditionString', 'MyString'); -- notice
+SELECT injection_points_run('TestConditionString', 'WrongString'); -- nothing
+SELECT injection_points_run('TestConditionString', NULL); -- nothing
+SELECT injection_points_detach('TestConditionString');
This is a duplicate set of tests with the local case. Let's trim the
cases a bit a drop this part.
SELECT injection_points_run('TestConditionLocal2'); -- nothing
+SELECT injection_points_run('TestConditionLocalString', 'LocalData'); --
nothing
This one also to check that the local point has been dropped is not
really required to me: we already make sure that a bunch of the other
points have been detached.
CREATE FUNCTION injection_points_attach(IN point_name TEXT,
[ ... ]
-LANGUAGE C STRICT PARALLEL UNSAFE;
+LANGUAGE C PARALLEL UNSAFE;
Okay with the strictness here.
INJ_CONDITION_ALWAYS = 0, /* always run */
- INJ_CONDITION_PID, /* PID restriction */
+ INJ_CONDITION_PID = 1 << 0, /* PID restriction */
+ INJ_CONDITION_STRING = 1 << 1, /* generic string match against arg */
Hmm. Could it be better to rename "ALWAYS" to "NONE" then? That
would feel less confusing to me after switching to a bitmask as we may
want to apply multiple conditions.
> Looking at [1], if we want to test toast reloptions for autovacuum, we
> will need some type
> of way to ensure that we are inspecting the correct relations for the
> test. Since
> autovacuum cannot rely on injection_point_set_local because workers will not
> set the local injection point, we can use string conditions to ensure
> we reached the
> correct table. the nocfbot test attached shows how we can build such tests for
> autovacuum. The test fails without the latest patchset in [1]. CCing Nathan as
> his patch is being discussed.
+# A TOAST relation inherits vacuum reloptions from its main table when it has
+# no toast.* value of its own. Condition strings key each wait to the TOAST
+# relation, so its resolution is observed on its own.
Better perhaps to wait for Nathan's input on how this would help.
That depends on the resulting commit merged into the tree. But I
don't think that we should, see below for extra ideas.
> Without this condition string, to implement such a test, we will need
> a NOTICE to log
> and we will need to use regexp to examine the log to make sure our
> toast table fired
> at the correct injection point. With the condition string, we can
> simply just use
> wait_for_event since we know which table the injection point will fire on.
So your argument is that we lack context data regarding these GUCs in
a non-backend context. Why not.
While I was tweaking with the stats code, I got a little bit annoyed
by the timing of the flushes. So one idea I could think about is a
point in a vacuum report, where we could play with a VACUUM command
and some partitions, checking that some relations trigger or not?
But actually, there is even simpler.. As one example, in 051 for
Sawada-san's effective WAL level, we have some local points that could
be made more efficient by targetting only the slot name we want. So
we could use this new facility to make the tests more surgical in the
way the points are run. Perhaps that sounds a bit pedantic, but
something like the attached would be enough for me as a starting
point. We don't really have to be ambitious in the first step, and we
could expand that to tighten as well some of the PID-based checks,
perhaps, for the checkpointer or the startup process. Basic idea is
attached.
--
Michael
From 44c1f85e696d53bb6e60ef155db1c3d787cf1f9c Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 13 Aug 2026 13:11:43 +0900 Subject: [PATCH] Add new string-based wait in 051_effective_wal_level.pl This serves as a demonstration for the new injpoint API. --- src/backend/replication/slot.c | 2 +- src/test/recovery/t/051_effective_wal_level.pl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 1a0ff6820689..63ce6d278854 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -420,7 +420,7 @@ ReplicationSlotCreate(const char *name, bool db_specific, errmsg("cannot enable failover for a temporary replication slot")); } - INJECTION_POINT("replication-slot-create-begin", NULL); + INJECTION_POINT("replication-slot-create-begin", (char *) name); /* * If some other backend ran this code concurrently with us, we'd likely diff --git a/src/test/recovery/t/051_effective_wal_level.pl b/src/test/recovery/t/051_effective_wal_level.pl index 11bf4cb61345..b8b48838afeb 100644 --- a/src/test/recovery/t/051_effective_wal_level.pl +++ b/src/test/recovery/t/051_effective_wal_level.pl @@ -622,7 +622,7 @@ select pg_sync_replication_slots(); qr/create_standby5_slot/, q(\echo create_standby5_slot select injection_points_set_local(); -select injection_points_attach('replication-slot-create-begin', 'wait'); +select injection_points_attach('replication-slot-create-begin', 'wait', 'standby5_slot'); select pg_create_logical_replication_slot('standby5_slot', 'test_decoding'); )); $standby5->wait_for_event('client backend', -- 2.55.0
signature.asc
Description: PGP signature
