Content-Type: multipart/mixed; boundary="------------Mg77xwopk0H9m5NPJaJFvu2R"
Message-ID: <884d9875-d35d-44fb-993d-f9326e5282a8@postgrespro.ru>
Date: Fri, 28 Mar 2025 10:57:58 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: sync_standbys_defined read/write race on startup
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
References: <163fcbec-900b-4b07-beaa-d2ead8634bec@postgrespro.ru>
 <Z-R79EJVYfRy-EpW@paquier.xyz>
Content-Language: en-US
From: "Maksim.Melnikov" <m.melnikov@postgrespro.ru>
In-Reply-To: <Z-R79EJVYfRy-EpW@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/884d9875-d35d-44fb-993d-f9326e5282a8%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------Mg77xwopk0H9m5NPJaJFvu2R
Content-Type: multipart/alternative;
 boundary="------------dPbUBH03iCnrd4CWrOSNm7Xo"

--------------dPbUBH03iCnrd4CWrOSNm7Xo
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 27.03.2025 01:13, Michael Paquier wrote:
> Tracking if the configuration has been properly loaded in
> WalSndCtlData makes sense here, but I am confused by the patch: you
> have defined SYNCSTANDBYSDEFINED but sync_standbys_defined never sets
> it.  It may be simpler to use a separate boolean flag for this
> purpose.  WalSndCtlData is a private structure holding the state of
> the WAL senders internally, so ABI does not stress me much here
> (flexible array at the end of the structure, as well..).

Thanks for your responses.
Yes, I agree that it looks more complicated then addition new bool field,
but there is place in SyncRepWaitForLSN method, where we check
WalSndCtlData->sync_standbys_defined out of acquire/release block.

