Re: Minimal logical decoding on standbys - Mailing list pgsql-hackers
From | Drouvot, Bertrand |
---|---|
Subject | Re: Minimal logical decoding on standbys |
Date | |
Msg-id | c188da88-a3d5-15c5-b6b5-f925abccc873@gmail.com Whole thread Raw |
In response to | Re: Minimal logical decoding on standbys ("Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>) |
Responses |
Re: Minimal logical decoding on standbys
Re: Minimal logical decoding on standbys |
List | pgsql-hackers |
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 timeout whenthere 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) Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
pgsql-hackers by date: