Content-Type: multipart/mixed; boundary="------------OKBzpDJu6imeCU0xRkhOI7yv"
Message-ID: <8a7da0c2-b7fe-1c78-4e47-a16239bf4839@gmail.com>
Date: Thu, 2 Mar 2023 10:20:09 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: Minimal logical decoding on standbys
Content-Language: en-US
To: Jeff Davis <pgsql@j-davis.com>, Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>, Thomas Munro
 <thomas.munro@gmail.com>, Alvaro Herrera <alvherre@2ndquadrant.com>,
 Ibrar Ahmed <ibrar.ahmad@gmail.com>, Amit Khandekar
 <amitdkhan.pg@gmail.com>, fabriziomello@gmail.com,
 tushar <tushar.ahuja@enterprisedb.com>,
 Rahila Syed <rahila.syed@2ndquadrant.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <CA+TgmoZ8-DXtDMMDGggFkSYyO8nmNdBKWjV6Ts-zYGR9p5sK5g@mail.gmail.com>
 <CA+TgmoaOsoQuXEo6gMq0GP8SyvCaS=ptYqdYc0uzDMoeShf_nw@mail.gmail.com>
 <CA+TgmoYTTsxP8y6uknZvCBNCRq+1FJ4zGbX8Px1TGW459fGsaQ@mail.gmail.com>
 <1f2ad9fd-53ff-74cc-a602-72d45423bcab@gmail.com>
 <a78de011-81cf-729c-e110-e6d636fdca4d@gmail.com>
 <05c0c4f3-82eb-bc6d-c876-5a265c05927b@gmail.com>
 <CA+TgmoY8LAaTwAhS+YNQNrM+3K8eqxGvPT3gbJaHER86XShvFw@mail.gmail.com>
 <20230106034036.2m4qnn7ep7b5ipet@awork3.anarazel.de>
 <490206c5-bcde-e558-628a-dc67dae4d439@gmail.com>
 <20230119024617.jzsgpcrgjdc6ysge@awork3.anarazel.de>
 <a343b646-c632-4b9a-0e01-b661075d1d8b@gmail.com>
 <b74520d5-5b9e-1477-2abc-95ba897f23e4@gmail.com>
 <94d9d47e-72f4-393a-246e-e3fe8db7b741@gmail.com>
 <a4c03fad-2986-ddab-c12e-ea2ee0119181@gmail.com>
 <2d314c22b9e03415aa1c7d8fd1f698dae60effa7.camel@j-davis.com>
 <061eeb26-156e-6011-02b6-b4b1422e2aa6@gmail.com>
 <ec744999afcdbd6bec5d9e6572aba875060019cb.camel@j-davis.com>
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <ec744999afcdbd6bec5d9e6572aba875060019cb.camel@j-davis.com>
Archived-At: <https://www.postgresql.org/message-id/8a7da0c2-b7fe-1c78-4e47-a16239bf4839%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------OKBzpDJu6imeCU0xRkhOI7yv
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi,

On 3/2/23 1:40 AM, Jeff Davis wrote:
> On Wed, 2023-03-01 at 11:51 +0100, Drouvot, Bertrand wrote:
> 
>>
>> Why not "simply" call ConditionVariablePrepareToSleep() without any
>> call to ConditionVariableTimedSleep() later?
> 
> ConditionVariableSleep() re-inserts itself into the queue if it was
> previously removed. Without that, a single wakeup could remove it from
> the wait queue, and the effects of ConditionVariablePrepareToSleep()
> would be lost.

Right, but in our case, right after the wakeup (the one due to the CV broadcast,
aka the one that will remove it from the wait queue) we'll exit the loop due to:

"
         /* check whether we're done */
         if (loc <= RecentFlushPtr)
             break;
"

as the CV broadcast means that a flush/replay occurred.

So I don't see any issue in this particular case (as we are removed from the queue
but we'll not have to wait anymore).

> 
>> In that case the walsender will be put in the wait queue (thanks to
>> ConditionVariablePrepareToSleep())
>> and will be waked up by the event on the socket, the timeout or the
>> CV broadcast
> 
> I believe it will only be awakened once, and if it enters WalSndWait()
> again, future ConditionVariableBroadcast/Signal() calls won't wake it
> up any more.

