Re: Minimal logical decoding on standbys - Mailing list pgsql-hackers
From | Drouvot, Bertrand |
---|---|
Subject | Re: Minimal logical decoding on standbys |
Date | |
Msg-id | f7e68298-dfb2-888d-016e-8cb8a9965a8b@gmail.com Whole thread Raw |
In response to | Re: Minimal logical decoding on standbys ("Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>) |
List | pgsql-hackers |
Hi, On 3/8/23 11:25 AM, Drouvot, Bertrand wrote: > Hi, > > On 3/3/23 5:26 PM, Drouvot, Bertrand wrote: >> Hi, >> >> On 3/3/23 8:58 AM, Jeff Davis wrote: >>> On Thu, 2023-03-02 at 11:45 -0800, Jeff Davis wrote: >>>> In this case it looks easier to add the right API than to be sure >>>> about >>>> whether it's needed or not. >>> >>> I attached a sketch of one approach. >> >> Oh, that's very cool, thanks a lot! >> >>> I'm not very confident that it's >>> the right API or even that it works as I intended it, but if others >>> like the approach I can work on it some more. >>> >> >> I'll look at it early next week. >> > > So, I took your patch and as an example I tried a quick integration in 0004, > (see 0004_new_API.txt attached) to put it in the logical decoding on standby context. > > Based on this, I've 3 comments: > > - Maybe ConditionVariableEventSleep() should take care of the “WaitEventSetWait returns 1 and cvEvent.event == WL_POSTMASTER_DEATH”case? > > - Maybe ConditionVariableEventSleep() could accept and deal with the CV being NULL? > I used it in the POC attached to handle logical decoding on the primary server case. > One option should be to create a dedicated CV for that case though. > > - In the POC attached I had to add this extra condition “(cv && !RecoveryInProgress())” to avoid waiting on the timeoutwhen there is a promotion. > That makes me think that we may want to add 2 extra parameters (as 2 functions returning a bool?) to ConditionVariableEventSleep() > to check whether or not we still want to test the socket or the CV wake up in each loop iteration. > > Also 3 additional remarks: > > 1) About InitializeConditionVariableWaitSet() and ConditionVariableWaitSetCreate(): I'm not sure about the naming as thereis no CV yet (they "just" deal with WaitEventSet). > > So, what about renaming? > > +static WaitEventSet *ConditionVariableWaitSet = NULL; > > to say, "LocalWaitSet" and then rename ConditionVariableWaitSetLatchPos, InitializeConditionVariableWaitSet() and ConditionVariableWaitSetCreate()accordingly? > > But it might be not needed (see 3) below). > > 2) > > /* > * Prepare to wait on a given condition variable. > * > @@ -97,7 +162,8 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv) > void > ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info) > { > - (void) ConditionVariableTimedSleep(cv, -1 /* no timeout */ , > + (void) ConditionVariableEventSleep(cv, ConditionVariableWaitSet, > + -1 /* no timeout */ , > wait_event_info); > } > > @@ -111,11 +177,27 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info) > bool > ConditionVariableTimedSleep(ConditionVariable *cv, long timeout, > uint32 wait_event_info) > +{ > + return ConditionVariableEventSleep(cv, ConditionVariableWaitSet, timeout, > + wait_event_info); > +} > + > > I like the idea of making use of the new ConditionVariableEventSleep() here, but on the other hand... > > 3) > > I wonder if there is no race conditions: ConditionVariableWaitSet is being initialized with PGINVALID_SOCKET > as WL_LATCH_SET and might be also (if IsUnderPostmaster) be initialized with PGINVALID_SOCKET as WL_EXIT_ON_PM_DEATH. > > So IIUC, the patch is introducing 2 new possible source of wake up. > > Then, what about? > > - not create ConditionVariableWaitSet, ConditionVariableWaitSetLatchPos, InitializeConditionVariableWaitSet() and ConditionVariableWaitSetCreate()at all? > - call ConditionVariableEventSleep() with a NULL parameter in ConditionVariableSleep() and ConditionVariableTimedSleep()? > - handle the case where the WaitEventSet parameter is NULL in ConditionVariableEventSleep()? (That could also make senseif we handle the case of the CV being NULL as proposed above) > I gave it a try, so please find attached v2-0001-Introduce-ConditionVariableEventSleep.txt (implementing the comments above)and 0004_new_API.txt to put the new API in the logical decoding on standby context. There is no change in v2-0001-Introduce-ConditionVariableEventSleep.txt regarding the up-thread comment related to WL_POSTMASTER_DEATH. What do you think? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
pgsql-hackers by date: