On Tue, Feb 20, 2024 at 03:55:08PM +0000, Bertrand Drouvot wrote:
> Okay, makes sense to keep this as it is as a "template" in case more stuff is
> added.
>
> + /* Counter advancing when injection_points_wake() is called */
> + int wait_counts;
>
> In that case what about using an unsigned instead? (Nit)
uint32. Sure.
> 1 ===
>
> +void
> +injection_wait(const char *name)
>
> Looks like name is not used in the function. I guess the reason it is a parameter
> is because that's the way the callback function is being called in
> InjectionPointRun()?
Right. The callback has to define this argument.
> 2 ===
>
> +PG_FUNCTION_INFO_V1(injection_points_wake);
> +Datum
> +injection_points_wake(PG_FUNCTION_ARGS)
> +{
>
> I think that This function will wake up all the "wait" injection points.
> Would that make sense to implement some filtering based on the name? That could
> be useful for tests that would need multiple wait injection points and that want
> to wake them up "sequentially".
>
> We could think about it if there is such a need in the future though.
Well, both you and Andrey are asking for it now, so let's do it. The
implementation is simple:
- Store in InjectionPointSharedState an array of wait_counts and an
array of names. There is only one condition variable.
- When a point wants to wait, it takes the spinlock and looks within
the array of names until it finds a free slot, adds its name into the
array to reserve a wait counter at the same position, releases the
spinlock. Then it loops on the condition variable for an update of
the counter it has reserved. It is possible to make something more
efficient, but at a small size it would not really matter.
- The wakeup takes a point name in argument, acquires the spinlock,
and checks if it can find the point into the array, pinpoints the
location of the counter to update and updates it. Then it broadcasts
the change.
- The wait loop checks its counter, leaves its loop, cancels the
sleep, takes the spinlock to unregister from the array, and leaves.
I would just hardcode the number of points that can wait, say 5 of
them tracked in shmem? Does that look like what you are looking at?
> +# Register a injection point on the standby so as the follow-up
>
> typo: "an injection"?
Oops. Fixed locally.
> +for (my $i = 0; $i < 3000; $i++)
> +{
>
> is 3000 due to?:
>
> +checkpoint_timeout = 30s
>
> If so, would that make sense to reduce the value for both?
That had better be based on PostgreSQL::Test::Utils::timeout_default,
actually, as in something like:
foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default)
--
Michael