I don't think that's right and that's not what my testing shows (please find attached 0004-CV-POC.txt,
a .txt file to not break the CF bot), as:

- If it is awakened due to the CV broadcast, then we'll right after exit the loop (see above)
- If it is awakened due to the timeout or the socket event then we're still in the CV wait queue
(as nothing removed it from the CV wait queue).

> 
>>   (since IIUC they all rely on the same latch).
> 
> Relying on that fact seems like too much action-at-a-distance to me
> If
> we change the implementation of condition variables, then it would stop
> working.
> 

I'm not sure about this one. I mean it would depend what the implementation changes are.
Also the related TAP test (0005) would probably fail or start taking a long time due to
the corner case we are trying to solve here coming back (like it was detected in [1])

> Also, since they are using the same latch, that means we are still
> waking up too frequently, right? We haven't really solved the problem.
> 

I don't think so as the first CV broadcast will make us exit the loop.
So, ISTM that we'll wake up as we currently do, expect when there is a flush/replay
which is what we want, right?

>> That looks weird to use ConditionVariablePrepareToSleep() without
>> actually using ConditionVariableTimedSleep()
>> but it looks to me that it would achieve the same goal: having the
>> walsender being waked up
>> by the event on the socket, the timeout or the CV broadcast.
> 
> I don't think it actually works, because something needs to keep re-
> inserting it into the queue after it gets removed.

I think that's not needed as we'd exit the loop right after we are awakened by a CV broadcast.
> 
> To use condition variables properly, I think we'd need an API like
> ConditionVariableEventsSleep(), which takes a WaitEventSet and a
> timeout. I think this is what Andres was suggesting and seems like a
> good idea. I looked into it and I don't think it's too hard to
> implement -- we just need to WaitEventSetWait instead of WaitLatch.
> There are a few details to sort out, like how to enable callers to
> easily create the right WaitEventSet (it obviously needs to include
> MyLatch, for instance) and update it with the right socket events.
> 

I agree that's a good idea and that it should/would work too. I just wanted to highlight that in this particular
case that might not be necessary to build this new API.

