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