void
SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
{
     int            mode;
...............................................
*    if (!SyncRepRequested() ||
         !((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)
         return;*
..........................

If we just add new bool field, we can't atomically check 
sync_standbys_defined flag
and new bool field, only in lock, so we lose fast exit optimization. But 
I agree
that in patch I lost SYNCSTANDBYSDEFINED setting, thanks for attentiveness,
so I've attached new patch with fix,also I've done bits setting more 
evident.

Anyway, if you still thinks that my arguments not enough, please notify, 
I will do patch with separate flag.


On 27.03.2025 01:13, Michael Paquier wrote:
> Also mentioned by Kirill downthread: let's add a regression test with
> an injection point that does a wait.  This race condition is worth
> having a test for.  You could define the point of where you have added
> your hardcoded sleep(), for example.
Sorry, but for me it seems that we can't use injection points toolset 
here, because we are talking
about startup process, and we can't call injection_points_attach on 
client backend before checkpointer.

Anyway, if you know how to do it, please share details, I will do.


Best regards,

Maksim Melnikov
--------------dPbUBH03iCnrd4CWrOSNm7Xo
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 27.03.2025 01:13, Michael Paquier
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:Z-R79EJVYfRy-EpW@paquier.xyz">
      <pre class="moz-quote-pre" wrap="">Tracking if the configuration has been properly loaded in
WalSndCtlData makes sense here, but I am confused by the patch: you
have defined SYNCSTANDBYSDEFINED but sync_standbys_defined never sets
it.  It may be simpler to use a separate boolean flag for this
purpose.  WalSndCtlData is a private structure holding the state of
the WAL senders internally, so ABI does not stress me much here
(flexible array at the end of the structure, as well..).</pre>
    </blockquote>
    <p>Thanks for your responses.<br>
      Yes, I agree that it looks more complicated then addition new bool
      field,<br>
      but there is place in SyncRepWaitForLSN method, where we check<br>
      WalSndCtlData-&gt;sync_standbys_defined out of acquire/release
      block.<br>
      <br>
      void<br>
      SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)<br>
      {<br>
          int            mode;<br>
      ...............................................<br>
      <b>    if (!SyncRepRequested() ||<br>
                !((volatile WalSndCtlData *)
        WalSndCtl)-&gt;sync_standbys_defined)<br>
                return;</b><br>
      ..........................<br>
      <br>
      If we just add new bool field, we can't atomically check
      sync_standbys_defined flag<br>
      and new bool field, only in lock, so we lose fast exit
      optimization. But I agree<br>
      that in patch I lost SYNCSTANDBYSDEFINED setting, thanks for
      attentiveness,<br>
      so I've attached new patch with fix,also I've done bits setting
      more evident. </p>
    <p>Anyway, if you still thinks that my arguments not enough, please
      notify, I will do patch with separate flag.</p>
    <br>
    <div class="moz-cite-prefix">On 27.03.2025 01:13, Michael Paquier
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:Z-R79EJVYfRy-EpW@paquier.xyz">
      <pre class="moz-quote-pre" wrap="">Also mentioned by Kirill downthread: let's add a regression test with
an injection point that does a wait.  This race condition is worth
having a test for.  You could define the point of where you have added
your hardcoded sleep(), for example.</pre>
    </blockquote>
    Sorry, but for me it seems that we can't use injection points
    toolset here, because we are talking<br>
    about startup process, and we can't call injection_points_attach on
    client backend before checkpointer.<br>
    <p>Anyway, if you know how to do it, please share details, I will
      do.</p>
    <p><br>
    </p>
    <p>Best regards,</p>
    Maksim Melnikov
  </body>
</html>

--------------dPbUBH03iCnrd4CWrOSNm7Xo--

--------------Mg77xwopk0H9m5NPJaJFvu2R
Content-Type: text/x-patch; charset=UTF-8;
 name="0002-Fix-sync_standbys_defined-race-on-startup.patch"
Content-Disposition: attachment;
 filename="0002-Fix-sync_standbys_defined-race-on-startup.patch"
Content-Transfer-Encoding: base64

RnJvbSBjODg5ZmQ4MTE5NjQyYTRlOWQzZmIzZTI0ZmU1ZDZjZDBlNjU2Y2E1IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBNYWtzaW0gTWVsbmlrb3YgPG0ubWVsbmlrb3ZAcG9z
dGdyZXNwcm8ucnU+CkRhdGU6IFdlZCwgMjYgTWFyIDIwMjUgMTY6NDQ6MDkgKzAzMDAKU3Vi
amVjdDogW1BBVENIXSBGaXggc3luY19zdGFuZGJ5c19kZWZpbmVkIHJhY2Ugb24gc3RhcnR1
cC4KCldhbFNuZEN0bC0+c3luY19zdGFuZGJ5c19kZWZpbmVkLCBjYW4gYmUgdXBkYXRlZApi
eSBjaGVja3BvaW50ZXIgcHJvY2VzcyBvbmx5IGFuZCBpbiBiYWNrZW5kcyBpdApjYW4gYmUg
cmVhZC4gU28sIG9uIHBvc3RncmVzIHN0YXJ0dXAsIHdlIGhhdmUgcmFjZQpiZXR3ZWVuIGNo
ZWNrcG9pbnRlciBwcm9jZXNzIGFuZCBiYWNrZW5kcywgdGhhdCBjaGVjawpzeW5jX3N0YW5k
YnlzX2RlZmluZWQgZm9yIHN5bmMgcmVwbGljYXRpb25zLgoKVG8gYXZvaWQgdGhpcywgbm93
IHdlIGFyZSB3b3JraW5nIHdpdGggc3luY19zdGFuZGJ5c19kZWZpbmVkCm5vdCBhcyBib29s
IGZpZWxkLCBidXQgYXMgYml0bWFzaywgd2hlcmUgN3RoIGJpdCBjaGVjawpzdGFuZGJ5c19k
ZWZpbmVkIG9yIG5vdCwgOHRoIGJpdCBjaGVjawpXYWxTbmRDdGxEYXRhLT5zeW5jX3N0YW5k
YnlzX2RlZmluZWQgZmllbGQgaGFzIGJlZW4KaW5pdGlhbGl6ZWQgYnkgY2hlY2twb2ludGVy
LiBTbyBpZiBpbiBzeW5jaHJvbm91cyByZXBsaWNhdGlvbiBmdW5jdGlvbnMKd2FzIGRldGVj
dGVkIHRoYXQgc3luY19zdGFuZGJ5c19kZWZpbmVkIGlzIG5vdCBpbml0aWFsaXplZCwKY2hl
Y2sgU3luY1N0YW5kYnlzRGVmaW5lZCgpIHZhbHVlLgotLS0KIHNyYy9iYWNrZW5kL3JlcGxp
Y2F0aW9uL3N5bmNyZXAuYyAgICAgICAgICAgfCA0NSArKysrKysrKysrKysrKysrKy0tLS0K
IHNyYy9pbmNsdWRlL3JlcGxpY2F0aW9uL3dhbHNlbmRlcl9wcml2YXRlLmggfCAxMCArKysr
LQogMiBmaWxlcyBjaGFuZ2VkLCA0NyBpbnNlcnRpb25zKCspLCA4IGRlbGV0aW9ucygtKQoK
ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3N5bmNyZXAuYyBiL3NyYy9i
YWNrZW5kL3JlcGxpY2F0aW9uL3N5bmNyZXAuYwppbmRleCBkNzVlMzk2ODAzNS4uZDJiYTRk
NjA0ZWQgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3N5bmNyZXAuYwor
KysgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9zeW5jcmVwLmMKQEAgLTE0OCw2ICsxNDgs
NyBAQCB2b2lkCiBTeW5jUmVwV2FpdEZvckxTTihYTG9nUmVjUHRyIGxzbiwgYm9vbCBjb21t
aXQpCiB7CiAJaW50CQkJbW9kZTsKKwlib29sCQlzeW5jX3N0YW5kYnlzX2RlZmluZWQ7CiAK
IAkvKgogCSAqIFRoaXMgc2hvdWxkIGJlIGNhbGxlZCB3aGlsZSBob2xkaW5nIGludGVycnVw
dHMgZHVyaW5nIGEgdHJhbnNhY3Rpb24KQEAgLTE3MCw3ICsxNzEsOCBAQCBTeW5jUmVwV2Fp
dEZvckxTTihYTG9nUmVjUHRyIGxzbiwgYm9vbCBjb21taXQpCiAJICogaXQncyBmYWxzZSwg
dGhlIGxvY2sgaXMgbm90IG5lY2Vzc2FyeSBiZWNhdXNlIHdlIGRvbid0IHRvdWNoIHRoZSBx
dWV1ZS4KIAkgKi8KIAlpZiAoIVN5bmNSZXBSZXF1ZXN0ZWQoKSB8fAotCQkhKCh2b2xhdGls
ZSBXYWxTbmRDdGxEYXRhICopIFdhbFNuZEN0bCktPnN5bmNfc3RhbmRieXNfZGVmaW5lZCkK
KwkJKCgodm9sYXRpbGUgV2FsU25kQ3RsRGF0YSAqKSBXYWxTbmRDdGwpLT5zeW5jX3N0YW5k
YnlzX2RlZmluZWQgJiBTWU5DU1RBTkRCWVNERUZJTkVESU5JVElBTElaRUQpCisJCT09IFNZ
TkNTVEFOREJZU0lOSVRJQUxJWkVEKQogCQlyZXR1cm47CiAKIAkvKiBDYXAgdGhlIGxldmVs
IGZvciBhbnl0aGluZyBvdGhlciB0aGFuIGNvbW1pdCB0byByZW1vdGUgZmx1c2ggb25seS4g
Ki8KQEAgLTE4NCw2ICsxODYsOCBAQCBTeW5jUmVwV2FpdEZvckxTTihYTG9nUmVjUHRyIGxz
biwgYm9vbCBjb21taXQpCiAKIAlMV0xvY2tBY3F1aXJlKFN5bmNSZXBMb2NrLCBMV19FWENM
VVNJVkUpOwogCUFzc2VydChNeVByb2MtPnN5bmNSZXBTdGF0ZSA9PSBTWU5DX1JFUF9OT1Rf
V0FJVElORyk7CisJc3luY19zdGFuZGJ5c19kZWZpbmVkID0gV2FsU25kQ3RsLT5zeW5jX3N0
YW5kYnlzX2RlZmluZWQgJiBTWU5DU1RBTkRCWVNJTklUSUFMSVpFRAorCQk/IFdhbFNuZEN0
bC0+c3luY19zdGFuZGJ5c19kZWZpbmVkICYgU1lOQ1NUQU5EQllTREVGSU5FRCA6IFN5bmNT
dGFuZGJ5c0RlZmluZWQoKTsKIAogCS8qCiAJICogV2UgZG9uJ3Qgd2FpdCBmb3Igc3luYyBy
ZXAgaWYgV2FsU25kQ3RsLT5zeW5jX3N0YW5kYnlzX2RlZmluZWQgaXMgbm90CkBAIC0xOTMs
NyArMTk3LDcgQEAgU3luY1JlcFdhaXRGb3JMU04oWExvZ1JlY1B0ciBsc24sIGJvb2wgY29t
bWl0KQogCSAqIGNvbmRpdGlvbiBidXQgd2UnbGwgYmUgZmV0Y2hpbmcgdGhhdCBjYWNoZSBs
aW5lIGFueXdheSBzbyBpdCdzIGxpa2VseQogCSAqIHRvIGJlIGEgbG93IGNvc3QgY2hlY2su
CiAJICovCi0JaWYgKCFXYWxTbmRDdGwtPnN5bmNfc3RhbmRieXNfZGVmaW5lZCB8fAorCWlm
ICghc3luY19zdGFuZGJ5c19kZWZpbmVkIHx8CiAJCWxzbiA8PSBXYWxTbmRDdGwtPmxzbltt
b2RlXSkKIAl7CiAJCUxXTG9ja1JlbGVhc2UoU3luY1JlcExvY2spOwpAQCAtOTIwLDE4ICs5
MjQsMTkgQEAgU3luY1JlcFdha2VRdWV1ZShib29sIGFsbCwgaW50IG1vZGUpCiB2b2lkCiBT
eW5jUmVwVXBkYXRlU3luY1N0YW5kYnlzRGVmaW5lZCh2b2lkKQogewotCWJvb2wJCXN5bmNf
c3RhbmRieXNfZGVmaW5lZCA9IFN5bmNTdGFuZGJ5c0RlZmluZWQoKTsKKwlib29sCQluZXdf
c3luY19zdGFuZGJ5c19kZWZpbmVkID0gU3luY1N0YW5kYnlzRGVmaW5lZCgpOworCWJvb2wJ
CW9sZF9zeW5jX3N0YW5kYnlzX2RlZmluZWQgPSAoV2FsU25kQ3RsLT5zeW5jX3N0YW5kYnlz
X2RlZmluZWQgJiBTWU5DU1RBTkRCWVNERUZJTkVEKTsKIAotCWlmIChzeW5jX3N0YW5kYnlz
X2RlZmluZWQgIT0gV2FsU25kQ3RsLT5zeW5jX3N0YW5kYnlzX2RlZmluZWQpCisJaWYgKG5l
d19zeW5jX3N0YW5kYnlzX2RlZmluZWQgIT0gb2xkX3N5bmNfc3RhbmRieXNfZGVmaW5lZCkK
IAl7CisJCXVpbnQ4IG52OwogCQlMV0xvY2tBY3F1aXJlKFN5bmNSZXBMb2NrLCBMV19FWENM
VVNJVkUpOwotCiAJCS8qCiAJCSAqIElmIHN5bmNocm9ub3VzX3N0YW5kYnlfbmFtZXMgaGFz
IGJlZW4gcmVzZXQgdG8gZW1wdHksIGl0J3MgZnV0aWxlCiAJCSAqIGZvciBiYWNrZW5kcyB0
byBjb250aW51ZSB3YWl0aW5nLiAgU2luY2UgdGhlIHVzZXIgbm8gbG9uZ2VyIHdhbnRzCiAJ
CSAqIHN5bmNocm9ub3VzIHJlcGxpY2F0aW9uLCB3ZSdkIGJldHRlciB3YWtlIHRoZW0gdXAu
CiAJCSAqLwotCQlpZiAoIXN5bmNfc3RhbmRieXNfZGVmaW5lZCkKKwkJaWYgKCFuZXdfc3lu
Y19zdGFuZGJ5c19kZWZpbmVkKQogCQl7CiAJCQlpbnQJCQlpOwogCkBAIC05NDYsOCArOTUx
LDM0IEBAIFN5bmNSZXBVcGRhdGVTeW5jU3RhbmRieXNEZWZpbmVkKHZvaWQpCiAJCSAqIGJh
Y2tlbmQgdGhhdCBoYXNuJ3QgeWV0IHJlbG9hZGVkIGl0cyBjb25maWcgbWlnaHQgZ28gdG8g
c2xlZXAgb24KIAkJICogdGhlIHF1ZXVlIChhbmQgbmV2ZXIgd2FrZSB1cCkuICBUaGlzIHBy
ZXZlbnRzIHRoYXQuCiAJCSAqLwotCQlXYWxTbmRDdGwtPnN5bmNfc3RhbmRieXNfZGVmaW5l
ZCA9IHN5bmNfc3RhbmRieXNfZGVmaW5lZDsKIAorCQkvKiBzZXQgaW5pdGlhbGl6ZWQgYml0
LiovCisJCW52ID0gV2FsU25kQ3RsLT5zeW5jX3N0YW5kYnlzX2RlZmluZWQgfCBTWU5DU1RB
TkRCWVNJTklUSUFMSVpFRDsKKwkJaWYgKG5ld19zeW5jX3N0YW5kYnlzX2RlZmluZWQpCisJ
CXsKKyAgCQkJLyogc2V0IGRlZmluZWQgYml0LiovCisJCQludiB8PSBTWU5DU1RBTkRCWVNE
RUZJTkVEOworCQl9CisJCWVsc2UKKwkJeworCQkJLyogcmVzZXQgZGVmaW5lZCBiaXQuKi8K
KwkJCW52ICY9ICh+U1lOQ1NUQU5EQllTREVGSU5FRCk7CisJCX0KKworCQlXYWxTbmRDdGwt
PnN5bmNfc3RhbmRieXNfZGVmaW5lZCA9IG52OworCisJCUxXTG9ja1JlbGVhc2UoU3luY1Jl
cExvY2spOworCX0KKwllbHNlIGlmICghKFdhbFNuZEN0bC0+c3luY19zdGFuZGJ5c19kZWZp
bmVkICYgU1lOQ1NUQU5EQllTSU5JVElBTElaRUQpKQorCXsKKwkJLyoKKwkJICogRXZlbiBp
ZiBuZXdfc3luY19zdGFuZGJ5c19kZWZpbmVkID09IG9sZF9zeW5jX3N0YW5kYnlzX2RlZmlu
ZWQsIHdlCisJCSAqIHNob3VsZCBjaGVjayBXYWxTbmRDdGwtPnN5bmNfc3RhbmRieXNfZGVm
aW5lZCBpcyBpbml0aWFsaXplZCBhbmQKKwkJICogc2V0IGl0cyBpbml0aWFsaXplIGJpdCwg
aWYgaXQgcmVzZXQuIEFueXdheSB0aGlzIGJpdCBzaG91bGQgYmUgc2V0CisJCSAqIG9ubHkg
b24gc3RhcnQgYW5kIHRoZW4gaXQgd2lsbCBiZSBhbHdheXMgc2V0dGVkLgorCQkgKi8KKwkJ
TFdMb2NrQWNxdWlyZShTeW5jUmVwTG9jaywgTFdfRVhDTFVTSVZFKTsKKwkJV2FsU25kQ3Rs
LT5zeW5jX3N0YW5kYnlzX2RlZmluZWQgfD0gU1lOQ1NUQU5EQllTSU5JVElBTElaRUQ7CiAJ
CUxXTG9ja1JlbGVhc2UoU3luY1JlcExvY2spOwogCX0KIH0KZGlmZiAtLWdpdCBhL3NyYy9p
bmNsdWRlL3JlcGxpY2F0aW9uL3dhbHNlbmRlcl9wcml2YXRlLmggYi9zcmMvaW5jbHVkZS9y
ZXBsaWNhdGlvbi93YWxzZW5kZXJfcHJpdmF0ZS5oCmluZGV4IDBmYzc3ZjFiNGFmLi5jYzZh
OThiN2UzMSAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvcmVwbGljYXRpb24vd2Fsc2VuZGVy
X3ByaXZhdGUuaAorKysgYi9zcmMvaW5jbHVkZS9yZXBsaWNhdGlvbi93YWxzZW5kZXJfcHJp
dmF0ZS5oCkBAIC0xMDAsNyArMTAwLDcgQEAgdHlwZWRlZiBzdHJ1Y3QKIAkgKiBjb25maWcg
ZmlsZSBzYWZlbHksIHNvIGNoZWNrcG9pbnRlciB1cGRhdGVzIHRoaXMgdmFsdWUgYXMgbmVl
ZGVkLgogCSAqIFByb3RlY3RlZCBieSBTeW5jUmVwTG9jay4KIAkgKi8KLQlib29sCQlzeW5j
X3N0YW5kYnlzX2RlZmluZWQ7CisJdWludDgJCXN5bmNfc3RhbmRieXNfZGVmaW5lZDsKIAog
CS8qIHVzZWQgYXMgYSByZWdpc3RyeSBvZiBwaHlzaWNhbCAvIGxvZ2ljYWwgd2Fsc2VuZGVy
cyB0byB3YWtlICovCiAJQ29uZGl0aW9uVmFyaWFibGUgd2FsX2ZsdXNoX2N2OwpAQCAtMTE2
LDYgKzExNiwxNCBAQCB0eXBlZGVmIHN0cnVjdAogCVdhbFNuZAkJd2Fsc25kc1tGTEVYSUJM
RV9BUlJBWV9NRU1CRVJdOwogfSBXYWxTbmRDdGxEYXRhOwogCisvKgorICogV2FsU25kQ3Rs
RGF0YS0+c3luY19zdGFuZGJ5c19kZWZpbmVkIGJpdCBmbGFncy4gN3RoIGJpdCBjaGVjayBz
dGFuZGJ5c19kZWZpbmVkIG9yIG5vdCwKKyAqIDh0aCBiaXQgY2hlY2sgV2FsU25kQ3RsRGF0
YS0+c3luY19zdGFuZGJ5c19kZWZpbmVkIGZpZWxkIGhhcyBiZWVuIGluaXRpYWxpemVkLgor
ICovCisjZGVmaW5lIFNZTkNTVEFOREJZU0lOSVRJQUxJWkVECSgxIDw8IDApCisjZGVmaW5l
IFNZTkNTVEFOREJZU0RFRklORUQJCSgxIDw8IDEpCisjZGVmaW5lIFNZTkNTVEFOREJZU0RF
RklORURJTklUSUFMSVpFRAkoU1lOQ1NUQU5EQllTSU5JVElBTElaRUQgJiBTWU5DU1RBTkRC
WVNERUZJTkVEKQorCiBleHRlcm4gUEdETExJTVBPUlQgV2FsU25kQ3RsRGF0YSAqV2FsU25k
Q3RsOwogCiAKLS0gCjIuNDMuMAoK

--------------Mg77xwopk0H9m5NPJaJFvu2R--