Re: Injection points: some tools to wait and wake

Hi,

On Wed, Feb 21, 2024 at 04:46:00PM +0900, Michael Paquier wrote:
> On Wed, Feb 21, 2024 at 07:08:03AM +0900, Michael Paquier wrote:
> > 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?
> 
> I was looking at that, and it proves to work OK, so you can do stuff
> like waits and wakeups for multiple processes in a controlled manner.
> The attached patch authorizes up to 32 waiters.  I have switched
> things so as the information reported in pg_stat_activity is the name
> of the injection point itself.

Thanks!

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).

A few comments:

1 ===

+-- Wakes a condition variable

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.

2 ===

+       /* Counters advancing when injection_points_wakeup() is called */
+       int                     wait_counts[INJ_MAX_WAIT];

uint32? (here and other places where counter is manipulated).

3 ===

+       /* Remove us from the waiting list */

"Remove this injection wait name from the waiting list" instead?

4 ===

+ * SQL function for waking a condition variable.

s/a condition variable/an injection wait point/ ?

5 ===

+PG_FUNCTION_INFO_V1(injection_points_wakeup);
+Datum
+injection_points_wakeup(PG_FUNCTION_ARGS)

Empty line missing before "Datum"?

6 ===

Also maybe some comments are missing above injection_point_init_state(), 
injection_init_shmem() but it's more a Nit.

7 ===

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?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com