Re: Minimal logical decoding on standbys

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 timeout when 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 there is 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 sense if 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