Hi, On Mon, Feb 19, 2024 at 04:51:45PM +0900, Michael Paquier wrote: > On Mon, Feb 19, 2024 at 03:01:40PM +0900, Michael Paquier wrote: > > 0002 is a polished version of the TAP test that makes use of this > > facility, providing coverage for the bug fixed by 7863ee4def65 > > (reverting this commit causes the test to fail), where a restart point > > runs across a promotion request. The trick is to stop the > > checkpointer in the middle of a restart point and issue a promotion > > in-between. > > The CF bot has been screaming at this one on Windows because the > process started with IPC::Run::start was not properly finished, so > attached is an updated version to bring that back to green.
Thanks for the patch, that's a very cool feature! Random comments: 1 === +CREATE FUNCTION injection_points_wake() what about injection_points_wakeup() instead? 2 === +/* Shared state information for injection points. */ +typedef struct InjectionPointSharedState +{ + /* protects accesses to wait_counts */ + slock_t lock; + + /* Counter advancing when injection_points_wake() is called */ + int wait_counts; If slock_t protects "only" one counter, then what about using pg_atomic_uint64 or pg_atomic_uint32 instead? And btw do we need wait_counts at all? (see comment number 4) 3 === + * SQL function for waking a condition variable waking up instead? 4 === + for (;;) + { + int new_wait_counts; + + SpinLockAcquire(&inj_state->lock); + new_wait_counts = inj_state->wait_counts; + SpinLockRelease(&inj_state->lock); + + if (old_wait_counts != new_wait_counts) + break; + ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + } I'm wondering if this loop and wait_counts are needed, why not doing something like (and completely get rid of wait_counts)? " ConditionVariablePrepareToSleep(&inj_state->wait_point); ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); ConditionVariableCancelSleep(); " It's true that the comment above ConditionVariableSleep() mentions that: " * This should be called in a predicate loop that tests for a specific exit * condition and otherwise sleeps " but is it needed in our particular case here? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com