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-08 10:25:10 |
Message-ID: | c188da88-a3d5-15c5-b6b5-f925abccc873@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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 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)
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 | 19.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Bharath Rupireddy | 2023-03-08 10:31:56 | Re: Add pg_walinspect function with block info columns |
Previous Message | Daniel Gustafsson | 2023-03-08 09:37:12 | Re: Allow tests to pass in OpenSSL FIPS mode |