[1]: https://www.postgresql.org/message-id/47606911-cf44-5a62-21d5-366d3bc6e445%40enterprisedb.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
--------------OKBzpDJu6imeCU0xRkhOI7yv
Content-Type: text/plain; charset=UTF-8; name="0004-CV-POC.txt"
Content-Disposition: attachment; filename="0004-CV-POC.txt"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2FtL3hsb2dyZWNvdmVyeS5j
IGIvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0veGxvZ3JlY292ZXJ5LmMKaW5kZXggZGJl
OTM5NDc2Mi4uOGE5NTA1YTUyZCAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvYWNjZXNzL3Ry
YW5zYW0veGxvZ3JlY292ZXJ5LmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0v
eGxvZ3JlY292ZXJ5LmMKQEAgLTM1OCw2ICszNTgsOSBAQCB0eXBlZGVmIHN0cnVjdCBYTG9n
UmVjb3ZlcnlDdGxEYXRhCiAJUmVjb3ZlcnlQYXVzZVN0YXRlIHJlY292ZXJ5UGF1c2VTdGF0
ZTsKIAlDb25kaXRpb25WYXJpYWJsZSByZWNvdmVyeU5vdFBhdXNlZENWOwogCisJLyogUmVw
bGF5IHN0YXRlIChzZWUgY2hlY2tfZm9yX3JlcGxheSgpIGZvciBtb3JlIGV4cGxhbmF0aW9u
KSAqLworCUNvbmRpdGlvblZhcmlhYmxlIHJlcGxheWVkQ1Y7CisKIAlzbG9ja190CQlpbmZv
X2xjazsJCS8qIGxvY2tzIHNoYXJlZCB2YXJpYWJsZXMgc2hvd24gYWJvdmUgKi8KIH0gWExv
Z1JlY292ZXJ5Q3RsRGF0YTsKIApAQCAtNDY4LDYgKzQ3MSw3IEBAIFhMb2dSZWNvdmVyeVNo
bWVtSW5pdCh2b2lkKQogCVNwaW5Mb2NrSW5pdCgmWExvZ1JlY292ZXJ5Q3RsLT5pbmZvX2xj
ayk7CiAJSW5pdFNoYXJlZExhdGNoKCZYTG9nUmVjb3ZlcnlDdGwtPnJlY292ZXJ5V2FrZXVw
TGF0Y2gpOwogCUNvbmRpdGlvblZhcmlhYmxlSW5pdCgmWExvZ1JlY292ZXJ5Q3RsLT5yZWNv
dmVyeU5vdFBhdXNlZENWKTsKKwlDb25kaXRpb25WYXJpYWJsZUluaXQoJlhMb2dSZWNvdmVy
eUN0bC0+cmVwbGF5ZWRDVik7CiB9CiAKIC8qCkBAIC0xOTM1LDYgKzE5MzksMTEgQEAgQXBw
bHlXYWxSZWNvcmQoWExvZ1JlYWRlclN0YXRlICp4bG9ncmVhZGVyLCBYTG9nUmVjb3JkICpy
ZWNvcmQsIFRpbWVMaW5lSUQgKnJlcGwKIAlYTG9nUmVjb3ZlcnlDdGwtPmxhc3RSZXBsYXll
ZFRMSSA9ICpyZXBsYXlUTEk7CiAJU3BpbkxvY2tSZWxlYXNlKCZYTG9nUmVjb3ZlcnlDdGwt
PmluZm9fbGNrKTsKIAorCS8qCisJICogd2FrZSB1cCB3YWxzZW5kZXIocykgdXNlZCBieSBs
b2dpY2FsIGRlY29kaW5nIG9uIHN0YW5kYnkuCisJICovCisJQ29uZGl0aW9uVmFyaWFibGVC
cm9hZGNhc3QoJlhMb2dSZWNvdmVyeUN0bC0+cmVwbGF5ZWRDVik7CisKIAkvKgogCSAqIElm
IHJtX3JlZG8gY2FsbGVkIFhMb2dSZXF1ZXN0V2FsUmVjZWl2ZXJSZXBseSwgdGhlbiB3ZSB3
YWtlIHVwIHRoZQogCSAqIHJlY2VpdmVyIHNvIHRoYXQgaXQgbm90aWNlcyB0aGUgdXBkYXRl
ZCBsYXN0UmVwbGF5ZWRFbmRSZWNQdHIgYW5kIHNlbmRzCkBAIC00OTQyLDMgKzQ5NTEsMjIg
QEAgYXNzaWduX3JlY292ZXJ5X3RhcmdldF94aWQoY29uc3QgY2hhciAqbmV3dmFsLCB2b2lk
ICpleHRyYSkKIAllbHNlCiAJCXJlY292ZXJ5VGFyZ2V0ID0gUkVDT1ZFUllfVEFSR0VUX1VO
U0VUOwogfQorCisvKgorICogUmV0dXJuIHRoZSBDb25kaXRpb25WYXJpYWJsZSBpbmRpY2F0
aW5nIHRoYXQgYSByZXBsYXkgaGFzIGJlZW4gZG9uZS4KKyAqCisgKiBUaGlzIGlzIG5lZWRl
ZCBmb3IgbG9naWNhbCBkZWNvZGluZyBvbiBzdGFuZGJ5LiBJbmRlZWQgdGhlICJwcm9ibGVt
IiBpcyB0aGF0CisgKiBXYWxTbmRXYWl0Rm9yV2FsKCkgd2FpdHMgZm9yIHRoZSAqcmVwbGF5
KiBMU04gdG8gaW5jcmVhc2UsIGJ1dCBnZXRzIHdva2VuIHVwCisgKiBieSB3YWxyZWNlaXZl
ciB3aGVuIG5ldyBXQUwgaGFzIGJlZW4gZmx1c2hlZC4gV2hpY2ggbWVhbnMgdGhhdCB0eXBp
Y2FsbHkKKyAqIHdhbHNlbmRlcnMgd2lsbCBnZXQgd29rZW4gdXAgYXQgdGhlIHNhbWUgdGlt
ZSB0aGF0IHRoZSBzdGFydHVwIHByb2Nlc3MKKyAqIHdpbGwgYmUgLSB3aGljaCBtZWFucyB0
aGF0IGJ5IHRoZSB0aW1lIHRoZSBsb2dpY2FsIHdhbHNlbmRlciBjaGVja3MKKyAqIEdldFhM
b2dSZXBsYXlSZWNQdHIoKSBpdCdzIHVubGlrZWx5IHRoYXQgdGhlIHN0YXJ0dXAgcHJvY2Vz
cyBhbHJlYWR5IHJlcGxheWVkCisgKiB0aGUgcmVjb3JkIGFuZCB1cGRhdGVkIFhMb2dDdGwt
Pmxhc3RSZXBsYXllZEVuZFJlY1B0ci4KKyAqCisgKiBUaGUgQ29uZGl0aW9uVmFyaWFibGUg
WExvZ1JlY292ZXJ5Q3RsLT5yZXBsYXllZENWIHNvbHZlcyB0aGlzIGNvcm5lciBjYXNlLgor
ICovCitDb25kaXRpb25WYXJpYWJsZSAqCitjaGVja19mb3JfcmVwbGF5KHZvaWQpCit7CisJ
cmV0dXJuICZYTG9nUmVjb3ZlcnlDdGwtPnJlcGxheWVkQ1Y7Cit9CmRpZmYgLS1naXQgYS9z
cmMvYmFja2VuZC9yZXBsaWNhdGlvbi93YWxzZW5kZXIuYyBiL3NyYy9iYWNrZW5kL3JlcGxp
Y2F0aW9uL3dhbHNlbmRlci5jCmluZGV4IDMwNDJlNWJkNjQuLjA1MzUwYmI1MzUgMTAwNjQ0
Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3dhbHNlbmRlci5jCisrKyBiL3NyYy9i
YWNrZW5kL3JlcGxpY2F0aW9uL3dhbHNlbmRlci5jCkBAIC0xNTUyLDYgKzE1NTIsMTcgQEAg
V2FsU25kV2FpdEZvcldhbChYTG9nUmVjUHRyIGxvYykKIHsKIAlpbnQJCQl3YWtlRXZlbnRz
OwogCXN0YXRpYyBYTG9nUmVjUHRyIFJlY2VudEZsdXNoUHRyID0gSW52YWxpZFhMb2dSZWNQ
dHI7CisJQ29uZGl0aW9uVmFyaWFibGUgKnJlcGxheWVkQ1YgPSBjaGVja19mb3JfcmVwbGF5
KCk7CisKKwkvKgorCSAqIFByZXBhcmUgdGhlIHJlcGxheWVkQ1YgdG8gc2xlZXAuIE5vdGUg
dGhhdCB0aGlzIGlzIGVub3VnaCB0byBiZSBhZGRlZAorCSAqIGluIHRoZSB3YWl0IHF1ZXVl
IGFuZCB0aGVuIHdha2VkIHVwICh3aGlsZSBpbiBXYWxTbmRXYWl0KCkgYmVsb3cpCisJICog
YnkgQ29uZGl0aW9uVmFyaWFibGVCcm9hZGNhc3QoKSBkdXJpbmcgdGhlIFdBTCByZXBsYXku
IEFsc28gTm90ZSB0aGF0CisJICogaWYgYXdha2FuZWQgYnkgdGhlIENWIGJyb2FkY2FzdCB3
ZSdsbCBleGl0IHRoZSBsb29wIHJpZ2h0IGFmdGVyIGR1ZSB0bworCSAqIHRoZSBsb2MgPD0g
UmVjZW50Rmx1c2hQdHIgdGVzdCBkb25lIGluIHRoZSBsb29wLiBJbmRlZWQsIENWIGJyb2Rj
YXN0CisJICogd291bGQgbWVhbiB0aGF0IGEgcmVwbGF5IG9jY3VyZWQuCisJICovCisJQ29u
ZGl0aW9uVmFyaWFibGVQcmVwYXJlVG9TbGVlcChyZXBsYXllZENWKTsKIAogCS8qCiAJICog
RmFzdCBwYXRoIHRvIGF2b2lkIGFjcXVpcmluZyB0aGUgc3BpbmxvY2sgaW4gY2FzZSB3ZSBh
bHJlYWR5IGtub3cgd2UKQEAgLTE2NzAsNiArMTY4MSw3IEBAIFdhbFNuZFdhaXRGb3JXYWwo
WExvZ1JlY1B0ciBsb2MpCiAJCVdhbFNuZFdhaXQod2FrZUV2ZW50cywgc2xlZXB0aW1lLCBX
QUlUX0VWRU5UX1dBTF9TRU5ERVJfV0FJVF9XQUwpOwogCX0KIAorCUNvbmRpdGlvblZhcmlh
YmxlQ2FuY2VsU2xlZXAoKTsKIAkvKiByZWFjdGl2YXRlIGxhdGNoIHNvIFdhbFNuZExvb3Ag
a25vd3MgdG8gY29udGludWUgKi8KIAlTZXRMYXRjaChNeUxhdGNoKTsKIAlyZXR1cm4gUmVj
ZW50Rmx1c2hQdHI7CmRpZmYgLS1naXQgYS9zcmMvaW5jbHVkZS9hY2Nlc3MveGxvZ3JlY292
ZXJ5LmggYi9zcmMvaW5jbHVkZS9hY2Nlc3MveGxvZ3JlY292ZXJ5LmgKaW5kZXggNDdjMjkz
NTBmNS4uMmJmZWFhYTAwZiAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvYWNjZXNzL3hsb2dy
ZWNvdmVyeS5oCisrKyBiL3NyYy9pbmNsdWRlL2FjY2Vzcy94bG9ncmVjb3ZlcnkuaApAQCAt
MTUsNiArMTUsNyBAQAogI2luY2x1ZGUgImNhdGFsb2cvcGdfY29udHJvbC5oIgogI2luY2x1
ZGUgImxpYi9zdHJpbmdpbmZvLmgiCiAjaW5jbHVkZSAidXRpbHMvdGltZXN0YW1wLmgiCisj
aW5jbHVkZSAic3RvcmFnZS9jb25kaXRpb25fdmFyaWFibGUuaCIKIAogLyoKICAqIFJlY292
ZXJ5IHRhcmdldCB0eXBlLgpAQCAtMTU1LDQgKzE1Niw2IEBAIGV4dGVybiB2b2lkIFJlY292
ZXJ5UmVxdWlyZXNJbnRQYXJhbWV0ZXIoY29uc3QgY2hhciAqcGFyYW1fbmFtZSwgaW50IGN1
cnJWYWx1ZSwKIAogZXh0ZXJuIHZvaWQgeGxvZ19vdXRkZXNjKFN0cmluZ0luZm8gYnVmLCBY
TG9nUmVhZGVyU3RhdGUgKnJlY29yZCk7CiAKK2V4dGVybiBDb25kaXRpb25WYXJpYWJsZSAq
Y2hlY2tfZm9yX3JlcGxheSh2b2lkKTsKKwogI2VuZGlmCQkJCQkJCS8qIFhMT0dSRUNPVkVS
WV9IICovCmRpZmYgLS1naXQgYS9zcmMvaW5jbHVkZS9yZXBsaWNhdGlvbi93YWxzZW5kZXIu
aCBiL3NyYy9pbmNsdWRlL3JlcGxpY2F0aW9uL3dhbHNlbmRlci5oCmluZGV4IDUyYmIzZTJh
YWUuLjJmZDc0NWZlNzIgMTAwNjQ0Ci0tLSBhL3NyYy9pbmNsdWRlL3JlcGxpY2F0aW9uL3dh
bHNlbmRlci5oCisrKyBiL3NyYy9pbmNsdWRlL3JlcGxpY2F0aW9uL3dhbHNlbmRlci5oCkBA
IC0xMyw2ICsxMyw3IEBACiAjZGVmaW5lIF9XQUxTRU5ERVJfSAogCiAjaW5jbHVkZSA8c2ln
bmFsLmg+CisjaW5jbHVkZSAic3RvcmFnZS9jb25kaXRpb25fdmFyaWFibGUuaCIKIAogLyoK
ICAqIFdoYXQgdG8gZG8gd2l0aCBhIHNuYXBzaG90IGluIGNyZWF0ZSByZXBsaWNhdGlvbiBz
bG90IGNvbW1hbmQuCg==

--------------OKBzpDJu6imeCU0xRkhOI7yv--