Remove condition variables from injection wait logic.
$sub proposed in a nearby thread. Looks like we have a consensus that
$subj is beneficial.
I implemented necessary legwork, namely a clock-based check in the
wait() routine, PFA. I'm not sure the default pg_sleep argument of 50
millisecond is good, but it is fast enough to not spot any difference
in by-hand testing.
[0]: /messages/by-id/aKT7qD0VkGhQgFJe@paquier.xyz
--
Best regards,
Kirill Reshke
Attachments:
v1-0001-Remove-condition-variables-from-injection-wait-lo.patchapplication/octet-stream; name=v1-0001-Remove-condition-variables-from-injection-wait-lo.patchDownload
From cd9e5a5c01f98c4288802401ab84aeb116019231 Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Wed, 20 Aug 2025 05:22:38 +0000
Subject: [PATCH v1] Remove condition variables from injection wait logic.
In order to avoid process to exit too early as a responce to Postmaster
death (which will be undesirable for some type of testing),
replace CondVar logic with simple sleep call in injection test module.
---
src/test/modules/injection_points/injection_points.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 31138301117..7080a954cbd 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -87,8 +87,6 @@ typedef struct InjectionPointSharedState
/* Names of injection points attached to wait counters */
char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN];
- /* Condition variable used for waits and wakeups */
- ConditionVariable wait_point;
} InjectionPointSharedState;
/* Pointer to shared-memory state. */
@@ -132,7 +130,6 @@ injection_point_init_state(void *ptr)
SpinLockInit(&state->lock);
memset(state->wait_counts, 0, sizeof(state->wait_counts));
memset(state->name, 0, sizeof(state->name));
- ConditionVariableInit(&state->wait_point);
}
/* Shared memory initialization when loading module */
@@ -323,7 +320,6 @@ injection_wait(const char *name, const void *private_data, void *arg)
name);
/* And sleep.. */
- ConditionVariablePrepareToSleep(&inj_state->wait_point);
for (;;)
{
uint32 new_wait_counts;
@@ -334,9 +330,11 @@ injection_wait(const char *name, const void *private_data, void *arg)
if (old_wait_counts != new_wait_counts)
break;
- ConditionVariableSleep(&inj_state->wait_point, injection_wait_event);
+
+ pgstat_report_wait_start(injection_wait_event);
+#define DEFAULT_INJ_POINT_SLEEP_MICROSEC 50000L /* 50 milliseconds */
+ pg_usleep(DEFAULT_INJ_POINT_SLEEP_MICROSEC);
}
- ConditionVariableCancelSleep();
/* Remove this injection point from the waiters. */
SpinLockAcquire(&inj_state->lock);
@@ -486,8 +484,6 @@ injection_points_wakeup(PG_FUNCTION_ARGS)
inj_state->wait_counts[index]++;
SpinLockRelease(&inj_state->lock);
- /* And broadcast the change to the waiters */
- ConditionVariableBroadcast(&inj_state->wait_point);
PG_RETURN_VOID();
}
--
2.43.0
On Wed, Aug 20, 2025 at 11:20:11AM +0500, Kirill Reshke wrote:
$sub proposed in a nearby thread. Looks like we have a consensus that
$subj is beneficial.
I implemented necessary legwork, namely a clock-based check in the
wait() routine, PFA. I'm not sure the default pg_sleep argument of 50
millisecond is good, but it is fast enough to not spot any difference
in by-hand testing.
I may be missing something, but I don't think that we have reached a
consensus yet. There is the argument of AIO and being able to
broadcast writes.
+ pgstat_report_wait_start(injection_wait_event);
+#define DEFAULT_INJ_POINT_SLEEP_MICROSEC 50000L /* 50 milliseconds */
+ pg_usleep(DEFAULT_INJ_POINT_SLEEP_MICROSEC);I would not object to that if that's the actual consensus as we don't
have a strong requirement for condition variables when it comes to
testing. That's just a more efficient implementation, and it makes
the tests faster. If we do that, I'd suggest to choose a cap and a
variable wait time, that increases across iterations to still make the
wait more responsive on faster machines.
Your patch lacks a pgstat_report_wait_end().
--
Michael
On 21 Aug 2025, at 04:02, Michael Paquier <michael@paquier.xyz> wrote:
I would not object to that if that's the actual consensus as we don't
have a strong requirement for condition variables when it comes to
testing. That's just a more efficient implementation, and it makes
the tests faster. If we do that, I'd suggest to choose a cap and a
variable wait time, that increases across iterations to still make the
wait more responsive on faster machines.
I want to do a test for suspected VM corruption (1).
I need a way to do injection point that can be kill-9-ed without corruption.
So I can just use Kirill's patch to develop my test, thanks! I do not need it committed until the work is over.
So far there are no tests in the tree that need this functionality in injection points.
And even when we will have such a test that needs this kind of sleep, it is only required if injection point is in critical section. Not for every injection point wait.
Also, CondVar might be fixed and allowed to be used in critical section (2). AIO needs it anyway.
Let's wait for (1) or (2), then decide if we need to do something with injection point waiting.
Thanks you both for working on these tools!
Best regards, Andrey Borodin.