Re: Minimal logical decoding on standbys

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