On Wed, Feb 21, 2024 at 11:50:21AM +0000, Bertrand Drouvot wrote:
> I think the approach is fine and the hardcoded value is "large" enough (it would
> be surprising, at least to me, to write a test that would need more than 32
> waiters).
This could use less. I've never used more than 3 of them in a single
test, and that was already very complex to follow.
> A few comments:
>
> 1 ===
> I think "up" is missing at several places in the patch where "wake" is used.
> I could be wrong as non native english speaker though.
Patched up three places to be more consisten.
> 2 ===
>
> + /* Counters advancing when injection_points_wakeup() is called */
> + int wait_counts[INJ_MAX_WAIT];
>
> uint32? (here and other places where counter is manipulated).
Okay, why not.
> "Remove this injection wait name from the waiting list" instead?
> s/a condition variable/an injection wait point/ ?
Okay.
> 5 ===
>
> +PG_FUNCTION_INFO_V1(injection_points_wakeup);
> +Datum
> +injection_points_wakeup(PG_FUNCTION_ARGS)
>
> Empty line missing before "Datum"?
I've used the same style as anywhere else.
> Also maybe some comments are missing above injection_point_init_state(),
> injection_init_shmem() but it's more a Nit.
Sure.
> While at it, should we add a second injection wait point in
> t/041_invalid_checkpoint_after_promote.pl to check that they are wake up
> individually?
I'm not sure that's a good idea for the sake of this test, TBH, as
that would provide coverage outside the original scope of the
restartpoint/promote check.
I have also looked at if it would be possible to get an isolation test
out of that, but the arbitrary wait does not make that possible with
the existing isolation APIs, see GetSafeSnapshotBlockingPids() used in
pg_isolation_test_session_is_blocked(). isolation/README seems to be
a bit off here, actually, mentioning pg_locks.. We could use some
tricks with transactions or locks internally, but I'm not really
tempted to make the wait callback more complicated for the sake of
more coverage as I'd rather keep it generic for anybody who wants to
control the order of events across processes.
Attaching a v3 for reference with everything that has been mentioned
up to now.
--
Michael