Message-ID: <825c1d30-e4cb-4f2c-b27b-0230d4079dcd@postgrespro.ru>
Date: Thu, 10 Apr 2025 10:12:34 +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: Kirill Reshke <reshkekirill@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <163fcbec-900b-4b07-beaa-d2ead8634bec@postgrespro.ru>
 <Z-R79EJVYfRy-EpW@paquier.xyz>
 <cf922d84-95dd-4628-bf1e-087535f95932@postgrespro.ru>
 <Z_YltcZNm1cCRwYj@paquier.xyz>
 <69541672-0fa2-41ea-af52-6e6110bf7235@postgrespro.ru>
 <Z_csIqEv6dhZ77jz@paquier.xyz>
Content-Language: en-US
From: "Maksim.Melnikov" <m.melnikov@postgrespro.ru>
In-Reply-To: <Z_csIqEv6dhZ77jz@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/825c1d30-e4cb-4f2c-b27b-0230d4079dcd%40postgrespro.ru>
Precedence: bulk


On 10.04.2025 05:25, Michael Paquier wrote:
> Confirmed.  One thing where it would be possible to make things work
> is to introduce some persistency of the injection points, say these
> are flushed at shutdown.  We could do that without touching at the
> backend code and only in the module injection_points, but perhaps this
> thread is not enough to justify this extra complexity.

Agree, it should be discussed in another thread.


On 10.04.2025 05:25, Michael Paquier wrote:
> What do you think?

Yes, patch for me looks ok, only one suggestion, I am not sure

but I am afraid we are loosing this.


         if (WalSndCtl->sync_standbys_status & SYNC_STANDBY_INIT)
         {
                 if ((WalSndCtl->sync_standbys_status & 
SYNC_STANDBY_DEFINED) == 0 ||
                         lsn <= WalSndCtl->lsn[mode])
                 {
                         LWLockRelease(SyncRepLock);
                         return;
                 }
         }
         else if (!SyncStandbysDefined())
         {
                 /*
                  * If we are here, the sync standby data has not been 
initialized yet,
                  * so fall back to the best thing we can do: a check on
                  * SyncStandbysDefined() to see if the GUC is set or not.
                  *
                  * If the GUC has a value, we wait until the 
checkpointer updates the
                  * status data because we cannot be sure yet if we 
should wait or not.
                  * If the GUC has *no* value, we are sure that there is 
no point to
                  * wait; this matters for example when initializing a 
cluster, where
                  * we should never wait, and no sync standbys is the 
default behavior.
                  */
                 LWLockRelease(SyncRepLock);
                 return;
         }
+       else if (lsn <= WalSndCtl->lsn[mode])
+       {
+               LWLockRelease(SyncRepLock);
+               return;
+       }

What do you think?


Best regards,

Maksim Melnikov