From: | "Drouvot, Bertrand" <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | Jeff Davis <pgsql(at)j-davis(dot)com>, Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>, fabriziomello(at)gmail(dot)com, tushar <tushar(dot)ahuja(at)enterprisedb(dot)com>, Rahila Syed <rahila(dot)syed(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Minimal logical decoding on standbys |
Date: | 2023-03-10 11:33:50 |
Message-ID: | f7e68298-dfb2-888d-016e-8cb8a9965a8b@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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 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
Attachment | Content-Type | Size |
---|---|---|
0004_new_API.txt | text/plain | 7.7 KB |
v2-0001-Introduce-ConditionVariableEventSleep.txt | text/plain | 5.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Önder Kalacı | 2023-03-10 11:46:43 | Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher |
Previous Message | Amit Kapila | 2023-03-10 11:30:53 | Re: Add macros for ReorderBufferTXN toptxn |