Thread
-
synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-02-24T22:08:37Z
Hi hackers, synchronized_standby_slots requires that every physical slot listed in the GUC has caught up before a logical failover slot is allowed to proceed with decoding. This is an ALL-of-N slots semantic. The logical slot availability model does not align with quorum replication semantics set using synchronous_standby_names which can be configured for quorum commit (ANY M of N). In a typical 3 Node HA deployment with quorum sync rep: Primary, standby1 (corresponds to sb1_slot), standby2 (corresponds to sb2_slot) synchronized_standby_slots = ' sb1_slot, sb2_slot' synchronous_standby_names = 'Any 1 ('standby1','standby2')' If standby1 goes down, synchronous commits still succeed because standby2 satisfies the quorum. However, logical decoding blocks indefinitely in WaitForStandbyConfirmation(), waiting for sb1_slot (corresponds to standby1) to catch up — even though the transaction is already safely committed on a quorum of synchronous standbys. This blocks logical decoding consumers from progressing and is inconsistent with the availability guarantee the DBA intended by choosing quorum commit. This scenario is constructed in the TAP test (052_synchronized_standby_slots_quorum.pl) in the attached patch. *Proposal:* Make synchronized_standby_slots quorum aware i.e. extend the GUC to accept an ANY M (slot1, slot2, ...) syntax similar to synchronous_standby_names, so StandbySlotsHaveCaughtup() can return true when M of N slots (where M <= N and M >= 1) have caught up. I still prefer two different GUCs for this as the list of slots to be synchronized can still be different (for example, DBA may want to ensure Geo standby to be sync before allowing the logical decoding client to read the changes). I kept synchronized_standby_slots parse logic similar to synchronous_standby_names to keep things simple. The default behavior is also not changed for synchronized_standby_slots. Added a draft patch (AI assisted). Please let me know your Thoughts. Thanks, Satya -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-02-25T13:51:17Z
Hi Satya, On Wed, Feb 25, 2026 at 3:38 AM SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> wrote: > > > Hi hackers, > > synchronized_standby_slots requires that every physical slot listed in the GUC has caught up before a logical failover slot is allowed to proceed with decoding. This is an ALL-of-N slots semantic. The logical slot availability model does not align with quorum replication semantics set using synchronous_standby_names which can be configured for quorum commit (ANY M of N). > > In a typical 3 Node HA deployment with quorum sync rep: > > Primary, standby1 (corresponds to sb1_slot), standby2 (corresponds to sb2_slot) > synchronized_standby_slots = ' sb1_slot, sb2_slot' > synchronous_standby_names = 'Any 1 ('standby1','standby2')' > > If standby1 goes down, synchronous commits still succeed because standby2 satisfies the quorum. However, logical decoding blocks indefinitely in WaitForStandbyConfirmation(), waiting for sb1_slot (corresponds to standby1) to catch up — even though the transaction is already safely committed on a quorum of synchronous standbys. This blocks logical decoding consumers from progressing and is inconsistent with the availability guarantee the DBA intended by choosing quorum commit. +1. This can indeed be a blocker for failover enabled logical replication. It not only has the potential to disrupt logical replication, but can also impact the primary server. Over time, it may silently lead to significant WAL accumulation on the primary, eventually causing disk-full scenarios and degrading the performance of applications running on the primary instance. Therefore, I too strongly believe this needs to be addressed to prevent such potentially disruptive situations. > > > Proposal: > > Make synchronized_standby_slots quorum aware i.e. extend the GUC to accept an ANY M (slot1, slot2, ...) syntax similar to synchronous_standby_names, so StandbySlotsHaveCaughtup() can return true when M of N slots (where M <= N and M >= 1) have caught up. I still prefer two different GUCs for this as the list of slots to be synchronized can still be different (for example, DBA may want to ensure Geo standby to be sync before allowing the logical decoding client to read the changes). I kept synchronized_standby_slots parse logic similar to synchronous_standby_names to keep things simple. The default behavior is also not changed for synchronized_standby_slots. > Thank you for the proposal. I can spend some time reviewing the changes and help take this forward. I would also be happy to hear others' thoughts and feedback on the proposal. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-02-26T04:58:31Z
Hi, On Wed, Feb 25, 2026 at 7:21 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi Satya, > > On Wed, Feb 25, 2026 at 3:38 AM SATYANARAYANA NARLAPURAM > <satyanarlapuram@gmail.com> wrote: > > > > > > Hi hackers, > > > > synchronized_standby_slots requires that every physical slot listed in the GUC has caught up before a logical failover slot is allowed to proceed with decoding. This is an ALL-of-N slots semantic. The logical slot availability model does not align with quorum replication semantics set using synchronous_standby_names which can be configured for quorum commit (ANY M of N). > > > > In a typical 3 Node HA deployment with quorum sync rep: > > > > Primary, standby1 (corresponds to sb1_slot), standby2 (corresponds to sb2_slot) > > synchronized_standby_slots = ' sb1_slot, sb2_slot' > > synchronous_standby_names = 'Any 1 ('standby1','standby2')' > > > > If standby1 goes down, synchronous commits still succeed because standby2 satisfies the quorum. However, logical decoding blocks indefinitely in WaitForStandbyConfirmation(), waiting for sb1_slot (corresponds to standby1) to catch up — even though the transaction is already safely committed on a quorum of synchronous standbys. This blocks logical decoding consumers from progressing and is inconsistent with the availability guarantee the DBA intended by choosing quorum commit. > > +1. This can indeed be a blocker for failover enabled logical > replication. It not only has the potential to disrupt logical > replication, but can also impact the primary server. Over time, it may > silently lead to significant WAL accumulation on the primary, > eventually causing disk-full scenarios and degrading the performance > of applications running on the primary instance. Therefore, I too > strongly believe this needs to be addressed to prevent such > potentially disruptive situations. > > > > > > > Proposal: > > > > Make synchronized_standby_slots quorum aware i.e. extend the GUC to accept an ANY M (slot1, slot2, ...) syntax similar to synchronous_standby_names, so StandbySlotsHaveCaughtup() can return true when M of N slots (where M <= N and M >= 1) have caught up. I still prefer two different GUCs for this as the list of slots to be synchronized can still be different (for example, DBA may want to ensure Geo standby to be sync before allowing the logical decoding client to read the changes). I kept synchronized_standby_slots parse logic similar to synchronous_standby_names to keep things simple. The default behavior is also not changed for synchronized_standby_slots. > > > > Thank you for the proposal. I can spend some time reviewing the > changes and help take this forward. I would also be happy to hear > others' thoughts and feedback on the proposal. > Thinking about this further, using quorum settings for synchronized_standby_slots can/will certainly result in at least one sync standby lagging behind the logical replica, making it probably impossible to continue with the existing logical replication setup after a failover to the standby that lags behind. Here is what I am mean: Let's say we have 2 synchronous standbys with "synchronized_standby_slots" configured as ANY 1 (sync_standby1, sync_standby2). With this quorum setting, WAL only needs to be confirmed by any one of the two standbys before it can be forwarded to the logical replica. Now consider a scenario where sync_standby1 is ahead of sync_standby2, new WAL gets confirmed by sync_standby1 and subsequently delivered to the logical replica. If sync_standby1 then goes down and we failover to sync_standby2, the new primary will be at a lower LSN than the logical replica, since sync_standby2 never received that WAL. At this point, the logical replication slot on the new primary is essentially stale, and the logical replication setup that existed before the failover cannot be resumed. Hence, I think it's important to ensure that the WAL (including all the necessary data needed for logical replication) gets delivered to all the servers/slots specified in synchronized_standby_slots before it gets delivered to the logical replica. While I agree that not allowing quorum like settings for this has the potential to accumulate WAL and impact logical replication, I think we can explore other ways to mitigate that concern separately. Let's see what experts have to say on this. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Amit Kapila <amit.kapila16@gmail.com> — 2026-02-26T06:19:53Z
On Thu, Feb 26, 2026 at 10:28 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > Proposal: > > > > > > Make synchronized_standby_slots quorum aware i.e. extend the GUC to accept an ANY M (slot1, slot2, ...) syntax similar to synchronous_standby_names, so StandbySlotsHaveCaughtup() can return true when M of N slots (where M <= N and M >= 1) have caught up. I still prefer two different GUCs for this as the list of slots to be synchronized can still be different (for example, DBA may want to ensure Geo standby to be sync before allowing the logical decoding client to read the changes). I kept synchronized_standby_slots parse logic similar to synchronous_standby_names to keep things simple. The default behavior is also not changed for synchronized_standby_slots. > > > ... > > Thinking about this further, using quorum settings for > synchronized_standby_slots can/will certainly result in at least one > sync standby lagging behind the logical replica, making it probably > impossible to continue with the existing logical replication setup > after a failover to the standby that lags behind. Here is what I am > mean: > But won't that be true even for synchronous_standby_names? I think in the case of quorum, it is the responsibility of the failover solution to select the most recent synced standby among all the standby's specified in synchronous_standby_names. Similarly here before failing over logical subscriber to one of physical standby, the failover tool needs to ensure it is switching over to the synced replica. We have given steps in the docs [1] that could be used to identify the replica where the subscriber can switchover. Will that address your concern? BTW, I have also suggested this idea in thread [2]. I don't recall all the ideas/points discussed in that thread but it would be good to check that thread for any alternative ideas and points raised, so that we don't miss anything. [1] - https://www.postgresql.org/docs/current/logical-replication-failover.html [2] - https://www.postgresql.org/message-id/CAA4eK1KLFdmj8CLrZNL0D4phqyQihb7NXOjmqvrU5DT8moQn9Q%40mail.gmail.com -- With Regards, Amit Kapila.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-02-26T07:42:06Z
Hi Amit, On Thu, Feb 26, 2026 at 11:50 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Feb 26, 2026 at 10:28 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > > > > Proposal: > > > > > > > > Make synchronized_standby_slots quorum aware i.e. extend the GUC to accept an ANY M (slot1, slot2, ...) syntax similar to synchronous_standby_names, so StandbySlotsHaveCaughtup() can return true when M of N slots (where M <= N and M >= 1) have caught up. I still prefer two different GUCs for this as the list of slots to be synchronized can still be different (for example, DBA may want to ensure Geo standby to be sync before allowing the logical decoding client to read the changes). I kept synchronized_standby_slots parse logic similar to synchronous_standby_names to keep things simple. The default behavior is also not changed for synchronized_standby_slots. > > > > > ... > > > > Thinking about this further, using quorum settings for > > synchronized_standby_slots can/will certainly result in at least one > > sync standby lagging behind the logical replica, making it probably > > impossible to continue with the existing logical replication setup > > after a failover to the standby that lags behind. Here is what I am > > mean: > > > > But won't that be true even for synchronous_standby_names? I think in > the case of quorum, it is the responsibility of the failover solution > to select the most recent synced standby among all the standby's > specified in synchronous_standby_names. Similarly here before failing > over logical subscriber to one of physical standby, the failover tool > needs to ensure it is switching over to the synced replica. We have > given steps in the docs [1] that could be used to identify the replica > where the subscriber can switchover. Will that address your concern? > Here's my understanding of this: I don't think we should be comparing "synchronous_standby_names" with "synchronized_standby_slots", even though they appear similar in purpose. All values listed in synchronous_standby_names represent synchronous standbys exclusively, whereas synchronized_standby_slots can hold values for both synchronous and asynchronous standbys. In other words, every server referenced by synchronous_standby_names is of the same type, but that may not be the case with synchronized_standby_slots. If a GUC can hold values of different types (sync vs. async), does it really make sense to use a qualifier like ANY 1 (val1, val2) when val1 and val2 are different in nature? For example, suppose val1 is a synchronous standby and val2 is an asynchronous standby, and we configure ANY 1 (val1, val2). It's possible for val2 to get ahead of val1 in terms of replication progress, which in turn could mean the logical replica is also ahead of val1. So if we were to fail over to val1 (since it's the only synchronous standby), we will not be able to use the existing logical replication setup. Please correct me if I have misunderstood anything here. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-02-26T08:02:56Z
Hi Amit, On Wed, Feb 25, 2026 at 10:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > ... > > > > Thinking about this further, using quorum settings for > > synchronized_standby_slots can/will certainly result in at least one > > sync standby lagging behind the logical replica, making it probably > > impossible to continue with the existing logical replication setup > > after a failover to the standby that lags behind. Here is what I am > > mean: > > > > But won't that be true even for synchronous_standby_names? I think in > the case of quorum, it is the responsibility of the failover solution > to select the most recent synced standby among all the standby's > specified in synchronous_standby_names. Similarly here before failing > over logical subscriber to one of physical standby, the failover tool > needs to ensure it is switching over to the synced replica. We have > given steps in the docs [1] that could be used to identify the replica > where the subscriber can switchover. Will that address your concern? > +1, the job of failover orchestration is to ensure the new primary is caught up at least until the quorum LSN. Otherwise, it can be a durability issue where users see missing committed transactions. > BTW, I have also suggested this idea in thread [2]. I don't recall all > the ideas/points discussed in that thread but it would be good to > check that thread for any alternative ideas and points raised, so that > we don't miss anything. > Thanks for sharing the links, the approach is similar. DEFAULT to SAME_AS_SYNCREP_STANDBYS is an interesting option. I like the idea of avoiding duplicate lists unless the user wants to maintain a separate list. Thanks, Satya
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-02-26T08:23:45Z
Hi Ashutosh, On Wed, Feb 25, 2026 at 11:42 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > I don't think we should be comparing "synchronous_standby_names" with > "synchronized_standby_slots", even though they appear similar in > purpose. All values listed in synchronous_standby_names represent > synchronous standbys exclusively, whereas synchronized_standby_slots > can hold values for both synchronous and asynchronous standbys. In > other words, every server referenced by synchronous_standby_names is > of the same type, but that may not be the case with > synchronized_standby_slots. > > If a GUC can hold values of different types (sync vs. async), does it > really make sense to use a qualifier like ANY 1 (val1, val2) when val1 > and val2 are different in nature? For example, suppose val1 is a > synchronous standby and val2 is an asynchronous standby, and we > configure ANY 1 (val1, val2). It's possible for val2 to get ahead of > val1 in terms of replication progress, which in turn could mean the > logical replica is also ahead of val1. So if we were to fail over to > val1 (since it's the only synchronous standby), we will not be able to > use the existing logical replication setup. > If the failover orchestrator cannot ensure standby1 to not get the quorum committed WAL (from archive or standby2) then the setting ANY 1 (val1, val2) is invalid. This setup also has issues because in your scenario, standby2 is ahead of the new primary (standby1) and standby2 requires now to rewind to be in sync with the new primary. Additionally, it allowed readers to read data that was lost at the end of the failover. We ideally need a mechanism to not send WAL to async replicas before the sync replicas commit (honoring syncrhnous_standby_names GUC) feature (similar to synchronized_standby_slots). It could be a different thread on its own.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-02-26T08:45:12Z
On Thu, Feb 26, 2026 at 1:54 PM SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> wrote: > > Hi Ashutosh, > > On Wed, Feb 25, 2026 at 11:42 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >> >> >> I don't think we should be comparing "synchronous_standby_names" with >> "synchronized_standby_slots", even though they appear similar in >> purpose. All values listed in synchronous_standby_names represent >> synchronous standbys exclusively, whereas synchronized_standby_slots >> can hold values for both synchronous and asynchronous standbys. In >> other words, every server referenced by synchronous_standby_names is >> of the same type, but that may not be the case with >> synchronized_standby_slots. >> >> If a GUC can hold values of different types (sync vs. async), does it >> really make sense to use a qualifier like ANY 1 (val1, val2) when val1 >> and val2 are different in nature? For example, suppose val1 is a >> synchronous standby and val2 is an asynchronous standby, and we >> configure ANY 1 (val1, val2). It's possible for val2 to get ahead of >> val1 in terms of replication progress, which in turn could mean the >> logical replica is also ahead of val1. So if we were to fail over to >> val1 (since it's the only synchronous standby), we will not be able to >> use the existing logical replication setup. > > > If the failover orchestrator cannot ensure standby1 to not get the quorum committed WAL (from archive or standby2) then the setting ANY 1 (val1, val2) is invalid. > This setup also has issues because in your scenario, standby2 is ahead of the new primary (standby1) and standby2 requires now to rewind to be in sync with the new primary. Additionally, it allowed readers to read data that was lost at the end of the failover. We ideally need a mechanism to not send WAL to async replicas before the sync replicas commit (honoring syncrhnous_standby_names GUC) feature (similar to synchronized_standby_slots). It could be a different thread on its own. +1 on the overall idea of the patch. I understand the concern raised above that one of the standbys in the quorum (synchronized_standby_slots) might lag behind the logical replica, and a user could potentially failover to such a standby. But I also agree with Amit that configuring failover correctly is ultimately the responsibility of failover-solution. And instructions in doc should be followed before deciding if a standby is failover-ready or not. As suggested in [1], IMO, it is a reasonably good idea for 'synchronized_standby_slots' to DEFAULT to the value of 'synchronous_standby_names'. That way, even if the user missed to configure 'synchronized_standby_slots' explicitly, we would still have reasonable protection in place. At the same time, if a user intentionally chooses not to configure it, a NULL/NONE value should remain a valid option. [1]: https://www.postgresql.org/message-id/CAJpy0uCZ04ZQFHs-tV5LprkYtSSwtBtUJW4O%3D0S01yc%2BTRw7EQ%40mail.gmail.com Thanks, Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-02-26T09:11:42Z
Hi, On Thu, Feb 26, 2026 at 2:15 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Feb 26, 2026 at 1:54 PM SATYANARAYANA NARLAPURAM > <satyanarlapuram@gmail.com> wrote: > > > > Hi Ashutosh, > > > > On Wed, Feb 25, 2026 at 11:42 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > >> > >> > >> I don't think we should be comparing "synchronous_standby_names" with > >> "synchronized_standby_slots", even though they appear similar in > >> purpose. All values listed in synchronous_standby_names represent > >> synchronous standbys exclusively, whereas synchronized_standby_slots > >> can hold values for both synchronous and asynchronous standbys. In > >> other words, every server referenced by synchronous_standby_names is > >> of the same type, but that may not be the case with > >> synchronized_standby_slots. > >> > >> If a GUC can hold values of different types (sync vs. async), does it > >> really make sense to use a qualifier like ANY 1 (val1, val2) when val1 > >> and val2 are different in nature? For example, suppose val1 is a > >> synchronous standby and val2 is an asynchronous standby, and we > >> configure ANY 1 (val1, val2). It's possible for val2 to get ahead of > >> val1 in terms of replication progress, which in turn could mean the > >> logical replica is also ahead of val1. So if we were to fail over to > >> val1 (since it's the only synchronous standby), we will not be able to > >> use the existing logical replication setup. > > > > > > If the failover orchestrator cannot ensure standby1 to not get the quorum committed WAL (from archive or standby2) then the setting ANY 1 (val1, val2) is invalid. > > This setup also has issues because in your scenario, standby2 is ahead of the new primary (standby1) and standby2 requires now to rewind to be in sync with the new primary. Additionally, it allowed readers to read data that was lost at the end of the failover. We ideally need a mechanism to not send WAL to async replicas before the sync replicas commit (honoring syncrhnous_standby_names GUC) feature (similar to synchronized_standby_slots). It could be a different thread on its own. > > > +1 on the overall idea of the patch. > I understand the concern raised above that one of the standbys in the > quorum (synchronized_standby_slots) might lag behind the logical > replica, and a user could potentially failover to such a standby. But > I also agree with Amit that configuring failover correctly is > ultimately the responsibility of failover-solution. And instructions > in doc should be followed before deciding if a standby is > failover-ready or not. > > As suggested in [1], IMO, it is a reasonably good idea for > 'synchronized_standby_slots' to DEFAULT to the value of > 'synchronous_standby_names'. That way, even if the user missed to > configure 'synchronized_standby_slots' explicitly, we would still have > reasonable protection in place. At the same time, if a user > intentionally chooses not to configure it, a NULL/NONE value should > remain a valid option. > AFAIU, not all names listed in "synchronous_standby_names" are necessarily synchronous standbys. Tools like pg_receivewal, for example, can establish a replication connection to the primary and appear in that list. Therefore, deriving "synchronized_standby_slots" from "synchronous_standby_names", if not set by the user would cause logical slots to be synchronized to whatever nodes those names represent, including a host running pg_receivewal, which is certainly not something the user would have intended to do. Therefore I feel this might not just be the good choice. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Alexander Kukushkin <cyberdemn@gmail.com> — 2026-02-26T09:29:29Z
Hi, On Thu, 26 Feb 2026 at 09:45, shveta malik <shveta.malik@gmail.com> wrote: > > As suggested in [1], IMO, it is a reasonably good idea for > 'synchronized_standby_slots' to DEFAULT to the value of > 'synchronous_standby_names'. That way, even if the user missed to > configure 'synchronized_standby_slots' explicitly, we would still have > reasonable protection in place. Hmm. synchronous_standby_names contains application_names, while synchronized_standby_slots contains names of physical replication slots. These are two different things, and in fact sync replication doesn't even require to use replication slots. What is worse, even when all standbys use physical replication slots there is no guarantee that values in synchronous_standby_names will match physical slot names. Regards, -- Alexander Kukushkin
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-02-26T10:38:44Z
Hi Alexnader, On Thu, Feb 26, 2026 at 1:29 AM Alexander Kukushkin <cyberdemn@gmail.com> wrote: > Hi, > > On Thu, 26 Feb 2026 at 09:45, shveta malik <shveta.malik@gmail.com> wrote: > >> >> As suggested in [1], IMO, it is a reasonably good idea for >> 'synchronized_standby_slots' to DEFAULT to the value of >> 'synchronous_standby_names'. That way, even if the user missed to >> configure 'synchronized_standby_slots' explicitly, we would still have >> reasonable protection in place. > > > Hmm. > synchronous_standby_names contains application_names, > while synchronized_standby_slots contains names of physical replication > slots. > These are two different things, and in fact sync replication doesn't even > require to use replication slots. > What is worse, even when all standbys use physical replication slots there > is no guarantee that values in synchronous_standby_names will match > physical slot names > That's right, thanks for reminding me. I am convinced that we can't use the defaults of synchronous_standby_names for synchronized_standby_slots. What do you think about the rest of the proposal? Thanks, Satya
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-02-26T10:46:08Z
Hi Ashutosh, On Thu, Feb 26, 2026 at 1:11 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > Hi, > > On Thu, Feb 26, 2026 at 2:15 PM shveta malik <shveta.malik@gmail.com> > wrote: > > > > On Thu, Feb 26, 2026 at 1:54 PM SATYANARAYANA NARLAPURAM > > <satyanarlapuram@gmail.com> wrote: > > > > > > Hi Ashutosh, > > > > > > On Wed, Feb 25, 2026 at 11:42 PM Ashutosh Sharma < > ashu.coek88@gmail.com> wrote: > > >> > > >> > > >> I don't think we should be comparing "synchronous_standby_names" with > > >> "synchronized_standby_slots", even though they appear similar in > > >> purpose. All values listed in synchronous_standby_names represent > > >> synchronous standbys exclusively, whereas synchronized_standby_slots > > >> can hold values for both synchronous and asynchronous standbys. In > > >> other words, every server referenced by synchronous_standby_names is > > >> of the same type, but that may not be the case with > > >> synchronized_standby_slots. > > >> > > >> If a GUC can hold values of different types (sync vs. async), does it > > >> really make sense to use a qualifier like ANY 1 (val1, val2) when val1 > > >> and val2 are different in nature? For example, suppose val1 is a > > >> synchronous standby and val2 is an asynchronous standby, and we > > >> configure ANY 1 (val1, val2). It's possible for val2 to get ahead of > > >> val1 in terms of replication progress, which in turn could mean the > > >> logical replica is also ahead of val1. So if we were to fail over to > > >> val1 (since it's the only synchronous standby), we will not be able to > > >> use the existing logical replication setup. > > > > > > > > > If the failover orchestrator cannot ensure standby1 to not get the > quorum committed WAL (from archive or standby2) then the setting ANY 1 > (val1, val2) is invalid. > > > This setup also has issues because in your scenario, standby2 is ahead > of the new primary (standby1) and standby2 requires now to rewind to be in > sync with the new primary. Additionally, it allowed readers to read data > that was lost at the end of the failover. We ideally need a mechanism to > not send WAL to async replicas before the sync replicas commit (honoring > syncrhnous_standby_names GUC) feature (similar to > synchronized_standby_slots). It could be a different thread on its own. > > > > > > +1 on the overall idea of the patch. > > I understand the concern raised above that one of the standbys in the > > quorum (synchronized_standby_slots) might lag behind the logical > > replica, and a user could potentially failover to such a standby. But > > I also agree with Amit that configuring failover correctly is > > ultimately the responsibility of failover-solution. And instructions > > in doc should be followed before deciding if a standby is > > failover-ready or not. > > > > As suggested in [1], IMO, it is a reasonably good idea for > > 'synchronized_standby_slots' to DEFAULT to the value of > > 'synchronous_standby_names'. That way, even if the user missed to > > configure 'synchronized_standby_slots' explicitly, we would still have > > reasonable protection in place. At the same time, if a user > > intentionally chooses not to configure it, a NULL/NONE value should > > remain a valid option. > > > > AFAIU, not all names listed in "synchronous_standby_names" are > necessarily synchronous standbys. Tools like pg_receivewal, for > example, can establish a replication connection to the primary and > appear in that list. Therefore, deriving "synchronized_standby_slots" > from "synchronous_standby_names", if not set by the user would cause > logical slots to be synchronized to whatever nodes those names > represent, including a host running pg_receivewal, which is certainly > not something the user would have intended to do. Therefore I feel > this might not just be the good choice. Agreed, not a good idea to have synchronized_standby_slots default to synchronous_standby_names because application_names and slot names are different as stated. Thanks, Satya
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-07T17:16:02Z
Hi All, Submitting a new version of the patch based on Satya's earlier work - [1]. Please take a look and let us know your thoughts. [1] - https://www.postgresql.org/message-id/CAHg%2BQDfU7rOebrLDESPpHSgdiadKbpCOmBokcbmM6Gr%2BA5VobQ%40mail.gmail.com -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-11T08:45:19Z
On Thu, Feb 26, 2026 at 4:16 PM SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> wrote: > > Hi Ashutosh, > > On Thu, Feb 26, 2026 at 1:11 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >> >> Hi, >> >> On Thu, Feb 26, 2026 at 2:15 PM shveta malik <shveta.malik@gmail.com> wrote: >> > >> > On Thu, Feb 26, 2026 at 1:54 PM SATYANARAYANA NARLAPURAM >> > <satyanarlapuram@gmail.com> wrote: >> > > >> > > Hi Ashutosh, >> > > >> > > On Wed, Feb 25, 2026 at 11:42 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >> > >> >> > >> >> > >> I don't think we should be comparing "synchronous_standby_names" with >> > >> "synchronized_standby_slots", even though they appear similar in >> > >> purpose. All values listed in synchronous_standby_names represent >> > >> synchronous standbys exclusively, whereas synchronized_standby_slots >> > >> can hold values for both synchronous and asynchronous standbys. In >> > >> other words, every server referenced by synchronous_standby_names is >> > >> of the same type, but that may not be the case with >> > >> synchronized_standby_slots. >> > >> >> > >> If a GUC can hold values of different types (sync vs. async), does it >> > >> really make sense to use a qualifier like ANY 1 (val1, val2) when val1 >> > >> and val2 are different in nature? For example, suppose val1 is a >> > >> synchronous standby and val2 is an asynchronous standby, and we >> > >> configure ANY 1 (val1, val2). It's possible for val2 to get ahead of >> > >> val1 in terms of replication progress, which in turn could mean the >> > >> logical replica is also ahead of val1. So if we were to fail over to >> > >> val1 (since it's the only synchronous standby), we will not be able to >> > >> use the existing logical replication setup. >> > > >> > > >> > > If the failover orchestrator cannot ensure standby1 to not get the quorum committed WAL (from archive or standby2) then the setting ANY 1 (val1, val2) is invalid. >> > > This setup also has issues because in your scenario, standby2 is ahead of the new primary (standby1) and standby2 requires now to rewind to be in sync with the new primary. Additionally, it allowed readers to read data that was lost at the end of the failover. We ideally need a mechanism to not send WAL to async replicas before the sync replicas commit (honoring syncrhnous_standby_names GUC) feature (similar to synchronized_standby_slots). It could be a different thread on its own. >> > >> > >> > +1 on the overall idea of the patch. >> > I understand the concern raised above that one of the standbys in the >> > quorum (synchronized_standby_slots) might lag behind the logical >> > replica, and a user could potentially failover to such a standby. But >> > I also agree with Amit that configuring failover correctly is >> > ultimately the responsibility of failover-solution. And instructions >> > in doc should be followed before deciding if a standby is >> > failover-ready or not. >> > >> > As suggested in [1], IMO, it is a reasonably good idea for >> > 'synchronized_standby_slots' to DEFAULT to the value of >> > 'synchronous_standby_names'. That way, even if the user missed to >> > configure 'synchronized_standby_slots' explicitly, we would still have >> > reasonable protection in place. At the same time, if a user >> > intentionally chooses not to configure it, a NULL/NONE value should >> > remain a valid option. >> > >> >> AFAIU, not all names listed in "synchronous_standby_names" are >> necessarily synchronous standbys. Tools like pg_receivewal, for >> example, can establish a replication connection to the primary and >> appear in that list. Therefore, deriving "synchronized_standby_slots" >> from "synchronous_standby_names", if not set by the user would cause >> logical slots to be synchronized to whatever nodes those names >> represent, including a host running pg_receivewal, which is certainly >> not something the user would have intended to do. Therefore I feel >> this might not just be the good choice. > > > Agreed, not a good idea to have synchronized_standby_slots default to synchronous_standby_names because application_names and slot names are different as stated. > yes, agreed. Sorry I missed this point earlier. > Submitting a new version of the patch based on Satya's earlier work - [1]. > > Please take a look and let us know your thoughts. Had a look at the patch. Few concerns: 1) StandbySlotsHaveCaughtup: + * If a slot listed in synchronized_standby_slots is not found, + * report an error. -- + * If a slot is not physical, report error. These comments are misleading as we may or may not report ERROR depending upon elevel passed by caller. 2) It seems to me (not tested yet) that even in priority based and quorum based configuration, if we have found our N slots, still we will end up emitting WARNING message for invalidated, missing slots etc such as: a) repplication slot \"%s\" specified in parameter \"%s\" does not exist Logical replication is waiting on the standby associated with replication slot. b) cannot specify logical replication slot \"%s\" in parameter Logical replication is waiting for correction on replication slot. c) physical replication slot \"%s\" specified in parameter \"%s\" has been invalidated Logical replication is waiting on the standby associated with replication slot. These messages may give the impression that logical replication is actually waiting, even though it might already be progressing normally because the required N slots have been found. OTOH, if we suppress these messages, there could be cases where we fail to find the required N valid slots and logical replication genuinely ends up waiting, but the user would receive no indication of the problem. One thing for sure is, that we need to emit such messages when 'wait_for_all' is true, but for rest, we can not decide inline. So there are 2 options: a) either we change the DETAIL slightly with each such reporting to say below or anything better: DETAIL: logical replication may wait if the required number of standby slots is not available. b) Or we collect the slot-names and emit the message at the end only 'if (caught_up_slot_num < required)'. Something like: WARNING: Some replication slots specified in parameter "%s" are not valid: invalidated (slot1, slot2), logical (slot3), missing (slot4). DETAIL: Logical replication is waiting because the required number of standby slots is not available. Thoughts? 3) If elevel is ERROR, do we want to error-out on the first occurence of invalid/missing slot? Shouldn't it look for first valid N slots in case of priority, quorum and then decide for ERROR? Currently it seems it will error out. So, if we go with solution 2b, this can also be resolved with that. ~~ I have not tested the patch, so let me know if I have mis-understood the logic. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-11T10:22:25Z
Hi, Thanks for the review. On Wed, Mar 11, 2026 at 2:15 PM shveta malik <shveta.malik@gmail.com> wrote: > Had a look at the patch. Few concerns: > > 1) > StandbySlotsHaveCaughtup: > > + * If a slot listed in synchronized_standby_slots is not found, > + * report an error. > > -- > > + * If a slot is not physical, report error. > > These comments are misleading as we may or may not report ERROR > depending upon elevel passed by caller. > > 2) > It seems to me (not tested yet) that even in priority based and quorum > based configuration, if we have found our N slots, still we will end > up emitting WARNING message for invalidated, missing slots etc such > as: > > a) > repplication slot \"%s\" specified in parameter \"%s\" does not exist > Logical replication is waiting on the standby associated with replication slot. > > b) > cannot specify logical replication slot \"%s\" in parameter > Logical replication is waiting for correction on replication slot. > > c) > physical replication slot \"%s\" specified in parameter \"%s\" has > been invalidated > Logical replication is waiting on the standby associated with replication slot. > > These messages may give the impression that logical replication is > actually waiting, even though it might already be progressing normally > because the required N slots have been found. > Agreed. We will need to refactor the code around these messages so that they are emitted only when necessary. > OTOH, if we suppress these messages, there could be cases where we > fail to find the required N valid slots and logical replication > genuinely ends up waiting, but the user would receive no indication of > the problem. > > One thing for sure is, that we need to emit such messages when > 'wait_for_all' is true, but for rest, we can not decide inline. > > So there are 2 options: > a) either we change the DETAIL slightly with each such reporting to > say below or anything better: > > DETAIL: logical replication may wait if the required number of standby > slots is not available. > > b) > Or we collect the slot-names and emit the message at the end only 'if > (caught_up_slot_num < required)'. Something like: > > WARNING: Some replication slots specified in parameter "%s" are not > valid: invalidated (slot1, slot2), logical (slot3), missing (slot4). > DETAIL: Logical replication is waiting because the required number of > standby slots is not available. > > > Thoughts? For now, 2b seems like the better option to me as well - I will share here if anything else (better than this) comes to mind while working on it -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ajin Cherian <itsajin@gmail.com> — 2026-03-12T11:17:31Z
On Sun, Mar 8, 2026 at 4:16 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi All, > > Submitting a new version of the patch based on Satya's earlier work - [1]. > > Please take a look and let us know your thoughts. > > [1] - https://www.postgresql.org/message-id/CAHg%2BQDfU7rOebrLDESPpHSgdiadKbpCOmBokcbmM6Gr%2BA5VobQ%40mail.gmail.com > Hi Ashutosh, I was testing this patch and it seems, if the name of the slots starts with first, say firstslot or firstsub, then the patch treats it as FIRST 1 priority mode. I tested with this: synchronized_standby_slots = 'firstsub1, firstsub2' regards, Ajin Cherian Fujitsu Australia
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-12T12:36:48Z
Hi Ajin, On Thu, Mar 12, 2026 at 4:47 PM Ajin Cherian <itsajin@gmail.com> wrote: > > On Sun, Mar 8, 2026 at 4:16 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi All, > > > > Submitting a new version of the patch based on Satya's earlier work - [1]. > > > > Please take a look and let us know your thoughts. > > > > [1] - https://www.postgresql.org/message-id/CAHg%2BQDfU7rOebrLDESPpHSgdiadKbpCOmBokcbmM6Gr%2BA5VobQ%40mail.gmail.com > > > > Hi Ashutosh, > > I was testing this patch and it seems, if the name of the slots starts > with first, say firstslot or firstsub, then the patch treats it as > FIRST 1 priority mode. > Thanks for identifying and reporting this - confirmed, it is indeed an issue. The attached patch addresses it and also adds a regression test case for the same. Additionally, it also fixes the issue related to log message reporting for unavailable/invalid slots that Shveta raised yesterday. Please take a look and feel free to share any further comments. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-13T04:23:29Z
On Thu, Mar 12, 2026 at 6:07 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi Ajin, > > On Thu, Mar 12, 2026 at 4:47 PM Ajin Cherian <itsajin@gmail.com> wrote: > > > > On Sun, Mar 8, 2026 at 4:16 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > Hi All, > > > > > > Submitting a new version of the patch based on Satya's earlier work - [1]. > > > > > > Please take a look and let us know your thoughts. > > > > > > [1] - https://www.postgresql.org/message-id/CAHg%2BQDfU7rOebrLDESPpHSgdiadKbpCOmBokcbmM6Gr%2BA5VobQ%40mail.gmail.com > > > > > > > Hi Ashutosh, > > > > I was testing this patch and it seems, if the name of the slots starts > > with first, say firstslot or firstsub, then the patch treats it as > > FIRST 1 priority mode. > > Good catch. > > Thanks for identifying and reporting this - confirmed, it is indeed an > issue. The attached patch addresses it and also adds a regression test > case for the same. > > Additionally, it also fixes the issue related to log message reporting > for unavailable/invalid slots that Shveta raised yesterday. > > Please take a look and feel free to share any further comments. > Thank You Ashutosh. A few comments: 1) StandbySlotsHaveCaughtup(): + /* If no specific slot state issues but requirement not met (slots still lagging) */ + if (num_slot_states == 0) + { + ereport(elevel, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("waiting for physical standbys corresponding to synchronized standby slots, some slots have not caught up")); + } Above means, when slots are lagging (not invalid), we will emit WARNING or even error out in some cases (based on elevel). IIUC, this behaviour is not the same as base code. Instead of ERROR, it used to wait for lagging slots. Is this change intentional? 2) Do you think we can move the reporting part (the below for-loop) to a new function (may be report_unavailable_sync_standby_slots or anything better) to keep the caught-up logic clean? + for (int i = 0; i < num_slot_states; i++) 3) + /*Record Slot State */ Space needed before Record. 4) + SS_SLOT_OK, /* slot is valid and can participate */ Do we need this value, we are not using it anywhere. 5) check_synchronized_standby_slots(): + /* Reject num_sync > nmembers — it can never be satisfied */ + if (syncrep_parse_result->num_sync > syncrep_parse_result->nmembers) + { + GUC_check_errmsg("number of synchronized standby slots (%d) must not exceed the number of listed slots (%d)", + syncrep_parse_result->num_sync, + syncrep_parse_result->nmembers); + return false; + } Do we need this? Is it ever a possibility? Even if it happens, IIUC, it will be our internal parser logic issue and not the user's GUC configuration fault. So do we mean it to be user facing GUC error? 6) If we plan to retain above, we shall move it before the previous 'if' block: + if (syncrep_parse_result->num_sync == 1 && + syncrep_parse_result->syncrep_method == SYNC_REP_PRIORITY && + syncrep_parse_result->nmembers > 1) thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-13T10:09:29Z
Hi, Thanks for the review, please find comments inline: On Fri, Mar 13, 2026 at 9:53 AM shveta malik <shveta.malik@gmail.com> wrote: > > > 1) > StandbySlotsHaveCaughtup(): > > + /* If no specific slot state issues but requirement not met (slots > still lagging) */ > + if (num_slot_states == 0) > + { > + ereport(elevel, > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg("waiting for physical standbys corresponding to synchronized > standby slots, some slots have not caught up")); > + } > > Above means, when slots are lagging (not invalid), we will emit > WARNING or even error out in some cases (based on elevel). IIUC, this > behaviour is not the same as base code. Instead of ERROR, it used to > wait for lagging slots. Is this change intentional? > Yes it was added intentionally, but the amount of information it was emitting wasn't very much useful. It was just saying "some slots have not caught up" which is already known by the fact that we are waiting. It doesn't say which slots are lagging or by how much, so it was not outputting any actionable information. To make it more useful, this is how it has been changed now: 1) Add a SS_SLOT_LAGGING state to the enum to track slots that were healthy but behind 2) Add a restart_lsn field to SyncStandbySlotsStateInfo to record how far behind each lagging slot is 3) Record lagging slots in the scanning loop (currently they're just skipped/broken out of without being saved) 4) Pass wait_for_lsn to ReportUnavailableSyncStandbySlots so it can report the gap With this change, we would see something like: "replication slot "slot1" has not caught up, the slot's restart_lsn 0/1234 is behind the required 0/5678" This looks more useful and actionable to me. AFA elevel is concerned, it's mostly WARNING except when the server is being stopped or when the cascaded standby is being promoted. So in normal circumstances all these messages will always be reported as WARNING, but still if you see any issues or you want the lagging slot to be reported as WARNING always (even when the elevel is WARNING) do let me know. > 2) > Do you think we can move the reporting part (the below for-loop) to a > new function (may be report_unavailable_sync_standby_slots or anything > better) to keep the caught-up logic clean? > + for (int i = 0; i < num_slot_states; i++) > Okay, moved the entire reporting logic to ReportUnavailableSyncStandbySlots(). This function name pattern was chosen considering the name of other functions in the file. > 3) > > + /*Record Slot State */ > Space needed before Record. > Added missing space. > 4) > + SS_SLOT_OK, /* slot is valid and can participate */ > Do we need this value, we are not using it anywhere. > This was added just to make the enum look a little self documenting, but it's true that it's not being used anywhere hence I removed it. > 5) > check_synchronized_standby_slots(): > + /* Reject num_sync > nmembers — it can never be satisfied */ > + if (syncrep_parse_result->num_sync > syncrep_parse_result->nmembers) > + { > + GUC_check_errmsg("number of synchronized standby slots (%d) must not > exceed the number of listed slots (%d)", > + syncrep_parse_result->num_sync, > + syncrep_parse_result->nmembers); > + return false; > + } > > Do we need this? Is it ever a possibility? Even if it happens, IIUC, > it will be our internal parser logic issue and not the user's GUC > configuration fault. So do we mean it to be user facing GUC error? > We need this to catch invalid settings like 'FIRST 5 (s1, s2, s3);'. AFAICS this is actually a missing part in the check hook for "synchronous_standby_names". It should have been present here as well. > 6) > If we plan to retain above, we shall move it before the previous 'if' block: > > + if (syncrep_parse_result->num_sync == 1 && > + syncrep_parse_result->syncrep_method == SYNC_REP_PRIORITY && > + syncrep_parse_result->nmembers > 1) > I don't see any benefit in moving it upward, but it doesn't have any functional harm as well, anyways I have just moved it up to the place you suggested. PFA patch with all the changes mentioned above and let me know if you have any further comments. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-16T06:30:06Z
On Fri, Mar 13, 2026 at 3:39 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > Thanks for the review, please find comments inline: > > On Fri, Mar 13, 2026 at 9:53 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > 1) > > StandbySlotsHaveCaughtup(): > > > > + /* If no specific slot state issues but requirement not met (slots > > still lagging) */ > > + if (num_slot_states == 0) > > + { > > + ereport(elevel, > > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > + errmsg("waiting for physical standbys corresponding to synchronized > > standby slots, some slots have not caught up")); > > + } > > > > Above means, when slots are lagging (not invalid), we will emit > > WARNING or even error out in some cases (based on elevel). IIUC, this > > behaviour is not the same as base code. Instead of ERROR, it used to > > wait for lagging slots. Is this change intentional? > > > > Yes it was added intentionally, but the amount of information it was > emitting wasn't very much useful. It was just saying "some slots have > not caught up" which is already known by the fact that we are waiting. > It doesn't say which slots are lagging or by how much, so it was not > outputting any actionable information. > > To make it more useful, this is how it has been changed now: > > 1) Add a SS_SLOT_LAGGING state to the enum to track slots that were > healthy but behind > 2) Add a restart_lsn field to SyncStandbySlotsStateInfo to record how > far behind each lagging slot is > 3) Record lagging slots in the scanning loop (currently they're just > skipped/broken out of without being saved) > 4) Pass wait_for_lsn to ReportUnavailableSyncStandbySlots so it can > report the gap > > With this change, we would see something like: > > "replication slot "slot1" has not caught up, the slot's restart_lsn > 0/1234 is behind the required 0/5678" > > This looks more useful and actionable to me. > > AFA elevel is concerned, it's mostly WARNING except when the server is > being stopped or when the cascaded standby is being promoted. So in > normal circumstances all these messages will always be reported as > WARNING, but still if you see any issues or you want the lagging slot > to be reported as WARNING always (even when the elevel is WARNING) do > let me know. I modified the patch to emit WARNING instead of ERROR on lagging case. I tested it for the case where standby is lagging, shutdown of primary waits for standby to catch up: -------------------- WARNING: replication slot "standby_1" specified in parameter "synchronized_standby_slots" has not caught up DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. LOG: received fast shutdown request LOG: aborting any active transactions FATAL: terminating connection due to administrator command LOG: background worker "logical replication launcher" (PID 144863) exited with exit code 1 LOG: shutting down WARNING: replication slot "standby_1" specified in parameter "synchronized_standby_slots" has not caught up DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. WARNING: replication slot "standby_1" specified in parameter "synchronized_standby_slots" has not caught up DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. LOG: released logical replication slot "sub1" ---------------------- This is how it is supposed to be as we want standby to catch-up before the walsender exits on shutdown. Please see below code in WalSndWaitForWal(): /* * If postmaster asked us to stop and the standby slots have caught up * to the flushed position, don't wait anymore. * * It's important to do this check after the recomputation of * RecentFlushPtr, so we can send all remaining data before shutting * down. */ if (got_STOPPING) { if (NeedToWaitForStandbys(RecentFlushPtr, &wait_event)) wait_for_standby_at_stop = true; else break; } But with the new change, walsender will ERROR out if standby is lagging during shutdown. So we should avoid that. We can even skip that complete section (case SS_SLOT_LAGGING in ReportUnavailableSyncStandbySlots). But if we really want to have some information for the user, we can make it DEBUG rather than Warning. IMO, warning sounds alarming for the case where the user can not do much. But let's see what others have to say on this. > > 2) > > Do you think we can move the reporting part (the below for-loop) to a > > new function (may be report_unavailable_sync_standby_slots or anything > > better) to keep the caught-up logic clean? > > + for (int i = 0; i < num_slot_states; i++) > > > > Okay, moved the entire reporting logic to > ReportUnavailableSyncStandbySlots(). This function name pattern was > chosen considering the name of other functions in the file. Thanks, it has better readability now. > > > 3) > > > > + /*Record Slot State */ > > Space needed before Record. > > > > Added missing space. > > > 4) > > + SS_SLOT_OK, /* slot is valid and can participate */ > > Do we need this value, we are not using it anywhere. > > > > This was added just to make the enum look a little self documenting, > but it's true that it's not being used anywhere hence I removed it. > > > 5) > > check_synchronized_standby_slots(): > > + /* Reject num_sync > nmembers — it can never be satisfied */ > > + if (syncrep_parse_result->num_sync > syncrep_parse_result->nmembers) > > + { > > + GUC_check_errmsg("number of synchronized standby slots (%d) must not > > exceed the number of listed slots (%d)", > > + syncrep_parse_result->num_sync, > > + syncrep_parse_result->nmembers); > > + return false; > > + } > > > > Do we need this? Is it ever a possibility? Even if it happens, IIUC, > > it will be our internal parser logic issue and not the user's GUC > > configuration fault. So do we mean it to be user facing GUC error? > > > > We need this to catch invalid settings like 'FIRST 5 (s1, s2, s3);'. > AFAICS this is actually a missing part in the check hook for > "synchronous_standby_names". It should have been present here as well. Okay, I see this is for a wrong configuration. > > 6) > > If we plan to retain above, we shall move it before the previous 'if' block: > > > > + if (syncrep_parse_result->num_sync == 1 && > > + syncrep_parse_result->syncrep_method == SYNC_REP_PRIORITY && > > + syncrep_parse_result->nmembers > 1) > > > > I don't see any benefit in moving it upward, but it doesn't have any > functional harm as well, anyways I have just moved it up to the place > you suggested. Please do as you feel fit. This comment was when I did not not have a valid scenario in mind for the concerned case. > PFA patch with all the changes mentioned above and let me know if you > have any further comments. > Will review the rest of the changes. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-16T09:03:21Z
Hi, On Mon, Mar 16, 2026 at 12:00 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Fri, Mar 13, 2026 at 3:39 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi, > > > > Thanks for the review, please find comments inline: > > > > On Fri, Mar 13, 2026 at 9:53 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > > 1) > > > StandbySlotsHaveCaughtup(): > > > > > > + /* If no specific slot state issues but requirement not met (slots > > > still lagging) */ > > > + if (num_slot_states == 0) > > > + { > > > + ereport(elevel, > > > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > > + errmsg("waiting for physical standbys corresponding to synchronized > > > standby slots, some slots have not caught up")); > > > + } > > > > > > Above means, when slots are lagging (not invalid), we will emit > > > WARNING or even error out in some cases (based on elevel). IIUC, this > > > behaviour is not the same as base code. Instead of ERROR, it used to > > > wait for lagging slots. Is this change intentional? > > > > > > > Yes it was added intentionally, but the amount of information it was > > emitting wasn't very much useful. It was just saying "some slots have > > not caught up" which is already known by the fact that we are waiting. > > It doesn't say which slots are lagging or by how much, so it was not > > outputting any actionable information. > > > > To make it more useful, this is how it has been changed now: > > > > 1) Add a SS_SLOT_LAGGING state to the enum to track slots that were > > healthy but behind > > 2) Add a restart_lsn field to SyncStandbySlotsStateInfo to record how > > far behind each lagging slot is > > 3) Record lagging slots in the scanning loop (currently they're just > > skipped/broken out of without being saved) > > 4) Pass wait_for_lsn to ReportUnavailableSyncStandbySlots so it can > > report the gap > > > > With this change, we would see something like: > > > > "replication slot "slot1" has not caught up, the slot's restart_lsn > > 0/1234 is behind the required 0/5678" > > > > This looks more useful and actionable to me. > > > > AFA elevel is concerned, it's mostly WARNING except when the server is > > being stopped or when the cascaded standby is being promoted. So in > > normal circumstances all these messages will always be reported as > > WARNING, but still if you see any issues or you want the lagging slot > > to be reported as WARNING always (even when the elevel is WARNING) do > > let me know. > > I modified the patch to emit WARNING instead of ERROR on lagging case. > I tested it for the case where standby is lagging, shutdown of primary > waits for standby to catch up: > > -------------------- > WARNING: replication slot "standby_1" specified in parameter > "synchronized_standby_slots" has not caught up > DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. > LOG: received fast shutdown request > LOG: aborting any active transactions > FATAL: terminating connection due to administrator command > LOG: background worker "logical replication launcher" (PID 144863) > exited with exit code 1 > LOG: shutting down > WARNING: replication slot "standby_1" specified in parameter > "synchronized_standby_slots" has not caught up > DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. > WARNING: replication slot "standby_1" specified in parameter > "synchronized_standby_slots" has not caught up > DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. > LOG: released logical replication slot "sub1" > ---------------------- > > This is how it is supposed to be as we want standby to catch-up before > the walsender exits on shutdown. Please see below code in > WalSndWaitForWal(): > > /* > * If postmaster asked us to stop and the standby slots have caught up > * to the flushed position, don't wait anymore. > * > * It's important to do this check after the recomputation of > * RecentFlushPtr, so we can send all remaining data before shutting > * down. > */ > if (got_STOPPING) > { > if (NeedToWaitForStandbys(RecentFlushPtr, &wait_event)) > wait_for_standby_at_stop = true; > else > break; > } > > > But with the new change, walsender will ERROR out if standby is > lagging during shutdown. So we should avoid that. We can even skip > that complete section (case SS_SLOT_LAGGING in > ReportUnavailableSyncStandbySlots). But if we really want to have some > information for the user, we can make it DEBUG rather than Warning. > IMO, warning sounds alarming for the case where the user can not do > much. But let's see what others have to say on this. > Yes, we should avoid using ERROR in this case. DEBUG looks like a better choice to me, as WARNING can be somewhat noisy at times and this appears to be more like debugging information. However, as you suggested, we can wait and see what others think. > > > 2) > > > Do you think we can move the reporting part (the below for-loop) to a > > > new function (may be report_unavailable_sync_standby_slots or anything > > > better) to keep the caught-up logic clean? > > > + for (int i = 0; i < num_slot_states; i++) > > > > > > > Okay, moved the entire reporting logic to > > ReportUnavailableSyncStandbySlots(). This function name pattern was > > chosen considering the name of other functions in the file. > > Thanks, it has better readability now. > > > > > > 3) > > > > > > + /*Record Slot State */ > > > Space needed before Record. > > > > > > > Added missing space. > > > > > 4) > > > + SS_SLOT_OK, /* slot is valid and can participate */ > > > Do we need this value, we are not using it anywhere. > > > > > > > This was added just to make the enum look a little self documenting, > > but it's true that it's not being used anywhere hence I removed it. > > > > > 5) > > > check_synchronized_standby_slots(): > > > + /* Reject num_sync > nmembers — it can never be satisfied */ > > > + if (syncrep_parse_result->num_sync > syncrep_parse_result->nmembers) > > > + { > > > + GUC_check_errmsg("number of synchronized standby slots (%d) must not > > > exceed the number of listed slots (%d)", > > > + syncrep_parse_result->num_sync, > > > + syncrep_parse_result->nmembers); > > > + return false; > > > + } > > > > > > Do we need this? Is it ever a possibility? Even if it happens, IIUC, > > > it will be our internal parser logic issue and not the user's GUC > > > configuration fault. So do we mean it to be user facing GUC error? > > > > > > > We need this to catch invalid settings like 'FIRST 5 (s1, s2, s3);'. > > AFAICS this is actually a missing part in the check hook for > > "synchronous_standby_names". It should have been present here as well. > > Okay, I see this is for a wrong configuration. > > > > 6) > > > If we plan to retain above, we shall move it before the previous 'if' block: > > > > > > + if (syncrep_parse_result->num_sync == 1 && > > > + syncrep_parse_result->syncrep_method == SYNC_REP_PRIORITY && > > > + syncrep_parse_result->nmembers > 1) > > > > > > > I don't see any benefit in moving it upward, but it doesn't have any > > functional harm as well, anyways I have just moved it up to the place > > you suggested. > > Please do as you feel fit. This comment was when I did not not have a > valid scenario in mind for the concerned case. > > > PFA patch with all the changes mentioned above and let me know if you > > have any further comments. > > > > Will review the rest of the changes. > Sure, thanks. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-16T11:27:32Z
On Mon, Mar 16, 2026 at 2:33 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > Will review the rest of the changes. > > > > Sure, thanks. > Please find comments for the review so far: 1) + Specifies which streaming replication standby server slots logical WAL + sender processes must wait for before delivering decoded changes. Shall we rephrase a bit for better readability: Specifies the streaming replication standby server slots that logical WAL sender processes must wait for before delivering decoded changes. 2) + If a slot is missing or invalidated, it will be skipped. Should we mention 'inactive' too? 3) + The keyword <literal>ANY</literal>, coupled with + <replaceable class="parameter">num_sync</replaceable>, specifies + quorum-based semantics. Logical decoding proceeds once at least + <replaceable class="parameter">num_sync</replaceable> of the listed + slots have caught up. Missing, invalidated, or lagging slots are + skipped when looking for the required number of caught-up slots. + For example, a setting of <literal>ANY 1 (sb1_slot, sb2_slot)</literal> + will allow logical decoding to proceed as soon as either physical slot + has confirmed WAL receipt. This is useful in conjunction with + quorum-based synchronous replication + (<literal>synchronous_standby_names = 'ANY ...'</literal>), so that + logical decoding availability matches the commit durability guarantee. IMO, it is not completely correct to say 'Missing, invalidated, or lagging slots are skipped'. This is because lagging slots are not skipped outright; they just don’t contribute toward the quorum until they catch up. If all slots are lagging, logical decoding still waits for enough slots to catch up. Should we instead say this: (or anything better you have in mind) The keyword ANY, coupled with num_sync, specifies quorum-based semantics. Logical decoding proceeds once at least num_sync of the listed slots have caught up. Missing or invalidated slots are skipped when determining candidates, and lagging slots simply do not count toward the required number until they catch up, i.e., there is no priority ordering. For example, a setting of ANY 1 (sb1_slot, sb2_slot) allows ....... 3) Existing doc: Note that logical replication will not proceed if the slots specified in synchronized_standby_slots do not exist or are invalidated. Shall we change it to: 'if the slots' --> 'if the required number of physical slots' 4) + <literal>FIRST</literal> and <literal>ANY</literal> are case-insensitive. + If these keywords are used as the name of a replication slot, + the <replaceable class="parameter">slot_name</replaceable> must + be double-quoted. + </para> + <para> + This guarantees that logical replication failover slots do not consume changes until those changes are received + and flushed to the required physical standbys. If a The sentence 'This guarantees that ...' does not appear to follow naturally from the preceding sentence about FIRST and ANY. The reference of 'This' is unclear. For better readability, should we replace 'This guarantees that' with 'The use of synchronized_standby_slots guarantees that ...'? 5) slot.c: + * The layout mirrors SyncRepConfigData so that the same quorum / priority quorum / priority --> quorum/priority 6) + * This reuses the syncrep_yyparse / syncrep_scanner infrastructure that is syncrep_yyparse / syncrep_scanner --> syncrep_yyparse/syncrep_scanner 7) IsExplicitFirstSyncStandbySlotsSyntax Should we rename to: IsPrioritySyncStandbySlotsSyntax() 8) I would like to understand how synchronous_standby_names deal with such a case: 'first as server name or FIRST as priority syntax'. I could not find any such function/logic there. 9) IsExplicitFirstSyncStandbySlotsSyntax(): + /* Explicit FIRST syntax then requires a parenthesized member list */ + while (*p && isspace((unsigned char) *p)) + p++; + + return (*p == '('); I could not find a case where we can reach above flow without '('. If we try to give without parenthese (say: first 1 slot1,slot2,slot3), it will be caught by syncrep_yyparse() itself. Shouldn't just checking FIRST + space(s)+ digit suffice? Or let me know if I am missing any case. 10) + * To distinguish simple list from explicit "FIRST N (...)", check + * whether the value starts with the FIRST keyword (after whitespace). It will be good to give example: To distinguish simple list starting with 'first' such as 'firstslot, secondslot',... And it will be better if we move this comment inside if-block atop 'IsExplicitFirstSyncStandbySlotsSyntax' call. 11) + * Simple list (e.g., "slot1, slot2"): + * ALL slots must be present, valid, and caught up. Returns false + * immediately if any slot is missing, invalid, or lagging. We can simply say: ALL slots must have caught up, returns false otherwise. But if we want to mention the states too, we shall mention 'inactive' as well, plus 'logical' to make the information complete. For rest of the comment too: missing/invalid --> missing/invalid/inactive/logical thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-17T09:25:53Z
On Mon, Mar 16, 2026 at 2:33 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Mon, Mar 16, 2026 at 12:00 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Fri, Mar 13, 2026 at 3:39 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > Hi, > > > > > > Thanks for the review, please find comments inline: > > > > > > On Fri, Mar 13, 2026 at 9:53 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > > > > > 1) > > > > StandbySlotsHaveCaughtup(): > > > > > > > > + /* If no specific slot state issues but requirement not met (slots > > > > still lagging) */ > > > > + if (num_slot_states == 0) > > > > + { > > > > + ereport(elevel, > > > > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > > > + errmsg("waiting for physical standbys corresponding to synchronized > > > > standby slots, some slots have not caught up")); > > > > + } > > > > > > > > Above means, when slots are lagging (not invalid), we will emit > > > > WARNING or even error out in some cases (based on elevel). IIUC, this > > > > behaviour is not the same as base code. Instead of ERROR, it used to > > > > wait for lagging slots. Is this change intentional? > > > > > > > > > > Yes it was added intentionally, but the amount of information it was > > > emitting wasn't very much useful. It was just saying "some slots have > > > not caught up" which is already known by the fact that we are waiting. > > > It doesn't say which slots are lagging or by how much, so it was not > > > outputting any actionable information. > > > > > > To make it more useful, this is how it has been changed now: > > > > > > 1) Add a SS_SLOT_LAGGING state to the enum to track slots that were > > > healthy but behind > > > 2) Add a restart_lsn field to SyncStandbySlotsStateInfo to record how > > > far behind each lagging slot is > > > 3) Record lagging slots in the scanning loop (currently they're just > > > skipped/broken out of without being saved) > > > 4) Pass wait_for_lsn to ReportUnavailableSyncStandbySlots so it can > > > report the gap > > > > > > With this change, we would see something like: > > > > > > "replication slot "slot1" has not caught up, the slot's restart_lsn > > > 0/1234 is behind the required 0/5678" > > > > > > This looks more useful and actionable to me. > > > > > > AFA elevel is concerned, it's mostly WARNING except when the server is > > > being stopped or when the cascaded standby is being promoted. So in > > > normal circumstances all these messages will always be reported as > > > WARNING, but still if you see any issues or you want the lagging slot > > > to be reported as WARNING always (even when the elevel is WARNING) do > > > let me know. > > > > I modified the patch to emit WARNING instead of ERROR on lagging case. > > I tested it for the case where standby is lagging, shutdown of primary > > waits for standby to catch up: > > > > -------------------- > > WARNING: replication slot "standby_1" specified in parameter > > "synchronized_standby_slots" has not caught up > > DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. > > LOG: received fast shutdown request > > LOG: aborting any active transactions > > FATAL: terminating connection due to administrator command > > LOG: background worker "logical replication launcher" (PID 144863) > > exited with exit code 1 > > LOG: shutting down > > WARNING: replication slot "standby_1" specified in parameter > > "synchronized_standby_slots" has not caught up > > DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. > > WARNING: replication slot "standby_1" specified in parameter > > "synchronized_standby_slots" has not caught up > > DETAIL: The slot's restart_lsn 0/26D1D250 is behind the required 0/26D1D2C0. > > LOG: released logical replication slot "sub1" > > ---------------------- > > > > This is how it is supposed to be as we want standby to catch-up before > > the walsender exits on shutdown. Please see below code in > > WalSndWaitForWal(): > > > > /* > > * If postmaster asked us to stop and the standby slots have caught up > > * to the flushed position, don't wait anymore. > > * > > * It's important to do this check after the recomputation of > > * RecentFlushPtr, so we can send all remaining data before shutting > > * down. > > */ > > if (got_STOPPING) > > { > > if (NeedToWaitForStandbys(RecentFlushPtr, &wait_event)) > > wait_for_standby_at_stop = true; > > else > > break; > > } > > > > > > But with the new change, walsender will ERROR out if standby is > > lagging during shutdown. So we should avoid that. We can even skip > > that complete section (case SS_SLOT_LAGGING in > > ReportUnavailableSyncStandbySlots). But if we really want to have some > > information for the user, we can make it DEBUG rather than Warning. > > IMO, warning sounds alarming for the case where the user can not do > > much. But let's see what others have to say on this. > > > > Yes, we should avoid using ERROR in this case. DEBUG looks like a > better choice to me, as WARNING can be somewhat noisy at times and > this appears to be more like debugging information. However, as you > suggested, we can wait and see what others think. > +1 for elevel as DEBUG1 in this case. BTW, IIRC, we need to wait here because otherwise standby's may miss some information that is sent to logical subscribers. This can happen because the walsender wait for all WAL records to be sent to logical subscribers before shutdown (as mentioned in the top of file in para: "If the server is shut down ..." and the code in WalSndDone() suggests the same). -- With Regards, Amit Kapila. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-18T04:45:16Z
Hi, Thanks again for reviewing the patch. Please my my responses inline below: On Mon, Mar 16, 2026 at 4:57 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Mon, Mar 16, 2026 at 2:33 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > Will review the rest of the changes. > > > > > > > Sure, thanks. > > > > Please find comments for the review so far: > > 1) > + Specifies which streaming replication standby server slots logical WAL > + sender processes must wait for before delivering decoded changes. > > Shall we rephrase a bit for better readability: > > Specifies the streaming replication standby server slots that logical > WAL sender processes must wait for before delivering decoded changes. > The suggested change looks good, applied in the attached patch. > 2) > + If a slot is missing or invalidated, it will be skipped. > > Should we mention 'inactive' too? > Yes, mentioned about all the slots that are being skipped. > 3) > + The keyword <literal>ANY</literal>, coupled with > + <replaceable class="parameter">num_sync</replaceable>, specifies > + quorum-based semantics. Logical decoding proceeds once at least > + <replaceable class="parameter">num_sync</replaceable> of the listed > + slots have caught up. Missing, invalidated, or lagging slots are > + skipped when looking for the required number of caught-up slots. > + For example, a setting of <literal>ANY 1 (sb1_slot, sb2_slot)</literal> > + will allow logical decoding to proceed as soon as either physical slot > + has confirmed WAL receipt. This is useful in conjunction with > + quorum-based synchronous replication > + (<literal>synchronous_standby_names = 'ANY ...'</literal>), so that > + logical decoding availability matches the commit durability guarantee. > > IMO, it is not completely correct to say 'Missing, invalidated, or > lagging slots are skipped'. This is because lagging slots are not > skipped outright; they just don’t contribute toward the quorum until > they catch up. If all slots are lagging, logical decoding still waits > for enough slots to catch up. > > Should we instead say this: (or anything better you have in mind) > > The keyword ANY, coupled with num_sync, specifies quorum-based > semantics. Logical decoding proceeds once at least num_sync of the > listed slots have caught up. Missing or invalidated slots are skipped > when determining candidates, and lagging slots simply do not count > toward the required number until they catch up, i.e., there is no > priority ordering. For example, a setting of ANY 1 (sb1_slot, > sb2_slot) allows ....... > Looks good, applied this as well in the attached patch. > 3) > Existing doc: > Note that logical replication will not proceed if the slots specified > in synchronized_standby_slots do not exist or are invalidated. > > Shall we change it to: > 'if the slots' --> 'if the required number of physical slots' > Changed as suggested. > 4) > > + <literal>FIRST</literal> and <literal>ANY</literal> are > case-insensitive. > + If these keywords are used as the name of a replication slot, > + the <replaceable class="parameter">slot_name</replaceable> must > + be double-quoted. > + </para> > + <para> > + This guarantees that logical replication > failover slots do not consume changes until those changes are received > + and flushed to the required physical standbys. If a > > The sentence 'This guarantees that ...' does not appear to follow > naturally from the preceding sentence about FIRST and ANY. The > reference of 'This' is unclear. For better readability, should we > replace 'This guarantees that' with 'The use of > synchronized_standby_slots guarantees that ...'? > We can. Modified likewise in the attached patch. > 5) > slot.c: > > + * The layout mirrors SyncRepConfigData so that the same quorum / priority > > quorum / priority --> quorum/priority > Fixed. > 6) > + * This reuses the syncrep_yyparse / syncrep_scanner infrastructure that is > > syncrep_yyparse / syncrep_scanner --> syncrep_yyparse/syncrep_scanner > Fixed. > 7) > IsExplicitFirstSyncStandbySlotsSyntax > > Should we rename to: IsPrioritySyncStandbySlotsSyntax() > > 8) > I would like to understand how synchronous_standby_names deal with > such a case: 'first as server name or FIRST as priority syntax'. I > could not find any such function/logic there. > Renamed as suggested. > 9) > IsExplicitFirstSyncStandbySlotsSyntax(): > > + /* Explicit FIRST syntax then requires a parenthesized member list */ > + while (*p && isspace((unsigned char) *p)) > + p++; > + > + return (*p == '('); > > I could not find a case where we can reach above flow without '('. If > we try to give without parenthese (say: first 1 slot1,slot2,slot3), it > will be caught by syncrep_yyparse() itself. Shouldn't just checking > FIRST + space(s)+ digit suffice? Or let me know if I am missing any > case. > Without '(', control would never reach this point. I would still retain it to ensure that when this function returns true, it has fully validated the 'FIRST (' syntax. > 10) > + * To distinguish simple list from explicit "FIRST N (...)", check > + * whether the value starts with the FIRST keyword (after whitespace). > > It will be good to give example: > > To distinguish simple list starting with 'first' such as 'firstslot, > secondslot',... > > And it will be better if we move this comment inside if-block atop > 'IsExplicitFirstSyncStandbySlotsSyntax' call. > Done. > 11) > + * Simple list (e.g., "slot1, slot2"): > + * ALL slots must be present, valid, and caught up. Returns false > + * immediately if any slot is missing, invalid, or lagging. > > We can simply say: > ALL slots must have caught up, returns false otherwise. > > But if we want to mention the states too, we shall mention 'inactive' > as well, plus 'logical' to make the information complete. > > For rest of the comment too: > missing/invalid --> missing/invalid/inactive/logical > Fixed. Other than these, I have also changed elevel to DEBUG1 for the message reported for lagging slots. PFA patch with all these changes. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-18T05:08:37Z
On Wed, Mar 18, 2026 at 10:15 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > Thanks again for reviewing the patch. Please my my responses inline below: > > On Mon, Mar 16, 2026 at 4:57 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Mon, Mar 16, 2026 at 2:33 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > Will review the rest of the changes. > > > > > > > > > > Sure, thanks. > > > > > > > Please find comments for the review so far: > > > > 1) > > + Specifies which streaming replication standby server slots logical WAL > > + sender processes must wait for before delivering decoded changes. > > > > Shall we rephrase a bit for better readability: > > > > Specifies the streaming replication standby server slots that logical > > WAL sender processes must wait for before delivering decoded changes. > > > > The suggested change looks good, applied in the attached patch. > > > 2) > > + If a slot is missing or invalidated, it will be skipped. > > > > Should we mention 'inactive' too? > > > > Yes, mentioned about all the slots that are being skipped. > > > 3) > > + The keyword <literal>ANY</literal>, coupled with > > + <replaceable class="parameter">num_sync</replaceable>, specifies > > + quorum-based semantics. Logical decoding proceeds once at least > > + <replaceable class="parameter">num_sync</replaceable> of the listed > > + slots have caught up. Missing, invalidated, or lagging slots are > > + skipped when looking for the required number of caught-up slots. > > + For example, a setting of <literal>ANY 1 (sb1_slot, sb2_slot)</literal> > > + will allow logical decoding to proceed as soon as either physical slot > > + has confirmed WAL receipt. This is useful in conjunction with > > + quorum-based synchronous replication > > + (<literal>synchronous_standby_names = 'ANY ...'</literal>), so that > > + logical decoding availability matches the commit durability guarantee. > > > > IMO, it is not completely correct to say 'Missing, invalidated, or > > lagging slots are skipped'. This is because lagging slots are not > > skipped outright; they just don’t contribute toward the quorum until > > they catch up. If all slots are lagging, logical decoding still waits > > for enough slots to catch up. > > > > Should we instead say this: (or anything better you have in mind) > > > > The keyword ANY, coupled with num_sync, specifies quorum-based > > semantics. Logical decoding proceeds once at least num_sync of the > > listed slots have caught up. Missing or invalidated slots are skipped > > when determining candidates, and lagging slots simply do not count > > toward the required number until they catch up, i.e., there is no > > priority ordering. For example, a setting of ANY 1 (sb1_slot, > > sb2_slot) allows ....... > > > > Looks good, applied this as well in the attached patch. > > > 3) > > Existing doc: > > Note that logical replication will not proceed if the slots specified > > in synchronized_standby_slots do not exist or are invalidated. > > > > Shall we change it to: > > 'if the slots' --> 'if the required number of physical slots' > > > > Changed as suggested. > > > 4) > > > > + <literal>FIRST</literal> and <literal>ANY</literal> are > > case-insensitive. > > + If these keywords are used as the name of a replication slot, > > + the <replaceable class="parameter">slot_name</replaceable> must > > + be double-quoted. > > + </para> > > + <para> > > + This guarantees that logical replication > > failover slots do not consume changes until those changes are received > > + and flushed to the required physical standbys. If a > > > > The sentence 'This guarantees that ...' does not appear to follow > > naturally from the preceding sentence about FIRST and ANY. The > > reference of 'This' is unclear. For better readability, should we > > replace 'This guarantees that' with 'The use of > > synchronized_standby_slots guarantees that ...'? > > > > We can. Modified likewise in the attached patch. > > > 5) > > slot.c: > > > > + * The layout mirrors SyncRepConfigData so that the same quorum / priority > > > > quorum / priority --> quorum/priority > > > > Fixed. > > > 6) > > + * This reuses the syncrep_yyparse / syncrep_scanner infrastructure that is > > > > syncrep_yyparse / syncrep_scanner --> syncrep_yyparse/syncrep_scanner > > > > Fixed. > > > 7) > > IsExplicitFirstSyncStandbySlotsSyntax > > > > Should we rename to: IsPrioritySyncStandbySlotsSyntax() > > > > 8) > > I would like to understand how synchronous_standby_names deal with > > such a case: 'first as server name or FIRST as priority syntax'. I > > could not find any such function/logic there. > > Let me know if you understand this part, else I will debug it once. > > Renamed as suggested. > > > 9) > > IsExplicitFirstSyncStandbySlotsSyntax(): > > > > + /* Explicit FIRST syntax then requires a parenthesized member list */ > > + while (*p && isspace((unsigned char) *p)) > > + p++; > > + > > + return (*p == '('); > > > > I could not find a case where we can reach above flow without '('. If > > we try to give without parenthese (say: first 1 slot1,slot2,slot3), it > > will be caught by syncrep_yyparse() itself. Shouldn't just checking > > FIRST + space(s)+ digit suffice? Or let me know if I am missing any > > case. > > > > Without '(', control would never reach this point. I would still > retain it to ensure that when this function returns true, it has fully > validated the 'FIRST (' syntax. > Okay, works for me. > > > 10) > > + * To distinguish simple list from explicit "FIRST N (...)", check > > + * whether the value starts with the FIRST keyword (after whitespace). > > > > It will be good to give example: > > > > To distinguish simple list starting with 'first' such as 'firstslot, > > secondslot',... > > > > And it will be better if we move this comment inside if-block atop > > 'IsExplicitFirstSyncStandbySlotsSyntax' call. > > > > Done. > > > 11) > > + * Simple list (e.g., "slot1, slot2"): > > + * ALL slots must be present, valid, and caught up. Returns false > > + * immediately if any slot is missing, invalid, or lagging. > > > > We can simply say: > > ALL slots must have caught up, returns false otherwise. > > > > But if we want to mention the states too, we shall mention 'inactive' > > as well, plus 'logical' to make the information complete. > > > > For rest of the comment too: > > missing/invalid --> missing/invalid/inactive/logical > > > > Fixed. > > Other than these, I have also changed elevel to DEBUG1 for the message > reported for lagging slots. PFA patch with all these changes. > Thanks, will review further. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-18T05:27:02Z
On Wed, Mar 18, 2026 at 10:38 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Wed, Mar 18, 2026 at 10:15 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > 8) > > > I would like to understand how synchronous_standby_names deal with > > > such a case: 'first as server name or FIRST as priority syntax'. I > > > could not find any such function/logic there. > > > > > Let me know if you understand this part, else I will debug it once. > This is not needed for synchronous_standby_names because specifying a list of values without the FIRST/ANY keyword is implicitly treated as FIRST 1, which the syncrep parser already handles. However, this behavior differs for synchronized_standby_slots, where a list of values without FIRST/ANY is treated as ALL mode. Since we reuse the syncrep parser here, we need to distinguish whether the FIRST keyword was explicitly provided by the user, which is the purpose of the IsExplicitFirstSyncStandbySlotsSyntax function. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-18T06:31:38Z
On Wed, Mar 18, 2026 at 10:57 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Wed, Mar 18, 2026 at 10:38 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Wed, Mar 18, 2026 at 10:15 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > 8) > > > > I would like to understand how synchronous_standby_names deal with > > > > such a case: 'first as server name or FIRST as priority syntax'. I > > > > could not find any such function/logic there. > > > > > > > > Let me know if you understand this part, else I will debug it once. > > > > This is not needed for synchronous_standby_names because specifying a > list of values without the FIRST/ANY keyword is implicitly treated as > FIRST 1, which the syncrep parser already handles. However, this > behavior differs for synchronized_standby_slots, where a list of > values without FIRST/ANY is treated as ALL mode. Since we reuse the > syncrep parser here, we need to distinguish whether the FIRST keyword > was explicitly provided by the user, which is the purpose of the > IsExplicitFirstSyncStandbySlotsSyntax function. > Ah, got it. Thanks for the details, no idea how I missed that. Few comments: 1) + +# Physical replication slots for the two standbys +$primary->safe_psql('postgres', + "SELECT pg_create_physical_replication_slot('sb1_slot');"); +$primary->safe_psql('postgres', + "SELECT pg_create_physical_replication_slot('sb2_slot');"); +$primary->safe_psql('postgres', + "SELECT pg_create_physical_replication_slot('first_slot');"); Can we please write a comment atop 'first_slot' creation for its purpose. The comment '+# Physical replication slots for the two standbys' confuses as we are creating 3 slots following that comment. 2) +# PART A: Plain list (ALL mode) blocks when any slot is unavailable +# PART C: Verify plain list (ALL mode) requires ALL slots I think we can merge the 2 tests. Is there a specific reason we have it in 2 different sections (am I missing something?). Part A can first test blocking on inactive standby (which it is doing already) and then restart standby and can check that logical decoding now proceeds. Part C can then be removed. 3) Same with these 2: +# PART B: ANY mode (quorum) — logical decoding proceeds with N-of-M slots +# PART D: ANY mode — verify it works with either standby Part B tests that it works when standby1 is down. Part D tests that it works when standby2 is down and also works when both are up. I think we don't need these duplicate test cases (unless I am missing something?), we can merge Part D to Part B itself i.e. test with standby1 down and up in a single test. 4) These 2 tests are basically testing the same thing. Do we need both? +# FIRST 1 should skip sb1_slot (unavailable) and use sb2_slot. +# Test that FIRST 1 is different from plain list — FIRST 1 succeeds with one down. 5) I see that we only need 'first_slot' for the last test and do not have even a standby associated with it. So we can even move it's creation to that test itself instead of creating it in the beginning and adding comments to explain 'why'. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-18T10:38:27Z
Hi, Thanks again for reviewing the test cases. Please find my comments inline below: On Wed, Mar 18, 2026 at 12:01 PM shveta malik <shveta.malik@gmail.com> wrote: > > > Few comments: > > 1) > + > +# Physical replication slots for the two standbys > +$primary->safe_psql('postgres', > + "SELECT pg_create_physical_replication_slot('sb1_slot');"); > +$primary->safe_psql('postgres', > + "SELECT pg_create_physical_replication_slot('sb2_slot');"); > +$primary->safe_psql('postgres', > + "SELECT pg_create_physical_replication_slot('first_slot');"); > > Can we please write a comment atop 'first_slot' creation for its purpose. > The comment '+# Physical replication slots for the two standbys' > confuses as we are creating 3 slots following that comment. > Moved this to PART E (the test-case where this slot is being used). This is also suggested in one of your comments below. > 2) > +# PART A: Plain list (ALL mode) blocks when any slot is unavailable > > +# PART C: Verify plain list (ALL mode) requires ALL slots > > I think we can merge the 2 tests. Is there a specific reason we have > it in 2 different sections (am I missing something?). > > Part A can first test blocking on inactive standby (which it is doing > already) and then restart standby and can check that logical decoding > now proceeds. Part C can then be removed. > I did not remove PART C entirely, however, I removed the portions that overlapped with PART A. I would suggest not just evaluating each test case independently, it would be useful to also consider the overall flow to understand why PART B follows PART A, and so on. Viewed sequentially, PART A tests a unique scenario while also setting up the environment needed for PART B (quorum mode). Similarly, PART C covers a different scenario and prepares the setup for the test case in PART D. This approach helps avoid unnecessary stop/start cycles of the synchronous standby. > 3) > Same with these 2: > +# PART B: ANY mode (quorum) — logical decoding proceeds with N-of-M slots > > +# PART D: ANY mode — verify it works with either standby > > Part B tests that it works when standby1 is down. > Part D tests that it works when standby2 is down and also works when > both are up. > > I think we don't need these duplicate test cases (unless I am missing > something?), we can merge Part D to Part B itself i.e. test with > standby1 down and up in a single test. > Removed PART D. It was almost similar to PART B. > 4) > These 2 tests are basically testing the same thing. Do we need both? > +# FIRST 1 should skip sb1_slot (unavailable) and use sb2_slot. > > +# Test that FIRST 1 is different from plain list — FIRST 1 succeeds > with one down. > Yes, they are actually the same. Hence removed the latter one. > > 5) > I see that we only need 'first_slot' for the last test and do not have > even a standby associated with it. So we can even move it's creation > to that test itself instead of creating it in the beginning and adding > comments to explain 'why'. > This is related to comment 1 above. PFA patch that addresses above comments. -- With Regards, Ashutosh Sharma. -
RE: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
(unknown) — 2026-03-19T06:38:04Z
On Wednesday, March 18, 2026 6:38 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > PFA patch that addresses above comments. Thanks for the patch! Overall, I think this is a valuable feature as I've heard requests from many customers for a way to avoid blocking logical replication when only some instances are down when using synchronized_standby_slots. I didn't find any bugs in the patch, but I have a few comments on the code and tests: 1. > /* > * Return true if value starts with explicit FIRST syntax: > * > * FIRST N (...) > * > * This is used to distinguish explicit FIRST from simple list syntax whose > * first slot name may start with "first". > */ > static bool > IsPrioritySyncStandbySlotsSyntax(const char *value) I think adding a new function to manually parse the list isn't the most elegant approach. Instead, it would be cleaner to have a new flag that distinguishes when a plain name list is specified, and use that to mark the case appropriately like: /* syncrep_method of SyncRepConfigData */ #define SYNC_REP_PRIORITY 0 #define SYNC_REP_QUORUM 1 +#define SYNC_REP_IMPLICIT 2 standby_config: - standby_list { $$ = create_syncrep_config("1", $1, SYNC_REP_PRIORITY); } + standby_list { $$ = create_syncrep_config("1", $1, SYNC_REP_IMPLICIT); } 2. + /* + * Allocate array to track slot states. Size it to the total number of + * configured slots since in the worst case all could have problem states. + */ + slot_states = (SyncStandbySlotsStateInfo *) + palloc(sizeof(SyncStandbySlotsStateInfo) * synchronized_standby_slots_config->nslotnames); I personally prefer building the list incrementally with List * and lappend rather than pre-allocating, since the list may often be empty in success cases. 3. +<synopsis> +[FIRST] <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="parameter">slot_name</replaceable> [, ...] ) +ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="parameter">slot_name</replaceable> [, ...] ) +<replaceable class="parameter">slot_name</replaceable> [, ...] +</synopsis> The documentation mentions that the FIRST keyword is optional, but the code and comments don't seem consistent. Some comments give the impression that FIRST is required: + * FIRST N (slot1, slot2, ...) -- wait for first N in priority order Additionally, IsPrioritySyncStandbySlotsSyntax() only checks for the FIRST keyword and doesn't handle the "N (slot1, slot2)" case where FIRST is omitted. 4. Regarding the new tests, I think we can avoid testing the plain slot list since that's already covered extensively in 040_standby_failover_slots_sync.pl. Instead, we could focus on testing edge cases for ANY and FIRST. I noticed there are no tests for scenarios where logical decoding blocks when using ANY or FIRST. For example, testing FIRST 1 (s1, s2) where s1 is alive but hasn't caught up would be valuable (if possible to test in tap-test, maybe test via recovery_min_apply_delay ?). This logic is more error-prone and required more careful review than other cases. BTW, Part E seems unnecessary since the patch reuses the sync_standby_names parser. ISTM, testing the first_ prefix in this context doesn't add valuable coverage. Best Regards, Hou zj -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-20T06:43:51Z
On Thu, Mar 19, 2026 at 12:08 PM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> wrote: > > On Wednesday, March 18, 2026 6:38 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > PFA patch that addresses above comments. > > Thanks for the patch! Overall, I think this is a valuable feature as I've heard > requests from many customers for a way to avoid blocking logical replication > when only some instances are down when using synchronized_standby_slots. > > I didn't find any bugs in the patch, but I have a few comments on the code and > tests: > > 1. > > > /* > > * Return true if value starts with explicit FIRST syntax: > > * > > * FIRST N (...) > > * > > * This is used to distinguish explicit FIRST from simple list syntax whose > > * first slot name may start with "first". > > */ > > static bool > > IsPrioritySyncStandbySlotsSyntax(const char *value) > > I think adding a new function to manually parse the list isn't the most elegant > approach. Instead, it would be cleaner to have a new flag that distinguishes > when a plain name list is specified, and use that to mark the case > appropriately like: > > /* syncrep_method of SyncRepConfigData */ > #define SYNC_REP_PRIORITY 0 > #define SYNC_REP_QUORUM 1 > +#define SYNC_REP_IMPLICIT 2 > > standby_config: > - standby_list { $$ = create_syncrep_config("1", $1, SYNC_REP_PRIORITY); } > + standby_list { $$ = create_syncrep_config("1", $1, SYNC_REP_IMPLICIT); } > I like the idea. The only thing we have to see is that the changes in synchronous_standby_names for this look clean (converting IMPLICIT to PRIORITY and overwriting num_sync to 1). Also, do you think SYNC_REP_ALL is more meaningful than SYNC_REP_IMPLICIT? thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-20T07:16:56Z
On Fri, Mar 20, 2026 at 12:14 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Mar 19, 2026 at 12:08 PM Hou, Zhijie/侯 志杰 > <houzj.fnst@fujitsu.com> wrote: > > > > On Wednesday, March 18, 2026 6:38 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > PFA patch that addresses above comments. > > > > Thanks for the patch! Overall, I think this is a valuable feature as I've heard > > requests from many customers for a way to avoid blocking logical replication > > when only some instances are down when using synchronized_standby_slots. > > > > I didn't find any bugs in the patch, but I have a few comments on the code and > > tests: > > > > 1. > > > > > /* > > > * Return true if value starts with explicit FIRST syntax: > > > * > > > * FIRST N (...) > > > * > > > * This is used to distinguish explicit FIRST from simple list syntax whose > > > * first slot name may start with "first". > > > */ > > > static bool > > > IsPrioritySyncStandbySlotsSyntax(const char *value) > > > > I think adding a new function to manually parse the list isn't the most elegant > > approach. Instead, it would be cleaner to have a new flag that distinguishes > > when a plain name list is specified, and use that to mark the case > > appropriately like: > > > > /* syncrep_method of SyncRepConfigData */ > > #define SYNC_REP_PRIORITY 0 > > #define SYNC_REP_QUORUM 1 > > +#define SYNC_REP_IMPLICIT 2 > > > > standby_config: > > - standby_list { $$ = create_syncrep_config("1", $1, SYNC_REP_PRIORITY); } > > + standby_list { $$ = create_syncrep_config("1", $1, SYNC_REP_IMPLICIT); } > > > > I like the idea. The only thing we have to see is that the changes in > synchronous_standby_names for this look clean (converting IMPLICIT to > PRIORITY and overwriting num_sync to 1). Also, do you think > SYNC_REP_ALL is more meaningful than SYNC_REP_IMPLICIT? > In my view, modifying the shared parser code (the syncrep parser in this case) may not be the best approach, especially when it can be avoided. The current design keeps changes localized to synchronized_standby_slots parsing within the slot handling logic, and preserves existing synchronous replication semantics. The localized approach is also comparatively safer (risk free) and easier to maintain. Additionally, the function that inspects the syncrep_parse output is fairly straightforward, it simply checks for the presence of the FIRST N syntax. -- With Regards, Ashutosh Sharma. -
RE: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
(unknown) — 2026-03-20T07:51:50Z
On Friday, March 20, 2026 3:17 PM Ashutosh Sharma <ashu.coek88@gmail.com> > > On Fri, Mar 20, 2026 at 12:14 PM shveta malik <shveta.malik@gmail.com> > wrote: > > > > On Thu, Mar 19, 2026 at 12:08 PM Hou, Zhijie > > <houzj.fnst@fujitsu.com> wrote: > > > I didn't find any bugs in the patch, but I have a few comments on the code > and > > > tests: > > > > > > 1. > > > > > > > /* > > > > * Return true if value starts with explicit FIRST syntax: > > > > * > > > > * FIRST N (...) > > > > * > > > > * This is used to distinguish explicit FIRST from simple list syntax whose > > > > * first slot name may start with "first". > > > > */ > > > > static bool > > > > IsPrioritySyncStandbySlotsSyntax(const char *value) > > > > > > I think adding a new function to manually parse the list isn't the most > elegant > > > approach. Instead, it would be cleaner to have a new flag that > distinguishes > > > when a plain name list is specified, and use that to mark the case > > > appropriately like: > > > > > > /* syncrep_method of SyncRepConfigData */ > > > #define SYNC_REP_PRIORITY 0 > > > #define SYNC_REP_QUORUM 1 > > > +#define SYNC_REP_IMPLICIT 2 > > > > > > standby_config: > > > - standby_list { $$ = create_syncrep_config("1", $1, > SYNC_REP_PRIORITY); } > > > + standby_list { $$ = create_syncrep_config("1", $1, > SYNC_REP_IMPLICIT); } > > > > > > > I like the idea. The only thing we have to see is that the changes in > > synchronous_standby_names for this look clean (converting IMPLICIT to > > PRIORITY and overwriting num_sync to 1). Also, do you think > > SYNC_REP_ALL is more meaningful than SYNC_REP_IMPLICIT? > > > > In my view, modifying the shared parser code (the syncrep parser in > this case) may not be the best approach, especially when it can be > avoided. The current design keeps changes localized to > synchronized_standby_slots parsing within the slot handling logic, and > preserves existing synchronous replication semantics. The localized > approach is also comparatively safer (risk free) and easier to > maintain. > > Additionally, the function that inspects the syncrep_parse output is > fairly straightforward, it simply checks for the presence of the FIRST > N syntax. Since we're reusing the same parser for two GUCs that have different interpretations of one syntax variant (the plain slot list), making the parser more general is a natural approach, especially given that the patch is adding new functionality here. My main concern is the IsPrioritySyncStandbySlotsSyntax() function. It introduces additional hard-coded parsing logic that duplicates what's already implemented in syncrep_gram.y. I'm also concerned about maintainability, particularly since we already discovered a bug in the hard-coded parser code [1] and the patch even added a tap-test (part E) to cover that path. All of this effort could be avoided by removing this function and leveraging functionality provided by the shared parser. That said, this is just my opinion. I'm okay with whichever approach the community ultimately agrees on. [1] https://www.postgresql.org/message-id/CAFPTHDbrJJtaR4Jf2HNOZQVyBLJF-kq8kk%3DFvmeZ1rfU%2BY3R5g%40mail.gmail.com Best Regards, Hou zj -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-20T08:38:18Z
Hi, On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> wrote: > > On Friday, March 20, 2026 3:17 PM Ashutosh Sharma <ashu.coek88@gmail.com> > > > > On Fri, Mar 20, 2026 at 12:14 PM shveta malik <shveta.malik@gmail.com> > > wrote: > > > > > > On Thu, Mar 19, 2026 at 12:08 PM Hou, Zhijie > > > <houzj.fnst@fujitsu.com> wrote: > > > > I didn't find any bugs in the patch, but I have a few comments on the code > > and > > > > tests: > > > > > > > > 1. > > > > > > > > > /* > > > > > * Return true if value starts with explicit FIRST syntax: > > > > > * > > > > > * FIRST N (...) > > > > > * > > > > > * This is used to distinguish explicit FIRST from simple list syntax whose > > > > > * first slot name may start with "first". > > > > > */ > > > > > static bool > > > > > IsPrioritySyncStandbySlotsSyntax(const char *value) > > > > > > > > I think adding a new function to manually parse the list isn't the most > > elegant > > > > approach. Instead, it would be cleaner to have a new flag that > > distinguishes > > > > when a plain name list is specified, and use that to mark the case > > > > appropriately like: > > > > > > > > /* syncrep_method of SyncRepConfigData */ > > > > #define SYNC_REP_PRIORITY 0 > > > > #define SYNC_REP_QUORUM 1 > > > > +#define SYNC_REP_IMPLICIT 2 > > > > > > > > standby_config: > > > > - standby_list { $$ = create_syncrep_config("1", $1, > > SYNC_REP_PRIORITY); } > > > > + standby_list { $$ = create_syncrep_config("1", $1, > > SYNC_REP_IMPLICIT); } > > > > > > > > > > I like the idea. The only thing we have to see is that the changes in > > > synchronous_standby_names for this look clean (converting IMPLICIT to > > > PRIORITY and overwriting num_sync to 1). Also, do you think > > > SYNC_REP_ALL is more meaningful than SYNC_REP_IMPLICIT? > > > > > > > In my view, modifying the shared parser code (the syncrep parser in > > this case) may not be the best approach, especially when it can be > > avoided. The current design keeps changes localized to > > synchronized_standby_slots parsing within the slot handling logic, and > > preserves existing synchronous replication semantics. The localized > > approach is also comparatively safer (risk free) and easier to > > maintain. > > > > Additionally, the function that inspects the syncrep_parse output is > > fairly straightforward, it simply checks for the presence of the FIRST > > N syntax. > > Since we're reusing the same parser for two GUCs that have different > interpretations of one syntax variant (the plain slot list), making the parser > more general is a natural approach, especially given that the patch is adding > new functionality here. > > My main concern is the IsPrioritySyncStandbySlotsSyntax() function. It > introduces additional hard-coded parsing logic that duplicates what's already > implemented in syncrep_gram.y. I'm also concerned about maintainability, > particularly since we already discovered a bug in the hard-coded parser code [1] > and the patch even added a tap-test (part E) to cover that path. All of this > effort could be avoided by removing this function and leveraging functionality > provided by the shared parser. > The issue that you are referring to here was without this function. The idea here is to reuse the existing synchronous_standby_names parser as-is, without changing its grammar or parse behavior. synchronized_standby_slots differs only in post-parse interpretation of simple-list syntax, so we add a local helper to disambiguate explicit priority mode from plain lists before applying synchronized_standby_slots semantics. > That said, this is just my opinion. I'm okay with whichever approach > the community ultimately agrees on. > Sure, thanks for sharing your thoughts. We can wait and see what others have to say on this. -- With Regards, Ashutosh Sharma. > [1] https://www.postgresql.org/message-id/CAFPTHDbrJJtaR4Jf2HNOZQVyBLJF-kq8kk%3DFvmeZ1rfU%2BY3R5g%40mail.gmail.com -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-20T11:08:12Z
On Fri, Mar 20, 2026 at 2:08 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> wrote: > > > > On Friday, March 20, 2026 3:17 PM Ashutosh Sharma <ashu.coek88@gmail.com> > > > > > > On Fri, Mar 20, 2026 at 12:14 PM shveta malik <shveta.malik@gmail.com> > > > wrote: > > > > > > > > On Thu, Mar 19, 2026 at 12:08 PM Hou, Zhijie > > > > <houzj.fnst@fujitsu.com> wrote: > > > > > I didn't find any bugs in the patch, but I have a few comments on the code > > > and > > > > > tests: > > > > > > > > > > 1. > > > > > > > > > > > /* > > > > > > * Return true if value starts with explicit FIRST syntax: > > > > > > * > > > > > > * FIRST N (...) > > > > > > * > > > > > > * This is used to distinguish explicit FIRST from simple list syntax whose > > > > > > * first slot name may start with "first". > > > > > > */ > > > > > > static bool > > > > > > IsPrioritySyncStandbySlotsSyntax(const char *value) > > > > > > > > > > I think adding a new function to manually parse the list isn't the most > > > elegant > > > > > approach. Instead, it would be cleaner to have a new flag that > > > distinguishes > > > > > when a plain name list is specified, and use that to mark the case > > > > > appropriately like: > > > > > > > > > > /* syncrep_method of SyncRepConfigData */ > > > > > #define SYNC_REP_PRIORITY 0 > > > > > #define SYNC_REP_QUORUM 1 > > > > > +#define SYNC_REP_IMPLICIT 2 > > > > > > > > > > standby_config: > > > > > - standby_list { $$ = create_syncrep_config("1", $1, > > > SYNC_REP_PRIORITY); } > > > > > + standby_list { $$ = create_syncrep_config("1", $1, > > > SYNC_REP_IMPLICIT); } > > > > > > > > > > > > > I like the idea. The only thing we have to see is that the changes in > > > > synchronous_standby_names for this look clean (converting IMPLICIT to > > > > PRIORITY and overwriting num_sync to 1). Also, do you think > > > > SYNC_REP_ALL is more meaningful than SYNC_REP_IMPLICIT? > > > > > > > > > > In my view, modifying the shared parser code (the syncrep parser in > > > this case) may not be the best approach, especially when it can be > > > avoided. The current design keeps changes localized to > > > synchronized_standby_slots parsing within the slot handling logic, and > > > preserves existing synchronous replication semantics. The localized > > > approach is also comparatively safer (risk free) and easier to > > > maintain. > > > > > > Additionally, the function that inspects the syncrep_parse output is > > > fairly straightforward, it simply checks for the presence of the FIRST > > > N syntax. > > > > Since we're reusing the same parser for two GUCs that have different > > interpretations of one syntax variant (the plain slot list), making the parser > > more general is a natural approach, especially given that the patch is adding > > new functionality here. > > > > My main concern is the IsPrioritySyncStandbySlotsSyntax() function. It > > introduces additional hard-coded parsing logic that duplicates what's already > > implemented in syncrep_gram.y. I'm also concerned about maintainability, > > particularly since we already discovered a bug in the hard-coded parser code [1] > > and the patch even added a tap-test (part E) to cover that path. All of this > > effort could be avoided by removing this function and leveraging functionality > > provided by the shared parser. > > > > The issue that you are referring to here was without this function. > > The idea here is to reuse the existing synchronous_standby_names > parser as-is, without changing its grammar or parse behavior. > synchronized_standby_slots differs only in post-parse interpretation > of simple-list syntax, so we add a local helper to disambiguate > explicit priority mode from plain lists before applying > synchronized_standby_slots semantics. > > > That said, this is just my opinion. I'm okay with whichever approach > > the community ultimately agrees on. > > > > Sure, thanks for sharing your thoughts. We can wait and see what > others have to say on this. > FWIW, I’m also slightly hesitant to touch the synchronous_standby_names code for this, and I had concern about whether the changes in that layer will be clean/acceptable. But let’s see what others have to say. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Dilip Kumar <dilipbalaut@gmail.com> — 2026-03-20T11:28:04Z
On Wed, Mar 18, 2026 at 4:08 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > Thanks again for reviewing the test cases. Please find my comments inline below: > > On Wed, Mar 18, 2026 at 12:01 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > Few comments: > > > > 1) > > + > > +# Physical replication slots for the two standbys > > +$primary->safe_psql('postgres', > > + "SELECT pg_create_physical_replication_slot('sb1_slot');"); > > +$primary->safe_psql('postgres', > > + "SELECT pg_create_physical_replication_slot('sb2_slot');"); > > +$primary->safe_psql('postgres', > > + "SELECT pg_create_physical_replication_slot('first_slot');"); > > > > Can we please write a comment atop 'first_slot' creation for its purpose. > > The comment '+# Physical replication slots for the two standbys' > > confuses as we are creating 3 slots following that comment. > > > > Moved this to PART E (the test-case where this slot is being used). > This is also suggested in one of your comments below. > > > 2) > > +# PART A: Plain list (ALL mode) blocks when any slot is unavailable > > > > +# PART C: Verify plain list (ALL mode) requires ALL slots > > > > I think we can merge the 2 tests. Is there a specific reason we have > > it in 2 different sections (am I missing something?). > > > > Part A can first test blocking on inactive standby (which it is doing > > already) and then restart standby and can check that logical decoding > > now proceeds. Part C can then be removed. > > > > I did not remove PART C entirely, however, I removed the portions that > overlapped with PART A. > > I would suggest not just evaluating each test case independently, it > would be useful to also consider the overall flow to understand why > PART B follows PART A, and so on. > > Viewed sequentially, PART A tests a unique scenario while also setting > up the environment needed for PART B (quorum mode). Similarly, PART C > covers a different scenario and prepares the setup for the test case > in PART D. This approach helps avoid unnecessary stop/start cycles of > the synchronous standby. > > > 3) > > Same with these 2: > > +# PART B: ANY mode (quorum) — logical decoding proceeds with N-of-M slots > > > > +# PART D: ANY mode — verify it works with either standby > > > > Part B tests that it works when standby1 is down. > > Part D tests that it works when standby2 is down and also works when > > both are up. > > > > I think we don't need these duplicate test cases (unless I am missing > > something?), we can merge Part D to Part B itself i.e. test with > > standby1 down and up in a single test. > > > > Removed PART D. It was almost similar to PART B. > > > 4) > > These 2 tests are basically testing the same thing. Do we need both? > > +# FIRST 1 should skip sb1_slot (unavailable) and use sb2_slot. > > > > +# Test that FIRST 1 is different from plain list — FIRST 1 succeeds > > with one down. > > > > Yes, they are actually the same. Hence removed the latter one. > > > > > 5) > > I see that we only need 'first_slot' for the last test and do not have > > even a standby associated with it. So we can even move it's creation > > to that test itself instead of creating it in the beginning and adding > > comments to explain 'why'. > > > > This is related to comment 1 above. > > PFA patch that addresses above comments. I was reviewing the latest patch and here are few comments/suggestions 1. + <para> + The keyword <literal>FIRST</literal>, coupled with + <replaceable class="parameter">num_sync</replaceable>, specifies + priority-based semantics. Logical decoding will wait for the first + <replaceable class="parameter">num_sync</replaceable> available + physical slots in priority order (the order they appear in the list). + If a slot is missing, logical, invalidated, or inactive, it will be + skipped. However, if a slot exists and is valid and active but has + not yet caught up, the system will wait for it rather than skipping + to lower-priority slots. + </para> This explains that it will skip missing, logical, invalidated, or inactive slots while processing the list and deciding on which first N slots to wait for, but I could not understand what would be the behavior if after skipping a few invalid slots the remaining valid slots are less than N, will it proceed for decoding? I think this should be clarified in docs. 2. I see a lot of overlapping code between check_synchronized_standby_slots() and check_synchronous_standby_names(), can we do some refactoring to make a common code? I have just pass through the patch and will try to complete the review soon. -- Regards, Dilip Kumar Google -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
surya poondla <suryapoondla4@gmail.com> — 2026-03-23T04:21:02Z
Hi All, Thank you for reporting a real gap and building this feature to address it. Very nice points were discussed in this thread. I reviewed the v20260318 patch and some comments. Documentation comments: 1. FIRST mode does not specify what happens when valid slots < N "If a slot is missing, logical, invalidated, or inactive, it will be skipped. However, if a slot exists and is valid and active but has not yet caught up, the system will wait for it rather than skipping to lower-priority slots." This paragraph explains the skip/wait distinction clearly, but doesn't clearly address what happens when, after skipping all missing/invalid/inactive/logical slots, the number of remaining valid slots is less than num_sync? For example, with FIRST 2 (sb1_slot, sb2_slot, sb3_slot): if sb1_slot and sb2_slot are both invalidated and only sb3_slot is valid but lagging FIRST 2 requires two slots, but only one candidate remains. Looking at the code in StandbySlotsHaveCaughtup(), when syncrep_method == SYNC_REP_PRIORITY and a slot is lagging, the code does: if (wait_for_all || synchronized_standby_slots_config->syncrep_method == SYNC_REP_PRIORITY) break; So the function breaks out of the loop and returns false. This is the correct behavior, but it is not stated anywhere in the documentation. A user encountering this scenario will not know whether to expect a wait or an error. The documentation should state explicitly that in FIRST mode, if fewer valid slots than num_sync are available, logical decoding waits indefinitely. 2. "Missing, logical, invalidated, or inactive slots are skipped when determining candidates, and lagging slots simply do not count toward the required number until they catch up" This is correct for the case where some slots are skipped and others have caught up. But it does not address the case where all listed slots are lagging and every slot is healthy and connected, but none have reached wait_for_lsn yet. In that situation, the code records each slot as SS_SLOT_LAGGING, does goto next_slot for each (because syncrep_method == SYNC_REP_QUORUM), and returns false because caught_up_slot_num < required. Logical decoding waits. You can append the following sentence to the above documentation paragraph "If fewer than num_sync slots have caught up at a given moment, logical decoding waits until that threshold is reached." Test comments: 1. "PART D: Verify FIRST N priority semantics. # 3. Wait for valid but lagging slots (not skip to lower priority)" The test implements this by calling $standby1->stop. A stopped standby has no active_pid, so the slot is classified as SS_SLOT_INACTIVE, not SS_SLOT_LAGGING. SS_SLOT_LAGGING means it is connected and streaming but restart_lsn < wait_for_lsn. As Hou previously mentioned, recovery_min_apply_delay on standby1 would be one way to keep it connected while forcing its WAL application to lag, exercising the SS_SLOT_LAGGING code path directly. It is worth adding a test that covers this, both for FIRST (to confirm it blocks) and for ANY (to confirm it does not). Overall a great patch. Regards, Surya Poondla -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-23T05:32:04Z
On Mon, Mar 23, 2026 at 9:51 AM surya poondla <suryapoondla4@gmail.com> wrote: > > Hi All, > > Thank you for reporting a real gap and building this feature to address it. Very nice points were discussed in this thread. > > I reviewed the v20260318 patch and some comments. > > Documentation comments: > 1. FIRST mode does not specify what happens when valid slots < N > "If a slot is missing, logical, invalidated, or inactive, it will be skipped. However, if a slot exists and is valid and active but has not yet caught up, the system will wait for it rather than skipping to lower-priority slots." > This paragraph explains the skip/wait distinction clearly, but doesn't clearly address what happens when, after skipping all missing/invalid/inactive/logical slots, the number of remaining valid slots is less than num_sync? > > For example, with FIRST 2 (sb1_slot, sb2_slot, sb3_slot): if sb1_slot and sb2_slot are both invalidated and only sb3_slot is valid but lagging FIRST 2 requires two slots, but only one candidate remains. > > Looking at the code in StandbySlotsHaveCaughtup(), when syncrep_method == SYNC_REP_PRIORITY and a slot is lagging, the code does: > if (wait_for_all || synchronized_standby_slots_config->syncrep_method == SYNC_REP_PRIORITY) > break; > > So the function breaks out of the loop and returns false. This is the correct behavior, but it is not stated anywhere in the documentation. A user encountering this scenario will not know whether to expect a wait or an error. The documentation should state explicitly that in FIRST mode, if fewer valid slots than num_sync are available, logical decoding waits indefinitely. > > 2. "Missing, logical, invalidated, or inactive slots are skipped when determining candidates, and lagging slots simply do not count toward the required number until they catch up" > This is correct for the case where some slots are skipped and others have caught up. But it does not address the case where all listed slots are lagging and every slot is healthy and connected, but none have reached wait_for_lsn yet. In that situation, the code records each slot as SS_SLOT_LAGGING, does goto next_slot for each (because syncrep_method == SYNC_REP_QUORUM), and returns false because caught_up_slot_num < required. Logical decoding waits. > > You can append the following sentence to the above documentation paragraph "If fewer than num_sync slots have caught up at a given moment, logical decoding waits until that threshold is reached." > > > Test comments: > 1. "PART D: Verify FIRST N priority semantics. # 3. Wait for valid but lagging slots (not skip to lower priority)" > The test implements this by calling $standby1->stop. A stopped standby has no active_pid, so the slot is classified as SS_SLOT_INACTIVE, not SS_SLOT_LAGGING. > SS_SLOT_LAGGING means it is connected and streaming but restart_lsn < wait_for_lsn. > > As Hou previously mentioned, recovery_min_apply_delay on standby1 would be one way to keep it connected while forcing its WAL application to lag, exercising the SS_SLOT_LAGGING code path directly. It is worth adding a test that covers this, both for FIRST (to confirm it blocks) and for ANY (to confirm it does not). > > Overall a great patch. > Thank you - Hou, Surya and Dilip for your feedback. I will address all your comments in the next patch. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-23T10:58:21Z
On Mon, Mar 23, 2026 at 9:51 AM surya poondla <suryapoondla4@gmail.com> wrote: > > Hi All, > > Thank you for reporting a real gap and building this feature to address it. Very nice points were discussed in this thread. > > I reviewed the v20260318 patch and some comments. > > Documentation comments: > 1. FIRST mode does not specify what happens when valid slots < N > "If a slot is missing, logical, invalidated, or inactive, it will be skipped. However, if a slot exists and is valid and active but has not yet caught up, the system will wait for it rather than skipping to lower-priority slots." > This paragraph explains the skip/wait distinction clearly, but doesn't clearly address what happens when, after skipping all missing/invalid/inactive/logical slots, the number of remaining valid slots is less than num_sync? > > For example, with FIRST 2 (sb1_slot, sb2_slot, sb3_slot): if sb1_slot and sb2_slot are both invalidated and only sb3_slot is valid but lagging FIRST 2 requires two slots, but only one candidate remains. > > Looking at the code in StandbySlotsHaveCaughtup(), when syncrep_method == SYNC_REP_PRIORITY and a slot is lagging, the code does: > if (wait_for_all || synchronized_standby_slots_config->syncrep_method == SYNC_REP_PRIORITY) > break; > > So the function breaks out of the loop and returns false. This is the correct behavior, but it is not stated anywhere in the documentation. A user encountering this scenario will not know whether to expect a wait or an error. The documentation should state explicitly that in FIRST mode, if fewer valid slots than num_sync are available, logical decoding waits indefinitely. > Agreed, have added a note about this in the documentation section for synchronized_standby_slots. > 2. "Missing, logical, invalidated, or inactive slots are skipped when determining candidates, and lagging slots simply do not count toward the required number until they catch up" > This is correct for the case where some slots are skipped and others have caught up. But it does not address the case where all listed slots are lagging and every slot is healthy and connected, but none have reached wait_for_lsn yet. In that situation, the code records each slot as SS_SLOT_LAGGING, does goto next_slot for each (because syncrep_method == SYNC_REP_QUORUM), and returns false because caught_up_slot_num < required. Logical decoding waits. > > You can append the following sentence to the above documentation paragraph "If fewer than num_sync slots have caught up at a given moment, logical decoding waits until that threshold is reached." > Agreed, have updated the documentation accordingly. > > Test comments: > 1. "PART D: Verify FIRST N priority semantics. # 3. Wait for valid but lagging slots (not skip to lower priority)" > The test implements this by calling $standby1->stop. A stopped standby has no active_pid, so the slot is classified as SS_SLOT_INACTIVE, not SS_SLOT_LAGGING. > SS_SLOT_LAGGING means it is connected and streaming but restart_lsn < wait_for_lsn. > > As Hou previously mentioned, recovery_min_apply_delay on standby1 would be one way to keep it connected while forcing its WAL application to lag, exercising the SS_SLOT_LAGGING code path directly. It is worth adding a test that covers this, both for FIRST (to confirm it blocks) and for ANY (to confirm it does not). > I've added a test case for this, though I should flag that it may not be fully deterministic. I've done my best to minimize flakiness, but I'm not sure if that's acceptable, happy to hear thoughts on whether such a test is worth keeping or if it should be reworked. > + /* > + * Allocate array to track slot states. Size it to the total number of > + * configured slots since in the worst case all could have problem states. > + */ > + slot_states = (SyncStandbySlotsStateInfo *) > + palloc(sizeof(SyncStandbySlotsStateInfo) * synchronized_standby_slots_config->nslotnames); > > I personally prefer building the list incrementally with List * and lappend > rather than pre-allocating, since the list may often be empty in success cases. Good thought, but I would not prefer to change it just for that reason. a) The current allocation is small and straightforward, and it is always freed on both paths whether success or failure. b) Allocating before taking the lock avoids doing memory allocation work while holding ReplicationSlotControlLock. c) Eager allocation keeps the loop single-pass and simple, lazy allocation adds branching complexity, and if done inside the loop it happens under lock. d) The optimization benefit is usually tiny unless nslotnames is large (which is very unlikely) > 3. > > +<synopsis> > +[FIRST] <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="parameter">slot_name</replaceable> [, ...] ) > +ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="parameter">slot_name</replaceable> [, ...] ) > +<replaceable class="parameter">slot_name</replaceable> [, ...] > +</synopsis> > > The documentation mentions that the FIRST keyword is optional, but the code > and comments don't seem consistent. Some comments give the impression that > FIRST is required: > > + * FIRST N (slot1, slot2, ...) -- wait for first N in priority order > Okay, I follow. Since we now support NUM (standby_list) as explicit priority syntax (equivalent to FIRST NUM (...)) for synchronized_standby_slots, this should be fine. The only divergence from synchronous_standby_names is the plain list form: for synchronized_standby_slots, a plain list means wait for all listed slots, and this is documented. > Additionally, IsPrioritySyncStandbySlotsSyntax() only checks for the FIRST > keyword and doesn't handle the "N (slot1, slot2)" case where FIRST is omitted. > Yeah, it doesn't, thanks for raising this concern. It has been taken care of in the attached patch. > 4. > > Regarding the new tests, I think we can avoid testing the plain slot list > since that's already covered extensively in 040_standby_failover_slots_sync.pl. > 040_standby_failover_slots_sync.pl mainly tests single-slot behavior, so it does not replace this multi-slot ALL-mode pair in 053. > Instead, we could focus on testing edge cases for ANY and FIRST. I noticed > there are no tests for scenarios where logical decoding blocks when using ANY or > FIRST. For example, testing FIRST 1 (s1, s2) where s1 is alive but hasn't caught > up would be valuable (if possible to test in tap-test, maybe test via > recovery_min_apply_delay ?). This logic is more error-prone and required more > careful review than other cases. > I have added this test-case but as mentioned in one of my earlier comments as well, it could be flaky, I am not quite sure if it is good to have any sort of test-case that is non-deterministic. > BTW, Part E seems unnecessary since the patch reuses the sync_standby_names > parser. ISTM, testing the first_ prefix in this context doesn't add valuable > coverage. Unlike sync_standby_names, where a plain list is treated as FIRST 1 mode, synchronized_standby_slots treats it as ALL mode. To handle this difference, the code checks whether FIRST ( was explicitly specified and, if so, adjusts the parser output to ensure it is treated as ALL mode instead. This test specifically validates that behavior. On Fri, Mar 20, 2026 at 4:58 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > I was reviewing the latest patch and here are few comments/suggestions > > 1. > + <para> > + The keyword <literal>FIRST</literal>, coupled with > + <replaceable class="parameter">num_sync</replaceable>, specifies > + priority-based semantics. Logical decoding will wait for the first > + <replaceable class="parameter">num_sync</replaceable> available > + physical slots in priority order (the order they appear in the list). > + If a slot is missing, logical, invalidated, or inactive, it will be > + skipped. However, if a slot exists and is valid and active but has > + not yet caught up, the system will wait for it rather than skipping > + to lower-priority slots. > + </para> > > This explains that it will skip missing, logical, invalidated, or > inactive slots while processing the list and deciding on which first N > slots to wait for, but I could not understand what would be the > behavior if after skipping a few invalid slots the remaining valid > slots are less than N, will it proceed for decoding? I think this > should be clarified in docs. > This has been taken care of in the attached patch. > 2. I see a lot of overlapping code between > check_synchronized_standby_slots() and > check_synchronous_standby_names(), can we do some refactoring to make > a common code? > I looked into both functions and found about ~20-25% of the code is shared. Given the relatively small overlap, I'm not sure it's worth refactoring, happy to do it if the consensus is that it improves maintainability. > I have just pass through the patch and will try to complete the review soon. Sure, thanks. PFA patch that addresses all the comments above. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Japin Li <japinli@hotmail.com> — 2026-03-24T04:12:19Z
Hi, Ashutosh On Mon, 23 Mar 2026 at 16:28, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > On Mon, Mar 23, 2026 at 9:51 AM surya poondla <suryapoondla4@gmail.com> wrote: >> >> Hi All, >> >> Thank you for reporting a real gap and building this feature to address it. Very nice points were discussed in this thread. >> >> I reviewed the v20260318 patch and some comments. >> >> Documentation comments: >> 1. FIRST mode does not specify what happens when valid slots < N >> "If a slot is missing, logical, invalidated, or inactive, it will be >> skipped. However, if a slot exists and is valid and active but has >> not yet caught up, the system will wait for it rather than skipping >> to lower-priority slots." >> This paragraph explains the skip/wait distinction clearly, but >> doesn't clearly address what happens when, after skipping all >> missing/invalid/inactive/logical slots, the number of remaining >> valid slots is less than num_sync? >> >> For example, with FIRST 2 (sb1_slot, sb2_slot, sb3_slot): if sb1_slot and sb2_slot are both invalidated and only sb3_slot is valid but lagging FIRST 2 requires two slots, but only one candidate remains. >> >> Looking at the code in StandbySlotsHaveCaughtup(), when syncrep_method == SYNC_REP_PRIORITY and a slot is lagging, the code does: >> if (wait_for_all || synchronized_standby_slots_config->syncrep_method == SYNC_REP_PRIORITY) >> break; >> >> So the function breaks out of the loop and returns false. This is >> the correct behavior, but it is not stated anywhere in the >> documentation. A user encountering this scenario will not know >> whether to expect a wait or an error. The documentation should state >> explicitly that in FIRST mode, if fewer valid slots than num_sync >> are available, logical decoding waits indefinitely. >> > > Agreed, have added a note about this in the documentation section for > synchronized_standby_slots. > >> 2. "Missing, logical, invalidated, or inactive slots are skipped when determining candidates, and lagging slots simply do not count toward the required number until they catch up" >> This is correct for the case where some slots are skipped and others >> have caught up. But it does not address the case where all listed >> slots are lagging and every slot is healthy and connected, but none >> have reached wait_for_lsn yet. In that situation, the code records >> each slot as SS_SLOT_LAGGING, does goto next_slot for each (because >> syncrep_method == SYNC_REP_QUORUM), and returns false because >> caught_up_slot_num < required. Logical decoding waits. >> >> You can append the following sentence to the above documentation paragraph "If fewer than num_sync slots have caught up at a given moment, logical decoding waits until that threshold is reached." >> > > Agreed, have updated the documentation accordingly. > >> >> Test comments: >> 1. "PART D: Verify FIRST N priority semantics. # 3. Wait for valid but lagging slots (not skip to lower priority)" >> The test implements this by calling $standby1->stop. A stopped standby has no active_pid, so the slot is classified as SS_SLOT_INACTIVE, not SS_SLOT_LAGGING. >> SS_SLOT_LAGGING means it is connected and streaming but restart_lsn < wait_for_lsn. >> >> As Hou previously mentioned, recovery_min_apply_delay on standby1 >> would be one way to keep it connected while forcing its WAL >> application to lag, exercising the SS_SLOT_LAGGING code path >> directly. It is worth adding a test that covers this, both for FIRST >> (to confirm it blocks) and for ANY (to confirm it does not). >> > > I've added a test case for this, though I should flag that it may not > be fully deterministic. I've done my best to minimize flakiness, but > I'm not sure if that's acceptable, happy to hear thoughts on whether > such a test is worth keeping or if it should be reworked. > >> + /* >> + * Allocate array to track slot states. Size it to the total number of >> + * configured slots since in the worst case all could have problem states. >> + */ >> + slot_states = (SyncStandbySlotsStateInfo *) >> + palloc(sizeof(SyncStandbySlotsStateInfo) * synchronized_standby_slots_config->nslotnames); >> >> I personally prefer building the list incrementally with List * and lappend >> rather than pre-allocating, since the list may often be empty in success cases. > > Good thought, but I would not prefer to change it just for that reason. > > a) The current allocation is small and straightforward, and it is > always freed on both paths whether success or failure. > b) Allocating before taking the lock avoids doing memory allocation > work while holding ReplicationSlotControlLock. > c) Eager allocation keeps the loop single-pass and simple, lazy > allocation adds branching complexity, and if done inside the loop it > happens under lock. > d) The optimization benefit is usually tiny unless nslotnames is large > (which is very unlikely) > >> 3. >> >> +<synopsis> >> +[FIRST] <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="parameter">slot_name</replaceable> [, ...] ) >> +ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="parameter">slot_name</replaceable> [, ...] ) >> +<replaceable class="parameter">slot_name</replaceable> [, ...] >> +</synopsis> >> >> The documentation mentions that the FIRST keyword is optional, but the code >> and comments don't seem consistent. Some comments give the impression that >> FIRST is required: >> >> + * FIRST N (slot1, slot2, ...) -- wait for first N in priority order >> > > Okay, I follow. Since we now support NUM (standby_list) as explicit > priority syntax (equivalent to FIRST NUM (...)) for > synchronized_standby_slots, this should be fine. The only divergence > from synchronous_standby_names is the plain list form: for > synchronized_standby_slots, a plain list means wait for all listed > slots, and this is documented. > >> Additionally, IsPrioritySyncStandbySlotsSyntax() only checks for the FIRST >> keyword and doesn't handle the "N (slot1, slot2)" case where FIRST is omitted. >> > > Yeah, it doesn't, thanks for raising this concern. It has been taken > care of in the attached patch. > >> 4. >> >> Regarding the new tests, I think we can avoid testing the plain slot list >> since that's already covered extensively in 040_standby_failover_slots_sync.pl. >> > > 040_standby_failover_slots_sync.pl mainly tests single-slot behavior, > so it does not replace this multi-slot ALL-mode pair in 053. > >> Instead, we could focus on testing edge cases for ANY and FIRST. I noticed >> there are no tests for scenarios where logical decoding blocks when using ANY or >> FIRST. For example, testing FIRST 1 (s1, s2) where s1 is alive but hasn't caught >> up would be valuable (if possible to test in tap-test, maybe test via >> recovery_min_apply_delay ?). This logic is more error-prone and required more >> careful review than other cases. >> > > I have added this test-case but as mentioned in one of my earlier > comments as well, it could be flaky, I am not quite sure if it is good > to have any sort of test-case that is non-deterministic. > >> BTW, Part E seems unnecessary since the patch reuses the sync_standby_names >> parser. ISTM, testing the first_ prefix in this context doesn't add valuable >> coverage. > > Unlike sync_standby_names, where a plain list is treated as FIRST 1 > mode, synchronized_standby_slots treats it as ALL mode. To handle this > difference, the code checks whether FIRST ( was explicitly specified > and, if so, adjusts the parser output to ensure it is treated as ALL > mode instead. This test specifically validates that behavior. > > > On Fri, Mar 20, 2026 at 4:58 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: >> >> >> I was reviewing the latest patch and here are few comments/suggestions >> >> 1. >> + <para> >> + The keyword <literal>FIRST</literal>, coupled with >> + <replaceable class="parameter">num_sync</replaceable>, specifies >> + priority-based semantics. Logical decoding will wait for the first >> + <replaceable class="parameter">num_sync</replaceable> available >> + physical slots in priority order (the order they appear in the list). >> + If a slot is missing, logical, invalidated, or inactive, it will be >> + skipped. However, if a slot exists and is valid and active but has >> + not yet caught up, the system will wait for it rather than skipping >> + to lower-priority slots. >> + </para> >> >> This explains that it will skip missing, logical, invalidated, or >> inactive slots while processing the list and deciding on which first N >> slots to wait for, but I could not understand what would be the >> behavior if after skipping a few invalid slots the remaining valid >> slots are less than N, will it proceed for decoding? I think this >> should be clarified in docs. >> > > This has been taken care of in the attached patch. > >> 2. I see a lot of overlapping code between >> check_synchronized_standby_slots() and >> check_synchronous_standby_names(), can we do some refactoring to make >> a common code? >> > > I looked into both functions and found about ~20-25% of the code is > shared. Given the relatively small overlap, I'm not sure it's worth > refactoring, happy to do it if the consensus is that it improves > maintainability. > >> I have just pass through the patch and will try to complete the review soon. > > Sure, thanks. > > PFA patch that addresses all the comments above. Thanks for updating the patch. A minor nitpick: 1. +typedef enum +{ + SS_SLOT_NOT_FOUND, /* slot does not exist */ + SS_SLOT_LOGICAL, /* slot is logical, not physical */ + SS_SLOT_INVALIDATED, /* slot has been invalidated */ + SS_SLOT_INACTIVE, /* slot is inactive (standby not connected) */ + SS_SLOT_LAGGING /* slot exists and is active but has not caught up */ +} SyncStandbySlotsState; IIRC, trailing commas are now used after the last enum. 2. + slot_states = (SyncStandbySlotsStateInfo *) + palloc(sizeof(SyncStandbySlotsStateInfo) * synchronized_standby_slots_config->nslotnames); With palloc_array() now available, it would be preferable. > > -- > With Regards, > Ashutosh Sharma. > > [2. text/x-diff; v20260323-0001-Add-FIRST-N-and-ANY-N-syntax-support-to-synchronized.patch]... -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-26T06:05:37Z
Hi Japin, > Thanks for updating the patch. A minor nitpick: > > 1. > +typedef enum > +{ > + SS_SLOT_NOT_FOUND, /* slot does not exist */ > + SS_SLOT_LOGICAL, /* slot is logical, not physical */ > + SS_SLOT_INVALIDATED, /* slot has been invalidated */ > + SS_SLOT_INACTIVE, /* slot is inactive (standby not connected) */ > + SS_SLOT_LAGGING /* slot exists and is active but has not caught up */ > +} SyncStandbySlotsState; > > IIRC, trailing commas are now used after the last enum. > Yes, I could see that in some of the lately added enums. Thanks for raising this point, it has been addressed in the attached patch. > 2. > + slot_states = (SyncStandbySlotsStateInfo *) > + palloc(sizeof(SyncStandbySlotsStateInfo) * synchronized_standby_slots_config->nslotnames); > > With palloc_array() now available, it would be preferable. > Makes sense. The attached patch addresses this too. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-03-26T06:33:15Z
On Thu, Mar 26, 2026 at 11:36 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Makes sense. The attached patch addresses this too. > > -- Thanks Ashutosh. I have not yet looked at today's patch, please find a few comments from previous one: 1) I noticed a change in behavior compared to the HEAD. Earlier, inactive slots were considered blocking only if they were lagging (restart_lsn < wait_for_lsn). Now, inactive slots are treated as blocking regardless of their restart_lsn. I think we should revert to the previous behavior. It’s possible for a slot to catch up and then become inactive; in such cases, it should still be treated as caught up rather than blocking. 2) + case SS_SLOT_LAGGING: .. + errdetail("The slot's restart_lsn %X/%X is behind the required %X/%X.", + LSN_FORMAT_ARGS(slot_states[i].restart_lsn), + LSN_FORMAT_ARGS(wait_for_lsn))); Here restart_lsn can even be invalid. See the caller: if (!XLogRecPtrIsValid(restart_lsn) || restart_lsn < wait_for_lsn) { slot_states[num_slot_states].state = SS_SLOT_LAGGING; slot_states[num_slot_states].restart_lsn = restart_lsn; } I think log-messages should be adjusted accordingly to handle invalid-restart-lsn. 3) + slots have caught up. Missing, logical, invalidated, or inactive + slots are skipped when determining candidates, and lagging slots + simply do not count toward the required number until they catch up, + so if fewer than <replaceable class="parameter">num_sync</replaceable> + slots have caught up at a given moment, logical decoding waits until + that threshold is reached. + i.e., there is no priority ordering. My preference wil be to start 'If fewer than num_sync slots have caught up at a given moment' as a new line to break this long sentence, ('so' can also be removed). But I will leave the decision to you. 4) + For example, a setting of <literal>ANY 1 (sb1_slot, sb2_slot)</literal> + will allow logical decoding to proceed as soon as either physical slot + has confirmed WAL receipt. This is useful in conjunction with + quorum-based synchronous replication + (<literal>synchronous_standby_names = 'ANY ...'</literal>), so that + logical decoding availability matches the commit durability guarantee. If we read this example in continuation of the previous explanation, the example feels incomplete and could benefit from clarifying what happens if none of the slots are available or caught up. how about: For example, a setting of ANY 1 (sb1_slot, sb2_slot) allows logical decoding to proceed as soon as either physical slot has confirmed WAL receipt. If none of the slots are available or have caught up, logical decoding will wait until at least one slot meets the required condition. 5) If we fix point 1, I think the doc should be reviewed to determine whether any sections mentioning that inactive slots are skipped need to be updated. ~~ <I have not reviewed test yet, will review.> thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-26T07:48:43Z
On Thu, Mar 26, 2026 at 12:03 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Mar 26, 2026 at 11:36 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Makes sense. The attached patch addresses this too. > > > > -- > > Thanks Ashutosh. I have not yet looked at today's patch, please find a > few comments from previous one: > > 1) > I noticed a change in behavior compared to the HEAD. > > Earlier, inactive slots were considered blocking only if they were > lagging (restart_lsn < wait_for_lsn). Now, inactive slots are treated > as blocking regardless of their restart_lsn. I think we should revert > to the previous behavior. It’s possible for a slot to catch up and > then become inactive; in such cases, it should still be treated as > caught up rather than blocking. > Oh, absolutely, you're spot on. I will get this (and other things related to this) fixed in the next patch. Thanks for pointing it out. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-03-26T11:52:52Z
Hi, Thanks for the review. Please find my comments inline below: On Thu, Mar 26, 2026 at 12:03 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Mar 26, 2026 at 11:36 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Makes sense. The attached patch addresses this too. > > > > -- > > Thanks Ashutosh. I have not yet looked at today's patch, please find a > few comments from previous one: > > 1) > I noticed a change in behavior compared to the HEAD. > > Earlier, inactive slots were considered blocking only if they were > lagging (restart_lsn < wait_for_lsn). Now, inactive slots are treated > as blocking regardless of their restart_lsn. I think we should revert > to the previous behavior. It’s possible for a slot to catch up and > then become inactive; in such cases, it should still be treated as > caught up rather than blocking. > Yes, we shouldn't be skipping the inactive slots that have already caught up. This has been completely addressed in the attached patch. > 2) > + case SS_SLOT_LAGGING: > .. > + errdetail("The slot's restart_lsn %X/%X is behind the required %X/%X.", > + LSN_FORMAT_ARGS(slot_states[i].restart_lsn), > + LSN_FORMAT_ARGS(wait_for_lsn))); > > Here restart_lsn can even be invalid. See the caller: > > if (!XLogRecPtrIsValid(restart_lsn) || restart_lsn < wait_for_lsn) > { > slot_states[num_slot_states].state = SS_SLOT_LAGGING; > slot_states[num_slot_states].restart_lsn = restart_lsn; > } > > I think log-messages should be adjusted accordingly to handle > invalid-restart-lsn. > I don't see any problem even if the restart_lsn is invalid. The log message uses LSN_FORMAT_ARGS which should be able to log lsn with value 0/0. However, in case of invalid restart_lsn the existing log message may look a little less informative, so I have adjusted it slightly which might even take care of your comment. > 3) > + slots have caught up. Missing, logical, invalidated, or inactive > + slots are skipped when determining candidates, and lagging slots > + simply do not count toward the required number until they catch up, > + so if fewer than <replaceable class="parameter">num_sync</replaceable> > + slots have caught up at a given moment, logical decoding waits until > + that threshold is reached. > + i.e., there is no priority ordering. > > My preference wil be to start 'If fewer than num_sync slots have > caught up at a given moment' as a new line to break this long > sentence, ('so' can also be removed). But I will leave the decision to > you. > Okay, as you said, I have broken the sentence into two parts. > 4) > + For example, a setting of <literal>ANY 1 (sb1_slot, sb2_slot)</literal> > + will allow logical decoding to proceed as soon as either physical slot > + has confirmed WAL receipt. This is useful in conjunction with > + quorum-based synchronous replication > + (<literal>synchronous_standby_names = 'ANY ...'</literal>), so that > + logical decoding availability matches the commit durability guarantee. > > If we read this example in continuation of the previous explanation, > the example feels incomplete and could benefit from clarifying what > happens if none of the slots are available or caught up. how about: > > For example, a setting of ANY 1 (sb1_slot, sb2_slot) allows logical > decoding to proceed as soon as either physical slot has confirmed WAL > receipt. If none of the slots are available or have caught up, logical > decoding will wait until at least one slot meets the required > condition. > Agreed, this does look somewhat incomplete, so changed as per your suggestions. > 5) > If we fix point 1, I think the doc should be reviewed to determine > whether any sections mentioning that inactive slots are skipped need > to be updated. > Yeah, updated all such references in the doc file. PFA patch addressing all the comments above and let me know for any further comments. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
surya poondla <suryapoondla4@gmail.com> — 2026-03-31T18:16:05Z
Hi Ashutosh, Thanks for addressing my comments in the latest patch. The documentation additions look correct, and the new Part E test cases in 053_synchronized_standby_slots_quorum.pl exercise the SS_SLOT_LAGGING path as requested. I noticed that Shveta caught a behavioral regression between v20260318 and v20260323 regarding inactive slots that had already caught up being incorrectly blocked. The fix in v20260326 (splitting into SS_SLOT_INACTIVE_LAGGING and SS_SLOT_ACTIVE_LAGGING, and counting a slot as caught up when its restart_lsn >= wait_for_lsn regardless of activity) looks correct to me. Overall I feel the patch is in good shape. Regards, Surya Poondla
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-01T06:36:32Z
On Thu, Mar 26, 2026 at 5:23 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > PFA patch addressing all the comments above and let me know for any > further comments. > Thank You Ashutosh. Doc looks good to me. Few comments: 1) slot_states[num_slot_states].slot_name = name; slot_states[num_slot_states].state = inactive ? SS_SLOT_INACTIVE_LAGGING : SS_SLOT_ACTIVE_LAGGING; if (!inactive) slot_states[num_slot_states].restart_lsn = restart_lsn; num_slot_states++; Just to simplify the above part, I think we can set restart_lsn irrespective of 'inactive', no harm in that. Feel free to ignore this comment if you like the current way. slot_states[num_slot_states].slot_name = name; slot_states[num_slot_states].restart_lsn = restart_lsn; slot_states[num_slot_states].state = inactive ? SS_SLOT_INACTIVE_LAGGING : SS_SLOT_ACTIVE_LAGGING; num_slot_states++; 2) Test case mentioned scenarios A,B,C in test header, we can either mention all there or none. 3) What is the execution time for this new test? I ran it on my VM (which is slightly on the slower side), and the runtime varies between ~60 seconds and ~140 seconds. I executed it around 10–15 times. Most runs completed in about 65 seconds (which is still more), but a few were significantly longer (100+ seconds). During the longer runs, I noticed the following entry in pub.log (possibly related to Test Scenario E taking more time?). Could you please try running this on your end as well? 2026-03-31 19:45:45.557 IST client backend[145705] 053_synchronized_standby_slots_quorum.pl LOG: statement: SELECT active_pid IS NOT NULL AND restart_lsn IS NOT NULL AND restart_lsn < '0/03000450'::pg_lsn FROM pg_replication_slots WHERE slot_name = 'sb1_slot'; Just for reference, the complete failover test (t/040_standby_failover_slots_sync.pl) takes somewhere between 7 to 10sec on my VM. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-02T10:24:26Z
Hi Shveta, On Wed, Apr 1, 2026 at 12:06 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Mar 26, 2026 at 5:23 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > PFA patch addressing all the comments above and let me know for any > > further comments. > > > > Thank You Ashutosh. Doc looks good to me. Few comments: > > 3) > What is the execution time for this new test? > I ran it on my VM (which is slightly on the slower side), and the > runtime varies between ~60 seconds and ~140 seconds. I executed it > around 10–15 times. Most runs completed in about 65 seconds (which is > still more), but a few were significantly longer (100+ seconds). > During the longer runs, I noticed the following entry in pub.log > (possibly related to Test Scenario E taking more time?). Could you > please try running this on your end as well? > > 2026-03-31 19:45:45.557 IST client backend[145705] > 053_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > active_pid IS NOT NULL > AND restart_lsn IS NOT NULL > AND restart_lsn < '0/03000450'::pg_lsn > FROM pg_replication_slots > WHERE slot_name = 'sb1_slot'; > > Just for reference, the complete failover test > (t/040_standby_failover_slots_sync.pl) takes somewhere between 7 to > 10sec on my VM. > My concern with this new test is that it's both slow to run and prone to flakiness, which makes me question whether it's worth keeping. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-03T04:16:49Z
On Thu, Apr 2, 2026 at 3:55 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi Shveta, > > On Wed, Apr 1, 2026 at 12:06 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Thu, Mar 26, 2026 at 5:23 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > PFA patch addressing all the comments above and let me know for any > > > further comments. > > > > > > > Thank You Ashutosh. Doc looks good to me. Few comments: > > > > 3) > > What is the execution time for this new test? > > I ran it on my VM (which is slightly on the slower side), and the > > runtime varies between ~60 seconds and ~140 seconds. I executed it > > around 10–15 times. Most runs completed in about 65 seconds (which is > > still more), but a few were significantly longer (100+ seconds). > > During the longer runs, I noticed the following entry in pub.log > > (possibly related to Test Scenario E taking more time?). Could you > > please try running this on your end as well? > > > > 2026-03-31 19:45:45.557 IST client backend[145705] > > 053_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > > active_pid IS NOT NULL > > AND restart_lsn IS NOT NULL > > AND restart_lsn < '0/03000450'::pg_lsn > > FROM pg_replication_slots > > WHERE slot_name = 'sb1_slot'; > > > > Just for reference, the complete failover test > > (t/040_standby_failover_slots_sync.pl) takes somewhere between 7 to > > 10sec on my VM. > > > > My concern with this new test is that it's both slow to run and prone > to flakiness, which makes me question whether it's worth keeping. > will review and share my thoughts. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-03T08:51:00Z
On Fri, Apr 3, 2026 at 9:46 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Apr 2, 2026 at 3:55 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi Shveta, > > > > On Wed, Apr 1, 2026 at 12:06 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > On Thu, Mar 26, 2026 at 5:23 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > > > > PFA patch addressing all the comments above and let me know for any > > > > further comments. > > > > > > > > > > Thank You Ashutosh. Doc looks good to me. Few comments: > > > > > > 3) > > > What is the execution time for this new test? > > > I ran it on my VM (which is slightly on the slower side), and the > > > runtime varies between ~60 seconds and ~140 seconds. I executed it > > > around 10–15 times. Most runs completed in about 65 seconds (which is > > > still more), but a few were significantly longer (100+ seconds). > > > During the longer runs, I noticed the following entry in pub.log > > > (possibly related to Test Scenario E taking more time?). Could you > > > please try running this on your end as well? > > > > > > 2026-03-31 19:45:45.557 IST client backend[145705] > > > 053_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > > > active_pid IS NOT NULL > > > AND restart_lsn IS NOT NULL > > > AND restart_lsn < '0/03000450'::pg_lsn > > > FROM pg_replication_slots > > > WHERE slot_name = 'sb1_slot'; > > > > > > Just for reference, the complete failover test > > > (t/040_standby_failover_slots_sync.pl) takes somewhere between 7 to > > > 10sec on my VM. > > > > > > > My concern with this new test is that it's both slow to run and prone > > to flakiness, which makes me question whether it's worth keeping. > > > > will review and share my thoughts. > I gave it more thought, another idea for a shorter and quicker testcase could be to check wait_event for that particular application_name in pg_stat_activity. A lagging standby will result in wait_event=WaitForStandbyConfirmation with backend_type=walsender. I have attached sample-code to do the same in the attached txt file, please have a look. I discussed with Hou-San offline, he is okay with this idea. Please see if it works and change it as needed. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-06T13:07:23Z
On Fri, Apr 3, 2026 at 2:21 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Fri, Apr 3, 2026 at 9:46 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Thu, Apr 2, 2026 at 3:55 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > Hi Shveta, > > > > > > On Wed, Apr 1, 2026 at 12:06 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > On Thu, Mar 26, 2026 at 5:23 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > > > > > > > PFA patch addressing all the comments above and let me know for any > > > > > further comments. > > > > > > > > > > > > > Thank You Ashutosh. Doc looks good to me. Few comments: > > > > > > > > 3) > > > > What is the execution time for this new test? > > > > I ran it on my VM (which is slightly on the slower side), and the > > > > runtime varies between ~60 seconds and ~140 seconds. I executed it > > > > around 10–15 times. Most runs completed in about 65 seconds (which is > > > > still more), but a few were significantly longer (100+ seconds). > > > > During the longer runs, I noticed the following entry in pub.log > > > > (possibly related to Test Scenario E taking more time?). Could you > > > > please try running this on your end as well? > > > > > > > > 2026-03-31 19:45:45.557 IST client backend[145705] > > > > 053_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > > > > active_pid IS NOT NULL > > > > AND restart_lsn IS NOT NULL > > > > AND restart_lsn < '0/03000450'::pg_lsn > > > > FROM pg_replication_slots > > > > WHERE slot_name = 'sb1_slot'; > > > > > > > > Just for reference, the complete failover test > > > > (t/040_standby_failover_slots_sync.pl) takes somewhere between 7 to > > > > 10sec on my VM. > > > > > > > > > > My concern with this new test is that it's both slow to run and prone > > > to flakiness, which makes me question whether it's worth keeping. > > > > > > > will review and share my thoughts. > > > > I gave it more thought, another idea for a shorter and quicker > testcase could be to check wait_event for that particular > application_name in pg_stat_activity. A lagging standby will result in > wait_event=WaitForStandbyConfirmation with backend_type=walsender. > > I have attached sample-code to do the same in the attached txt file, > please have a look. I discussed with Hou-San offline, he is okay with > this idea. Please see if it works and change it as needed. > More than the execution time, I'm concerned if the test-case effectively validates what we want. With below setup, here is what I observe: Setup: Primary : psql -p 5555 (synchronous_standby_names = 'ANY 1 (standby1, standby2)'; synchronized_standby_slots = 'FIRST 1 (sb1_slot, sb2_slot)') Standby1 : psql -p 5556 (wal_receiver_status_interval=0) Standby2 : psql -p 5557 (wal_receiver_status_interval=10s) -- Observations: [local]:5555 ashu@postgres=# SELECT pg_logical_emit_message(true, 'qtest', 'first_1_lagging_blocks_1'); pg_logical_emit_message ------------------------- 0/04000220 (1 row) Time: 14.378 ms [local]:5555 ashu@postgres=# select slot_name, active_pid, restart_lsn from pg_replication_slots where slot_type = 'physical'; slot_name | active_pid | restart_lsn -----------+------------+------------- sb1_slot | 105328 | 0/04000250 sb2_slot | 105381 | 0/04000250 (2 rows) Time: 1.370 ms -- [local]:5555 ashu@postgres=# SELECT pg_logical_emit_message(true, 'qtest', 'first_1_lagging_blocks_2'); pg_logical_emit_message ------------------------- 0/040002A0 (1 row) Time: 13.533 ms [local]:5555 ashu@postgres=# select slot_name, active_pid, restart_lsn from pg_replication_slots where slot_type = 'physical'; slot_name | active_pid | restart_lsn -----------+------------+------------- sb1_slot | 105328 | 0/040002D0 sb2_slot | 105381 | 0/040002D0 (2 rows) -- Takeaways: 1) In both the cases, even though wal_receiver_status_interval = 0 on standby1, the restart_lsn of the standby1 quickly moved past the lsn of the logical message emitted which kind of gives sense that wal_receiver_status_interval = 0 disables periodic status packets, but receiver/walsender still exchange feedback on other events, so slot restart_lsn can move quickly. 2) On a fast local setup, both sb1_slot and sb2_slot can advance past the emitted LSN before we query pg_replication_slots making the test-case flaky/nondeterministic, it becomes time sensitive. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-07T03:34:26Z
On Mon, Apr 6, 2026 at 6:37 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Fri, Apr 3, 2026 at 2:21 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Fri, Apr 3, 2026 at 9:46 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > On Thu, Apr 2, 2026 at 3:55 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > Hi Shveta, > > > > > > > > On Wed, Apr 1, 2026 at 12:06 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > > > On Thu, Mar 26, 2026 at 5:23 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > > > > > > > > > > PFA patch addressing all the comments above and let me know for any > > > > > > further comments. > > > > > > > > > > > > > > > > Thank You Ashutosh. Doc looks good to me. Few comments: > > > > > > > > > > 3) > > > > > What is the execution time for this new test? > > > > > I ran it on my VM (which is slightly on the slower side), and the > > > > > runtime varies between ~60 seconds and ~140 seconds. I executed it > > > > > around 10–15 times. Most runs completed in about 65 seconds (which is > > > > > still more), but a few were significantly longer (100+ seconds). > > > > > During the longer runs, I noticed the following entry in pub.log > > > > > (possibly related to Test Scenario E taking more time?). Could you > > > > > please try running this on your end as well? > > > > > > > > > > 2026-03-31 19:45:45.557 IST client backend[145705] > > > > > 053_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > > > > > active_pid IS NOT NULL > > > > > AND restart_lsn IS NOT NULL > > > > > AND restart_lsn < '0/03000450'::pg_lsn > > > > > FROM pg_replication_slots > > > > > WHERE slot_name = 'sb1_slot'; > > > > > > > > > > Just for reference, the complete failover test > > > > > (t/040_standby_failover_slots_sync.pl) takes somewhere between 7 to > > > > > 10sec on my VM. > > > > > > > > > > > > > My concern with this new test is that it's both slow to run and prone > > > > to flakiness, which makes me question whether it's worth keeping. > > > > > > > > > > will review and share my thoughts. > > > > > > > I gave it more thought, another idea for a shorter and quicker > > testcase could be to check wait_event for that particular > > application_name in pg_stat_activity. A lagging standby will result in > > wait_event=WaitForStandbyConfirmation with backend_type=walsender. > > > > I have attached sample-code to do the same in the attached txt file, > > please have a look. I discussed with Hou-San offline, he is okay with > > this idea. Please see if it works and change it as needed. > > > > More than the execution time, I'm concerned if the test-case > effectively validates what we want. > I see your point. I agree that using wal_receiver_status_interval for this test may not be a reliable way. Can we attempt using pg_wal_replay_pause() on standby and then checking wait_event=WaitForStandbyConfirmation with backend_type=walsender on primary? Or do you see any issues in this approach that I might be overlooking? > With below setup, here is what I observe: > > Setup: > > Primary : psql -p 5555 (synchronous_standby_names = 'ANY 1 > (standby1, standby2)'; synchronized_standby_slots = 'FIRST 1 > (sb1_slot, sb2_slot)') > Standby1 : psql -p 5556 (wal_receiver_status_interval=0) > Standby2 : psql -p 5557 (wal_receiver_status_interval=10s) > > -- > > Observations: > > [local]:5555 ashu@postgres=# SELECT pg_logical_emit_message(true, > 'qtest', 'first_1_lagging_blocks_1'); > pg_logical_emit_message > ------------------------- > 0/04000220 > (1 row) > > Time: 14.378 ms > > [local]:5555 ashu@postgres=# select slot_name, active_pid, restart_lsn > from pg_replication_slots where slot_type = 'physical'; > slot_name | active_pid | restart_lsn > -----------+------------+------------- > sb1_slot | 105328 | 0/04000250 > sb2_slot | 105381 | 0/04000250 > (2 rows) > > Time: 1.370 ms > > -- > > [local]:5555 ashu@postgres=# SELECT pg_logical_emit_message(true, > 'qtest', 'first_1_lagging_blocks_2'); > pg_logical_emit_message > ------------------------- > 0/040002A0 > (1 row) > > Time: 13.533 ms > > [local]:5555 ashu@postgres=# select slot_name, active_pid, restart_lsn > from pg_replication_slots where slot_type = 'physical'; > slot_name | active_pid | restart_lsn > -----------+------------+------------- > sb1_slot | 105328 | 0/040002D0 > sb2_slot | 105381 | 0/040002D0 > (2 rows) > > -- > > Takeaways: > > 1) In both the cases, even though wal_receiver_status_interval = 0 on > standby1, the restart_lsn of the standby1 quickly moved past the lsn > of the logical message emitted which kind of gives sense that > wal_receiver_status_interval = 0 disables periodic status packets, but > receiver/walsender still exchange feedback on other events, so slot > restart_lsn can move quickly. > 2) On a fast local setup, both sb1_slot and sb2_slot can advance past > the emitted LSN before we query pg_replication_slots making the > test-case flaky/nondeterministic, it becomes time sensitive. > > -- > With Regards, > Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-07T03:41:10Z
On Tue, Apr 7, 2026 at 9:04 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Mon, Apr 6, 2026 at 6:37 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > On Fri, Apr 3, 2026 at 2:21 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > On Fri, Apr 3, 2026 at 9:46 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > On Thu, Apr 2, 2026 at 3:55 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > > Hi Shveta, > > > > > > > > > > On Wed, Apr 1, 2026 at 12:06 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > > > > > On Thu, Mar 26, 2026 at 5:23 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > PFA patch addressing all the comments above and let me know for any > > > > > > > further comments. > > > > > > > > > > > > > > > > > > > Thank You Ashutosh. Doc looks good to me. Few comments: > > > > > > > > > > > > 3) > > > > > > What is the execution time for this new test? > > > > > > I ran it on my VM (which is slightly on the slower side), and the > > > > > > runtime varies between ~60 seconds and ~140 seconds. I executed it > > > > > > around 10–15 times. Most runs completed in about 65 seconds (which is > > > > > > still more), but a few were significantly longer (100+ seconds). > > > > > > During the longer runs, I noticed the following entry in pub.log > > > > > > (possibly related to Test Scenario E taking more time?). Could you > > > > > > please try running this on your end as well? > > > > > > > > > > > > 2026-03-31 19:45:45.557 IST client backend[145705] > > > > > > 053_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > > > > > > active_pid IS NOT NULL > > > > > > AND restart_lsn IS NOT NULL > > > > > > AND restart_lsn < '0/03000450'::pg_lsn > > > > > > FROM pg_replication_slots > > > > > > WHERE slot_name = 'sb1_slot'; > > > > > > > > > > > > Just for reference, the complete failover test > > > > > > (t/040_standby_failover_slots_sync.pl) takes somewhere between 7 to > > > > > > 10sec on my VM. > > > > > > > > > > > > > > > > My concern with this new test is that it's both slow to run and prone > > > > > to flakiness, which makes me question whether it's worth keeping. > > > > > > > > > > > > > will review and share my thoughts. > > > > > > > > > > I gave it more thought, another idea for a shorter and quicker > > > testcase could be to check wait_event for that particular > > > application_name in pg_stat_activity. A lagging standby will result in > > > wait_event=WaitForStandbyConfirmation with backend_type=walsender. > > > > > > I have attached sample-code to do the same in the attached txt file, > > > please have a look. I discussed with Hou-San offline, he is okay with > > > this idea. Please see if it works and change it as needed. > > > > > > > More than the execution time, I'm concerned if the test-case > > effectively validates what we want. > > > > I see your point. I agree that using wal_receiver_status_interval for > this test may not be a reliable way. Can we attempt using > pg_wal_replay_pause() on standby and then checking > wait_event=WaitForStandbyConfirmation with backend_type=walsender on > primary? Or do you see any issues in this approach that I might be > overlooking? > Noticed that 049_wait_for_lsn.pl is doing something similar using pg_wal_replay_pause(), worth having a look. > > With below setup, here is what I observe: > > > > Setup: > > > > Primary : psql -p 5555 (synchronous_standby_names = 'ANY 1 > > (standby1, standby2)'; synchronized_standby_slots = 'FIRST 1 > > (sb1_slot, sb2_slot)') > > Standby1 : psql -p 5556 (wal_receiver_status_interval=0) > > Standby2 : psql -p 5557 (wal_receiver_status_interval=10s) > > > > -- > > > > Observations: > > > > [local]:5555 ashu@postgres=# SELECT pg_logical_emit_message(true, > > 'qtest', 'first_1_lagging_blocks_1'); > > pg_logical_emit_message > > ------------------------- > > 0/04000220 > > (1 row) > > > > Time: 14.378 ms > > > > [local]:5555 ashu@postgres=# select slot_name, active_pid, restart_lsn > > from pg_replication_slots where slot_type = 'physical'; > > slot_name | active_pid | restart_lsn > > -----------+------------+------------- > > sb1_slot | 105328 | 0/04000250 > > sb2_slot | 105381 | 0/04000250 > > (2 rows) > > > > Time: 1.370 ms > > > > -- > > > > [local]:5555 ashu@postgres=# SELECT pg_logical_emit_message(true, > > 'qtest', 'first_1_lagging_blocks_2'); > > pg_logical_emit_message > > ------------------------- > > 0/040002A0 > > (1 row) > > > > Time: 13.533 ms > > > > [local]:5555 ashu@postgres=# select slot_name, active_pid, restart_lsn > > from pg_replication_slots where slot_type = 'physical'; > > slot_name | active_pid | restart_lsn > > -----------+------------+------------- > > sb1_slot | 105328 | 0/040002D0 > > sb2_slot | 105381 | 0/040002D0 > > (2 rows) > > > > -- > > > > Takeaways: > > > > 1) In both the cases, even though wal_receiver_status_interval = 0 on > > standby1, the restart_lsn of the standby1 quickly moved past the lsn > > of the logical message emitted which kind of gives sense that > > wal_receiver_status_interval = 0 disables periodic status packets, but > > receiver/walsender still exchange feedback on other events, so slot > > restart_lsn can move quickly. > > 2) On a fast local setup, both sb1_slot and sb2_slot can advance past > > the emitted LSN before we query pg_replication_slots making the > > test-case flaky/nondeterministic, it becomes time sensitive. > > > > -- > > With Regards, > > Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-07T05:50:41Z
Hi, On Tue, Apr 7, 2026 at 9:04 AM shveta malik <shveta.malik@gmail.com> wrote: > > > I see your point. I agree that using wal_receiver_status_interval for > this test may not be a reliable way. Can we attempt using > pg_wal_replay_pause() on standby and then checking > wait_event=WaitForStandbyConfirmation with backend_type=walsender on > primary? Or do you see any issues in this approach that I might be > overlooking? > Yes, I think we can make use of the WAL replay pause/resume mechanism. This seems like the right approach, as it gives us a more controlled and deterministic way to validate the lagging behavior. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-07T10:26:23Z
Hi, On Tue, Apr 7, 2026 at 11:20 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > On Tue, Apr 7, 2026 at 9:04 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > I see your point. I agree that using wal_receiver_status_interval for > > this test may not be a reliable way. Can we attempt using > > pg_wal_replay_pause() on standby and then checking > > wait_event=WaitForStandbyConfirmation with backend_type=walsender on > > primary? Or do you see any issues in this approach that I might be > > overlooking? > > > > Yes, I think we can make use of the WAL replay pause/resume mechanism. > This seems like the right approach, as it gives us a more controlled > and deterministic way to validate the lagging behavior. > Looking at 049_wait_for_lsn.pl (the test case you referenced), it explicitly stops the WAL receiver by setting primary_conninfo to an empty string, rather than just pausing WAL replay. Using pg_wal_replay_pause() alone only halts replay; the WAL receiver continues running, keeps receiving WAL, and sends feedback/status to the primary. That feedback is sufficient to advance restart_lsn on the standby’s slot, which would violate the restart_lsn < wait_for_lsn condition inside StandbySlotsHaveCaughtup(), which is not what we want. This leads to the question: can we construct a realistic test case where a failover standby remains active (WAL receiver running) while its restart_lsn is still genuinely lagging and consistently so? This likely needs further exploration. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-07T11:48:13Z
On Tue, Apr 7, 2026 at 3:56 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > On Tue, Apr 7, 2026 at 11:20 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi, > > > > On Tue, Apr 7, 2026 at 9:04 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > > I see your point. I agree that using wal_receiver_status_interval for > > > this test may not be a reliable way. Can we attempt using > > > pg_wal_replay_pause() on standby and then checking > > > wait_event=WaitForStandbyConfirmation with backend_type=walsender on > > > primary? Or do you see any issues in this approach that I might be > > > overlooking? > > > > > > > Yes, I think we can make use of the WAL replay pause/resume mechanism. > > This seems like the right approach, as it gives us a more controlled > > and deterministic way to validate the lagging behavior. > > > > Looking at 049_wait_for_lsn.pl (the test case you referenced), it > explicitly stops the WAL receiver by setting primary_conninfo to an > empty string, rather than just pausing WAL replay. Oh, I missed it in that testcase. Setting primary_conninfo to NULL essentially means not starting the walreceiver and thus making the standby slot as inactive, for which we already have a testcase. > Using > pg_wal_replay_pause() alone only halts replay; the WAL receiver > continues running, keeps receiving WAL, and sends feedback/status to > the primary. That feedback is sufficient to advance restart_lsn on the > standby’s slot, which would violate the restart_lsn < wait_for_lsn > condition inside StandbySlotsHaveCaughtup(), which is not what we > want. Yes, I see. IIUC, the same problem will be there if we use recovery_min_apply_delay i.e., WALs will be received, flushed and feedback will be sent to primary, only replay will be delayed. We can use 'synchronous_commit = remote_apply' along with 'recovery_min_apply_delay ' but that would mean delaying logical replication because transaction commit is blocking not because standby is actually lagging. It will not be a suitable test for 'synchronized_satndby_slots'. > > This leads to the question: can we construct a realistic test case > where a failover standby remains active (WAL receiver running) while > its restart_lsn is still genuinely lagging and consistently so? This > likely needs further exploration. > I have no more ideas here. We can get rid of lagging testcase. But let's wait for a day to see if Hou-San has any further ideas on how to write a deterministic testcase here. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-07T13:53:30Z
On Tue, Apr 7, 2026 at 5:18 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Tue, Apr 7, 2026 at 3:56 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi, > > > > On Tue, Apr 7, 2026 at 11:20 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > Hi, > > > > > > On Tue, Apr 7, 2026 at 9:04 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > > > > > I see your point. I agree that using wal_receiver_status_interval for > > > > this test may not be a reliable way. Can we attempt using > > > > pg_wal_replay_pause() on standby and then checking > > > > wait_event=WaitForStandbyConfirmation with backend_type=walsender on > > > > primary? Or do you see any issues in this approach that I might be > > > > overlooking? > > > > > > > > > > Yes, I think we can make use of the WAL replay pause/resume mechanism. > > > This seems like the right approach, as it gives us a more controlled > > > and deterministic way to validate the lagging behavior. > > > > > > > Looking at 049_wait_for_lsn.pl (the test case you referenced), it > > explicitly stops the WAL receiver by setting primary_conninfo to an > > empty string, rather than just pausing WAL replay. > > Oh, I missed it in that testcase. Setting primary_conninfo to NULL > essentially means not starting the walreceiver and thus making the > standby slot as inactive, for which we already have a testcase. > > > Using > > pg_wal_replay_pause() alone only halts replay; the WAL receiver > > continues running, keeps receiving WAL, and sends feedback/status to > > the primary. That feedback is sufficient to advance restart_lsn on the > > standby’s slot, which would violate the restart_lsn < wait_for_lsn > > condition inside StandbySlotsHaveCaughtup(), which is not what we > > want. > > Yes, I see. IIUC, the same problem will be there if we use > recovery_min_apply_delay i.e., WALs will be received, flushed and > feedback will be sent to primary, only replay will be delayed. We can > use 'synchronous_commit = remote_apply' along with > 'recovery_min_apply_delay ' but that would mean delaying logical > replication because transaction commit is blocking not because standby > is actually lagging. It will not be a suitable test for > 'synchronized_satndby_slots'. > Even with synchronous_commit = remote_apply and paused replay, standby can still send replies to the primary updating the slot's restart_lsn. -- With Regards, Ashutosh Sharma.
-
RE: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2026-04-08T02:09:18Z
On Tuesday, April 7, 2026 9:54 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Tue, Apr 7, 2026 at 5:18 PM shveta malik <shveta.malik@gmail.com> > wrote: > > > > On Tue, Apr 7, 2026 at 3:56 PM Ashutosh Sharma > <ashu.coek88@gmail.com> wrote: > > > > > > Hi, > > > > > > On Tue, Apr 7, 2026 at 11:20 AM Ashutosh Sharma > <ashu.coek88@gmail.com> wrote: > > > > > > > > Hi, > > > > > > > > On Tue, Apr 7, 2026 at 9:04 AM shveta malik > <shveta.malik@gmail.com> wrote: > > > > > > > > > > > > > > > I see your point. I agree that using > > > > > wal_receiver_status_interval for this test may not be a reliable > > > > > way. Can we attempt using > > > > > pg_wal_replay_pause() on standby and then checking > > > > > wait_event=WaitForStandbyConfirmation with > > > > > backend_type=walsender on primary? Or do you see any issues in > > > > > this approach that I might be overlooking? > > > > > > > > > > > > > Yes, I think we can make use of the WAL replay pause/resume > mechanism. > > > > This seems like the right approach, as it gives us a more > > > > controlled and deterministic way to validate the lagging behavior. > > > > > > > > > > Looking at 049_wait_for_lsn.pl (the test case you referenced), it > > > explicitly stops the WAL receiver by setting primary_conninfo to an > > > empty string, rather than just pausing WAL replay. > > > > Oh, I missed it in that testcase. Setting primary_conninfo to NULL > > essentially means not starting the walreceiver and thus making the > > standby slot as inactive, for which we already have a testcase. > > > > > Using > > > pg_wal_replay_pause() alone only halts replay; the WAL receiver > > > continues running, keeps receiving WAL, and sends feedback/status to > > > the primary. That feedback is sufficient to advance restart_lsn on > > > the standby’s slot, which would violate the restart_lsn < > > > wait_for_lsn condition inside StandbySlotsHaveCaughtup(), which is > > > not what we want. > > > > Yes, I see. IIUC, the same problem will be there if we use > > recovery_min_apply_delay i.e., WALs will be received, flushed and > > feedback will be sent to primary, only replay will be delayed. We can > > use 'synchronous_commit = remote_apply' along with > > 'recovery_min_apply_delay ' but that would mean delaying logical > > replication because transaction commit is blocking not because standby > > is actually lagging. It will not be a suitable test for > > 'synchronized_satndby_slots'. > > > > Even with synchronous_commit = remote_apply and paused replay, standby > can still send replies to the primary updating the slot's restart_lsn. If we only want to keep the slot active without advancing restart_lsn, we could start a replication connection and then acquire the slot with the help of the replication command: START_REPLICATION SLOT physical 0/01788488; E.g., $standby->psql( 'postgres', qq[START_REPLICATION SLOT physical 0/01788488;], replication => 'database'); Best Regards, Hou zj
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-08T11:52:25Z
Hi, On Wed, Apr 8, 2026 at 7:39 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote: > > If we only want to keep the slot active without advancing restart_lsn, we could > start a replication connection and then acquire the slot with the help of > the replication command: START_REPLICATION SLOT physical 0/01788488; > > E.g., > > $standby->psql( > 'postgres', > qq[START_REPLICATION SLOT physical 0/01788488;], > replication => 'database'); > I see your point. You are suggesting to use psql as a replication client (instead of a standby or pg_receivewal) that doesn’t send feedback to the walsender unlike walreceiver in case of standbys. In that case, the slot remains active but restart_lsn doesn’t advance, effectively leaving it active but lagging. While exploring this further, I found "019_replslot_limit.pl", which uses SIGSTOP and SIGCONT to pause and resume the walsender process. Pausing the walsender prevents it from streaming new WAL to the standby, resulting in a slot that is active but lagging. I followed a similar approach to build a test case that creates an active yet lagging standby slot. This slot does not satisfy priority/quorum conditions for synchronized_standby_slots, causing the logical walsender to wait for standby confirmation. Once SIGCONT is sent to the paused walsender, WAL streaming resumes and the logical walsender, which was blocked waiting for standby confirmation, proceeds. The attached patch implements this. Please have a look and let me know your thoughts. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ajin Cherian <itsajin@gmail.com> — 2026-04-08T12:53:29Z
On Wed, Apr 8, 2026 at 9:52 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > On Wed, Apr 8, 2026 at 7:39 AM Zhijie Hou (Fujitsu) > <houzj.fnst@fujitsu.com> wrote: > > > > If we only want to keep the slot active without advancing restart_lsn, we could > > start a replication connection and then acquire the slot with the help of > > the replication command: START_REPLICATION SLOT physical 0/01788488; > > > > E.g., > > > > $standby->psql( > > 'postgres', > > qq[START_REPLICATION SLOT physical 0/01788488;], > > replication => 'database'); > > > > I see your point. You are suggesting to use psql as a replication > client (instead of a standby or pg_receivewal) that doesn’t send > feedback to the walsender unlike walreceiver in case of standbys. In > that case, the slot remains active but restart_lsn doesn’t advance, > effectively leaving it active but lagging. > > While exploring this further, I found "019_replslot_limit.pl", which > uses SIGSTOP and SIGCONT to pause and resume the walsender process. > Pausing the walsender prevents it from streaming new WAL to the > standby, resulting in a slot that is active but lagging. I followed a > similar approach to build a test case that creates an active yet > lagging standby slot. This slot does not satisfy priority/quorum > conditions for synchronized_standby_slots, causing the logical > walsender to wait for standby confirmation. Once SIGCONT is sent to > the paused walsender, WAL streaming resumes and the logical walsender, > which was blocked waiting for standby confirmation, proceeds. > I was just trying out Hou-san's suggestion and I came up with a different approach. Attaching my modified test script. If you think it is better, feel free to use it. regards, Ajin Cherian Fujitsu Australia
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-08T14:55:00Z
On Wed, Apr 8, 2026 at 6:23 PM Ajin Cherian <itsajin@gmail.com> wrote: > > On Wed, Apr 8, 2026 at 9:52 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi, > > > > On Wed, Apr 8, 2026 at 7:39 AM Zhijie Hou (Fujitsu) > > <houzj.fnst@fujitsu.com> wrote: > > > > > > If we only want to keep the slot active without advancing restart_lsn, we could > > > start a replication connection and then acquire the slot with the help of > > > the replication command: START_REPLICATION SLOT physical 0/01788488; > > > > > > E.g., > > > > > > $standby->psql( > > > 'postgres', > > > qq[START_REPLICATION SLOT physical 0/01788488;], > > > replication => 'database'); > > > > > > > I see your point. You are suggesting to use psql as a replication > > client (instead of a standby or pg_receivewal) that doesn’t send > > feedback to the walsender unlike walreceiver in case of standbys. In > > that case, the slot remains active but restart_lsn doesn’t advance, > > effectively leaving it active but lagging. > > > > While exploring this further, I found "019_replslot_limit.pl", which > > uses SIGSTOP and SIGCONT to pause and resume the walsender process. > > Pausing the walsender prevents it from streaming new WAL to the > > standby, resulting in a slot that is active but lagging. I followed a > > similar approach to build a test case that creates an active yet > > lagging standby slot. This slot does not satisfy priority/quorum > > conditions for synchronized_standby_slots, causing the logical > > walsender to wait for standby confirmation. Once SIGCONT is sent to > > the paused walsender, WAL streaming resumes and the logical walsender, > > which was blocked waiting for standby confirmation, proceeds. > > > > I was just trying out Hou-san's suggestion and I came up with a > different approach. Attaching my modified test script. > If you think it is better, feel free to use it. > Thanks Ajin for sharing your version based on Hou-san's suggestions. This approach looks more robust and is also platform independent, so in my view we should proceed with it. I’ll review the changes, incorporate them into the main patch, and share an updated patch. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-09T03:31:26Z
On Wed, Apr 8, 2026 at 6:23 PM Ajin Cherian <itsajin@gmail.com> wrote: > > On Wed, Apr 8, 2026 at 9:52 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi, > > > > On Wed, Apr 8, 2026 at 7:39 AM Zhijie Hou (Fujitsu) > > <houzj.fnst@fujitsu.com> wrote: > > > > > > If we only want to keep the slot active without advancing restart_lsn, we could > > > start a replication connection and then acquire the slot with the help of > > > the replication command: START_REPLICATION SLOT physical 0/01788488; > > > > > > E.g., > > > > > > $standby->psql( > > > 'postgres', > > > qq[START_REPLICATION SLOT physical 0/01788488;], > > > replication => 'database'); > > > > > > > I see your point. You are suggesting to use psql as a replication > > client (instead of a standby or pg_receivewal) that doesn’t send > > feedback to the walsender unlike walreceiver in case of standbys. In > > that case, the slot remains active but restart_lsn doesn’t advance, > > effectively leaving it active but lagging. > > > > While exploring this further, I found "019_replslot_limit.pl", which > > uses SIGSTOP and SIGCONT to pause and resume the walsender process. > > Pausing the walsender prevents it from streaming new WAL to the > > standby, resulting in a slot that is active but lagging. I followed a > > similar approach to build a test case that creates an active yet > > lagging standby slot. This slot does not satisfy priority/quorum > > conditions for synchronized_standby_slots, causing the logical > > walsender to wait for standby confirmation. Once SIGCONT is sent to > > the paused walsender, WAL streaming resumes and the logical walsender, > > which was blocked waiting for standby confirmation, proceeds. > > > > I was just trying out Hou-san's suggestion and I came up with a > different approach. Attaching my modified test script. > If you think it is better, feel free to use it. > Thanks Ajin. Would you be able to share the timing of test (complete file) as well with this change in testfile. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-09T03:36:21Z
On Wed, Apr 8, 2026 at 8:25 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > I was just trying out Hou-san's suggestion and I came up with a > > different approach. Attaching my modified test script. > > If you think it is better, feel free to use it. > > > > Thanks Ajin for sharing your version based on Hou-san's suggestions. > This approach looks more robust and is also platform independent, so > in my view we should proceed with it. +1. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ajin Cherian <itsajin@gmail.com> — 2026-04-09T04:24:54Z
On Thu, Apr 9, 2026 at 1:31 PM shveta malik <shveta.malik@gmail.com> wrote: > > Thanks Ajin. Would you be able to share the timing of test (complete > file) as well with this change in testfile. There was a small typo in the file I shared. (fixed and updated) The times are: # +++ tap check in src/test/recovery +++ [14:22:31] t/053_synchronized_standby_slots_quorum.pl .. ok 6453 ms ( 0.01 usr 0.00 sys + 0.36 cusr 0.49 csys = 0.86 CPU) [14:22:37] All tests successful. Files=1, Tests=14, 6 wallclock secs ( 0.02 usr 0.00 sys + 0.36 cusr 0.49 csys = 0.87 CPU) Result: PASS regards, Ajin Cherian Fujitsu Australia
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-09T10:02:04Z
On Thu, Apr 9, 2026 at 9:55 AM Ajin Cherian <itsajin@gmail.com> wrote: > > On Thu, Apr 9, 2026 at 1:31 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > Thanks Ajin. Would you be able to share the timing of test (complete > > file) as well with this change in testfile. > > There was a small typo in the file I shared. (fixed and updated) > > The times are: > > # +++ tap check in src/test/recovery +++ > [14:22:31] t/053_synchronized_standby_slots_quorum.pl .. ok 6453 > ms ( 0.01 usr 0.00 sys + 0.36 cusr 0.49 csys = 0.86 CPU) > [14:22:37] > All tests successful. > Files=1, Tests=14, 6 wallclock secs ( 0.02 usr 0.00 sys + 0.36 cusr > 0.49 csys = 0.87 CPU) > Result: PASS > Okay, looks good. 6 secs is far better than previous case of 60-100 secs. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Amit Kapila <amit.kapila16@gmail.com> — 2026-04-11T04:34:14Z
On Fri, Mar 20, 2026 at 2:08 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> wrote: > > > > Since we're reusing the same parser for two GUCs that have different > > interpretations of one syntax variant (the plain slot list), making the parser > > more general is a natural approach, especially given that the patch is adding > > new functionality here. > > > > My main concern is the IsPrioritySyncStandbySlotsSyntax() function. It > > introduces additional hard-coded parsing logic that duplicates what's already > > implemented in syncrep_gram.y. I'm also concerned about maintainability, > > particularly since we already discovered a bug in the hard-coded parser code [1] > > and the patch even added a tap-test (part E) to cover that path. All of this > > effort could be avoided by removing this function and leveraging functionality > > provided by the shared parser. > > > > The issue that you are referring to here was without this function. > > The idea here is to reuse the existing synchronous_standby_names > parser as-is, without changing its grammar or parse behavior. > synchronized_standby_slots differs only in post-parse interpretation > of simple-list syntax, so we add a local helper to disambiguate > explicit priority mode from plain lists before applying > synchronized_standby_slots semantics. > How about splitting the patch to separate out the ANY configuration as the first patch? Then we can focus on the FIRST configuration separately and it would be easier to evaluate whether changing the parser for it is worth the additional complexity. -- With Regards, Amit Kapila.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-04-11T05:26:12Z
Hi, On Fri, Apr 10, 2026 at 9:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Fri, Mar 20, 2026 at 2:08 PM Ashutosh Sharma <ashu.coek88@gmail.com> > wrote: > > > > On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> > wrote: > > > > > > Since we're reusing the same parser for two GUCs that have different > > > interpretations of one syntax variant (the plain slot list), making > the parser > > > more general is a natural approach, especially given that the patch is > adding > > > new functionality here. > > > > > > My main concern is the IsPrioritySyncStandbySlotsSyntax() function. It > > > introduces additional hard-coded parsing logic that duplicates what's > already > > > implemented in syncrep_gram.y. I'm also concerned about > maintainability, > > > particularly since we already discovered a bug in the hard-coded > parser code [1] > > > and the patch even added a tap-test (part E) to cover that path. All > of this > > > effort could be avoided by removing this function and leveraging > functionality > > > provided by the shared parser. > > > > > > > The issue that you are referring to here was without this function. > > > > The idea here is to reuse the existing synchronous_standby_names > > parser as-is, without changing its grammar or parse behavior. > > synchronized_standby_slots differs only in post-parse interpretation > > of simple-list syntax, so we add a local helper to disambiguate > > explicit priority mode from plain lists before applying > > synchronized_standby_slots semantics. > > > > How about splitting the patch to separate out the ANY configuration as > the first patch? Then we can focus on the FIRST configuration > separately and it would be easier to evaluate whether changing the > parser for it is worth the additional complexity. +1, this seems to be a reasonable approach.
-
RE: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2026-04-13T03:54:12Z
On Saturday, April 11, 2026 12:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Mar 20, 2026 at 2:08 PM Ashutosh Sharma <ashu.coek88@gmail.com> > wrote: > > > > On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 > <houzj.fnst@fujitsu.com> wrote: > > > > > > Since we're reusing the same parser for two GUCs that have different > > > interpretations of one syntax variant (the plain slot list), making > > > the parser more general is a natural approach, especially given that > > > the patch is adding new functionality here. > > > > > > My main concern is the IsPrioritySyncStandbySlotsSyntax() function. > > > It introduces additional hard-coded parsing logic that duplicates > > > what's already implemented in syncrep_gram.y. I'm also concerned > > > about maintainability, particularly since we already discovered a > > > bug in the hard-coded parser code [1] and the patch even added a > > > tap-test (part E) to cover that path. All of this effort could be > > > avoided by removing this function and leveraging functionality provided by > the shared parser. > > > > > > > The issue that you are referring to here was without this function. > > > > The idea here is to reuse the existing synchronous_standby_names > > parser as-is, without changing its grammar or parse behavior. > > synchronized_standby_slots differs only in post-parse interpretation > > of simple-list syntax, so we add a local helper to disambiguate > > explicit priority mode from plain lists before applying > > synchronized_standby_slots semantics. > > > > How about splitting the patch to separate out the ANY configuration as the > first patch? Then we can focus on the FIRST configuration separately and it > would be easier to evaluate whether changing the parser for it is worth the > additional complexity. +1 For reference only, I'm attaching a top-up patch (on top of v20260408-0001) that refactors the parsing logic to distinguish between FIRST and plain lists, as previously discussed. Best Regards, Hou zj
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-13T08:10:20Z
Hi, On Sat, Apr 11, 2026 at 10:04 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Mar 20, 2026 at 2:08 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> wrote: > > > > > > Since we're reusing the same parser for two GUCs that have different > > > interpretations of one syntax variant (the plain slot list), making the parser > > > more general is a natural approach, especially given that the patch is adding > > > new functionality here. > > > > > > My main concern is the IsPrioritySyncStandbySlotsSyntax() function. It > > > introduces additional hard-coded parsing logic that duplicates what's already > > > implemented in syncrep_gram.y. I'm also concerned about maintainability, > > > particularly since we already discovered a bug in the hard-coded parser code [1] > > > and the patch even added a tap-test (part E) to cover that path. All of this > > > effort could be avoided by removing this function and leveraging functionality > > > provided by the shared parser. > > > > > > > The issue that you are referring to here was without this function. > > > > The idea here is to reuse the existing synchronous_standby_names > > parser as-is, without changing its grammar or parse behavior. > > synchronized_standby_slots differs only in post-parse interpretation > > of simple-list syntax, so we add a local helper to disambiguate > > explicit priority mode from plain lists before applying > > synchronized_standby_slots semantics. > > > > How about splitting the patch to separate out the ANY configuration as > the first patch? Then we can focus on the FIRST configuration > separately and it would be easier to evaluate whether changing the > parser for it is worth the additional complexity. > That sounds like a good approach. I’ll split the current patch into two parts, with support for the ANY-based syntax being the first patch, and share it. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-14T10:56:01Z
Hi, On Mon, Apr 13, 2026 at 1:40 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > On Sat, Apr 11, 2026 at 10:04 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Fri, Mar 20, 2026 at 2:08 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> wrote: > > > > > > > > Since we're reusing the same parser for two GUCs that have different > > > > interpretations of one syntax variant (the plain slot list), making the parser > > > > more general is a natural approach, especially given that the patch is adding > > > > new functionality here. > > > > > > > > My main concern is the IsPrioritySyncStandbySlotsSyntax() function. It > > > > introduces additional hard-coded parsing logic that duplicates what's already > > > > implemented in syncrep_gram.y. I'm also concerned about maintainability, > > > > particularly since we already discovered a bug in the hard-coded parser code [1] > > > > and the patch even added a tap-test (part E) to cover that path. All of this > > > > effort could be avoided by removing this function and leveraging functionality > > > > provided by the shared parser. > > > > > > > > > > The issue that you are referring to here was without this function. > > > > > > The idea here is to reuse the existing synchronous_standby_names > > > parser as-is, without changing its grammar or parse behavior. > > > synchronized_standby_slots differs only in post-parse interpretation > > > of simple-list syntax, so we add a local helper to disambiguate > > > explicit priority mode from plain lists before applying > > > synchronized_standby_slots semantics. > > > > > > > How about splitting the patch to separate out the ANY configuration as > > the first patch? Then we can focus on the FIRST configuration > > separately and it would be easier to evaluate whether changing the > > parser for it is worth the additional complexity. > > > > That sounds like a good approach. I’ll split the current patch into > two parts, with support for the ANY-based syntax being the first > patch, and share it. > I've incorporated Ajin's incremental patch - [1] which includes the test case for logical walsender waiting on an active but lagging standby slot into the last main patch "v20260408-0001*.patch" - [2], and split it into two parts as suggested: the first one adds quorum-based syntax support for synchronized_standby_slots, and the second adds priority-based support (FIRST N). The two patches are attached below. Please take a look and share your thoughts. Hou-san's suggestion to update the common syncrep parser logic for the second patch is not included for now. I'll wait to hear from Amit and others before deciding whether to incorporate it. [1] - https://www.postgresql.org/message-id/CAFPTHDZFtHyJ3bijxcsTqgF%2BN3XseLGrWseiXDTGPJ%2BNM0AhdA%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CAE9k0P%3DJF_SRrYHb2t7CW-_We-vt1bsJForByf4cwYkgeQAJtg%40mail.gmail.com -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Amit Kapila <amit.kapila16@gmail.com> — 2026-04-14T11:25:58Z
On Tue, Apr 14, 2026 at 4:26 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hou-san's suggestion to update the common syncrep parser logic for the > second patch is not included for now. I'll wait to hear from Amit and > others before deciding whether to incorporate it. > The idea was to keep it as a 0003 patch atop 0002. That will help to evaluate both your and Hou-San's approaches. -- With Regards, Amit Kapila.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-14T12:31:39Z
On Tue, Apr 14, 2026 at 4:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Apr 14, 2026 at 4:26 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hou-san's suggestion to update the common syncrep parser logic for the > > second patch is not included for now. I'll wait to hear from Amit and > > others before deciding whether to incorporate it. > > > > The idea was to keep it as a 0003 patch atop 0002. That will help to > evaluate both your and Hou-San's approaches. > > -- Sure. Attached is the 0003 patch, based on Hou-San's suggestion, which refactors the syncrep parser to explicitly represent bare standby lists (i.e. those without the ANY or FIRST keyword). I've also attached the two preceding patches for completeness. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-15T05:39:18Z
On Tue, Apr 14, 2026 at 6:01 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > Sure. Attached is the 0003 patch, based on Hou-San's suggestion, which > refactors the syncrep parser to explicitly represent bare standby > lists (i.e. those without the ANY or FIRST keyword). I've also > attached the two preceding patches for completeness. > > -- Thanks Ashutosh. I was testing 001 alone (assuming we would like to commit it standalone). This part seems problematic in standlaone 001 patch: + /* + * For synchronized_standby_slots, a comma-separated list means all + * listed slots are required. The parser returns num_sync=1 in this + * shape, so map it to nmembers to enforce all-mode semantics. + */ + if (syncrep_parse_result->num_sync == 1 && + syncrep_parse_result->syncrep_method == SYNC_REP_PRIORITY && + syncrep_parse_result->nmembers > 1) + syncrep_parse_result->num_sync = syncrep_parse_result->nmembers; With this, 'FIRST 1(standby_1, standby_2)' is convered to wait_for_all mode, which is not correct and it keeps wiating for standby_2 when standby_1 has already taken changes. I am not sure what is correct way to deal with it when it comes to first patch alone. My expectation was that FIRST-syntax is blocked i.e. it errors out instead of partially implemented and misbehaving. But if we plan to do so, the challenge will be how to distinguish actual FIRST and comma separated list implicitly converted to 'FIRST 1' by syncrep parser. For that we will need either 003 or IsPrioritySyncStandbySlotsSyntax', thus defeating the whole purpose of separating the patches. What do you think on this? 001 is okay as is or we shall block FIRST? thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-04-15T06:30:16Z
Hi Shveta, On Wed, Apr 15, 2026 at 11:09 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Tue, Apr 14, 2026 at 6:01 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > Sure. Attached is the 0003 patch, based on Hou-San's suggestion, which > > refactors the syncrep parser to explicitly represent bare standby > > lists (i.e. those without the ANY or FIRST keyword). I've also > > attached the two preceding patches for completeness. > > > > -- > > Thanks Ashutosh. I was testing 001 alone (assuming we would like to > commit it standalone). > > This part seems problematic in standlaone 001 patch: > > + /* > + * For synchronized_standby_slots, a comma-separated list means all > + * listed slots are required. The parser returns num_sync=1 in this > + * shape, so map it to nmembers to enforce all-mode semantics. > + */ > + if (syncrep_parse_result->num_sync == 1 && > + syncrep_parse_result->syncrep_method == SYNC_REP_PRIORITY && > + syncrep_parse_result->nmembers > 1) > + syncrep_parse_result->num_sync = syncrep_parse_result->nmembers; > > > With this, 'FIRST 1(standby_1, standby_2)' is convered to wait_for_all > mode, which is not correct and it keeps wiating for standby_2 when > standby_1 has already taken changes. I am not sure what is correct way > to deal with it when it comes to first patch alone. My expectation was > that FIRST-syntax is blocked i.e. it errors out instead of partially > implemented and misbehaving. But if we plan to do so, the challenge > will be how to distinguish actual FIRST and comma separated list > implicitly converted to 'FIRST 1' by syncrep parser. For that we will > need either 003 or IsPrioritySyncStandbySlotsSyntax', thus defeating > the whole purpose of separating the patches. What do you think on > this? 001 is okay as is or we shall block FIRST? > AFAICS, we should first finalize the synchronous replication parser changes to give an identity to the plain list mode. Once that is settled, we may need to reorder the patch based on the decision we take. If we decide to proceed with the parser changes, my understanding is that they should come first, followed by the implementation of support for the ANY/FIRST clauses. Otherwise, we can move ahead with a single patch (or split it, if that seems cleaner and more appropriate). -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-04-15T06:47:17Z
On Wed, Apr 15, 2026 at 12:00 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi Shveta, > > On Wed, Apr 15, 2026 at 11:09 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Tue, Apr 14, 2026 at 6:01 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > Sure. Attached is the 0003 patch, based on Hou-San's suggestion, which > > > refactors the syncrep parser to explicitly represent bare standby > > > lists (i.e. those without the ANY or FIRST keyword). I've also > > > attached the two preceding patches for completeness. > > > > > > -- > > > > Thanks Ashutosh. I was testing 001 alone (assuming we would like to > > commit it standalone). > > > > This part seems problematic in standlaone 001 patch: > > > > + /* > > + * For synchronized_standby_slots, a comma-separated list means all > > + * listed slots are required. The parser returns num_sync=1 in this > > + * shape, so map it to nmembers to enforce all-mode semantics. > > + */ > > + if (syncrep_parse_result->num_sync == 1 && > > + syncrep_parse_result->syncrep_method == SYNC_REP_PRIORITY && > > + syncrep_parse_result->nmembers > 1) > > + syncrep_parse_result->num_sync = syncrep_parse_result->nmembers; > > > > > > With this, 'FIRST 1(standby_1, standby_2)' is convered to wait_for_all > > mode, which is not correct and it keeps wiating for standby_2 when > > standby_1 has already taken changes. I am not sure what is correct way > > to deal with it when it comes to first patch alone. My expectation was > > that FIRST-syntax is blocked i.e. it errors out instead of partially > > implemented and misbehaving. But if we plan to do so, the challenge > > will be how to distinguish actual FIRST and comma separated list > > implicitly converted to 'FIRST 1' by syncrep parser. For that we will > > need either 003 or IsPrioritySyncStandbySlotsSyntax', thus defeating > > the whole purpose of separating the patches. What do you think on > > this? 001 is okay as is or we shall block FIRST? > > > > AFAICS, we should first finalize the synchronous replication parser > changes to give an identity to the plain list mode. Once that is > settled, we may need to reorder the patch based on the decision we > take. If we decide to proceed with the parser changes, my > understanding is that they should come first, followed by the > implementation of support for the ANY/FIRST clauses. I agree. I do not see any other correct way to do it. A few trivial comments on 001: 1) +# B) ANY 1 (sb1_slot, sb2_slot) (quorum mode) +# - Proceeds when at least N slots have caught up +# - Skips missing/invalid/logical slots and lagging slots (inactive or active) +# to find N caught-up slots We can mention 'ANY N' instead of 'ANY 1' so that the explanation using 'N' makes more sense. 2) +sub poll_query_until_quiet +{ + my ($node, $dbname, $query, $expected, $timeout_secs) = @_; + + $expected = 't' unless defined($expected); + $timeout_secs = $PostgreSQL::Test::Utils::timeout_default We can get rid of this function now. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-05-13T11:55:25Z
Hi, On Wed, Apr 15, 2026 at 12:17 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Wed, Apr 15, 2026 at 12:00 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > On Wed, Apr 15, 2026 at 11:09 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > With this, 'FIRST 1(standby_1, standby_2)' is convered to wait_for_all > > > mode, which is not correct and it keeps wiating for standby_2 when > > > standby_1 has already taken changes. I am not sure what is correct way > > > to deal with it when it comes to first patch alone. My expectation was > > > that FIRST-syntax is blocked i.e. it errors out instead of partially > > > implemented and misbehaving. But if we plan to do so, the challenge > > > will be how to distinguish actual FIRST and comma separated list > > > implicitly converted to 'FIRST 1' by syncrep parser. For that we will > > > need either 003 or IsPrioritySyncStandbySlotsSyntax', thus defeating > > > the whole purpose of separating the patches. What do you think on > > > this? 001 is okay as is or we shall block FIRST? > > > > > > > AFAICS, we should first finalize the synchronous replication parser > > changes to give an identity to the plain list mode. Once that is > > settled, we may need to reorder the patch based on the decision we > > take. If we decide to proceed with the parser changes, my > > understanding is that they should come first, followed by the > > implementation of support for the ANY/FIRST clauses. > > I agree. I do not see any other correct way to do it. > I've reordered the patches as mentioned here to ensure each patch is independently functional. The updated sequence is: 1) 0001-Refactor-syncrep-parsing-to-represent-bare-standby-l.patch : Introduces a new synchronous replication method SYNC_REP_DEFAULT to represent the bare list form parsed from standby_list. This lets callers clearly distinguish between three forms: - Explicit priority syntax — FIRST N (...) or N (...) - Quorum syntax — ANY N (...) - Simple list syntax — no FIRST or ANY keyword 2) 0002-Add-ANY-N-semantics-to-synchronized_standby_slots.patch : Extends synchronized_standby_slots with ANY N quorum semantics. 3) 0003-Add-FIRST-N-and-N-.-priority-syntax-to-synchronized_.patch : Adds support for FIRST N and N (...) priority syntax to synchronized_standby_slots > > A few trivial comments on 001: > > 1) > +# B) ANY 1 (sb1_slot, sb2_slot) (quorum mode) > +# - Proceeds when at least N slots have caught up > +# - Skips missing/invalid/logical slots and lagging slots > (inactive or active) > +# to find N caught-up slots > > We can mention 'ANY N' instead of 'ANY 1' so that the explanation > using 'N' makes more sense. > > 2) > +sub poll_query_until_quiet > +{ > + my ($node, $dbname, $query, $expected, $timeout_secs) = @_; > + > + $expected = 't' unless defined($expected); > + $timeout_secs = $PostgreSQL::Test::Utils::timeout_default > > We can get rid of this function now. Above two comments have been taken care of in the attached patches. Please take a look and let me know your views. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-05-14T11:06:18Z
On Wed, May 13, 2026 at 5:25 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > On Wed, Apr 15, 2026 at 12:17 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Wed, Apr 15, 2026 at 12:00 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > On Wed, Apr 15, 2026 at 11:09 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > With this, 'FIRST 1(standby_1, standby_2)' is convered to wait_for_all > > > > mode, which is not correct and it keeps wiating for standby_2 when > > > > standby_1 has already taken changes. I am not sure what is correct way > > > > to deal with it when it comes to first patch alone. My expectation was > > > > that FIRST-syntax is blocked i.e. it errors out instead of partially > > > > implemented and misbehaving. But if we plan to do so, the challenge > > > > will be how to distinguish actual FIRST and comma separated list > > > > implicitly converted to 'FIRST 1' by syncrep parser. For that we will > > > > need either 003 or IsPrioritySyncStandbySlotsSyntax', thus defeating > > > > the whole purpose of separating the patches. What do you think on > > > > this? 001 is okay as is or we shall block FIRST? > > > > > > > > > > AFAICS, we should first finalize the synchronous replication parser > > > changes to give an identity to the plain list mode. Once that is > > > settled, we may need to reorder the patch based on the decision we > > > take. If we decide to proceed with the parser changes, my > > > understanding is that they should come first, followed by the > > > implementation of support for the ANY/FIRST clauses. > > > > I agree. I do not see any other correct way to do it. > > > > I've reordered the patches as mentioned here to ensure each patch is > independently functional. The updated sequence is: > > 1) 0001-Refactor-syncrep-parsing-to-represent-bare-standby-l.patch : > Introduces a new synchronous replication method SYNC_REP_DEFAULT to > represent the bare list form parsed from standby_list. This lets > callers clearly distinguish between three forms: > > - Explicit priority syntax — FIRST N (...) or N (...) > - Quorum syntax — ANY N (...) > - Simple list syntax — no FIRST or ANY keyword > > 2) 0002-Add-ANY-N-semantics-to-synchronized_standby_slots.patch : > Extends synchronized_standby_slots with ANY N quorum semantics. > > 3) 0003-Add-FIRST-N-and-N-.-priority-syntax-to-synchronized_.patch : > Adds support for FIRST N and N (...) priority syntax to > synchronized_standby_slots > > > > > A few trivial comments on 001: > > > > 1) > > +# B) ANY 1 (sb1_slot, sb2_slot) (quorum mode) > > +# - Proceeds when at least N slots have caught up > > +# - Skips missing/invalid/logical slots and lagging slots > > (inactive or active) > > +# to find N caught-up slots > > > > We can mention 'ANY N' instead of 'ANY 1' so that the explanation > > using 'N' makes more sense. > > > > 2) > > +sub poll_query_until_quiet > > +{ > > + my ($node, $dbname, $query, $expected, $timeout_secs) = @_; > > + > > + $expected = 't' unless defined($expected); > > + $timeout_secs = $PostgreSQL::Test::Utils::timeout_default > > > > We can get rid of this function now. > > Above two comments have been taken care of in the attached patches. > > Please take a look and let me know your views. > Okay, it seems much better now. IMO, the changes of 001 are simple enough to be incorporated but lets see what others have to say. By changing the order of patches, 0002 (ANY) now works well, the previously reported problem is gone. I had yet to verify 003 compeltely. Will finish the reveiw soon. A few minor points, though: patch 002: 1) + * The layout mirrors SyncRepConfigData so that the same quorum/priority quorum/priority --> quorum and priority 2) + if (!inactive) + slot_states[num_slot_states].restart_lsn = restart_lsn; I think there is no harm in making this assignmenet irrespective of 'inactive' flag. The code will look clean. patch003: 3) - wait_for_all = (required == synchronized_standby_slots_config->nslotnames); + wait_for_all = + (synchronized_standby_slots_config->syncrep_method == SYNC_REP_DEFAULT); We could have done the same in patch002 itself, or not? 4) - /* Stop processing if the required number of slots have caught up. */ + /* + * Stop processing if the required number of slots have caught up. In + * priority mode (FIRST N), this ensures we select the first N slots in + * sequential order. In quorum mode (ANY N), we still process in order for + * efficiency, stopping once we find any N. + */ I am not able to understand this comment very well despite of knowing the code well. Do you think we can improve it? Or was the first sentence enough (the existing one)? That explained it well. ~~ Reviewing and verifying further. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-05-15T03:58:35Z
On Thu, May 14, 2026 at 4:36 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Wed, May 13, 2026 at 5:25 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi, > > > > On Wed, Apr 15, 2026 at 12:17 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > On Wed, Apr 15, 2026 at 12:00 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > > > > > > > On Wed, Apr 15, 2026 at 11:09 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > > > With this, 'FIRST 1(standby_1, standby_2)' is convered to wait_for_all > > > > > mode, which is not correct and it keeps wiating for standby_2 when > > > > > standby_1 has already taken changes. I am not sure what is correct way > > > > > to deal with it when it comes to first patch alone. My expectation was > > > > > that FIRST-syntax is blocked i.e. it errors out instead of partially > > > > > implemented and misbehaving. But if we plan to do so, the challenge > > > > > will be how to distinguish actual FIRST and comma separated list > > > > > implicitly converted to 'FIRST 1' by syncrep parser. For that we will > > > > > need either 003 or IsPrioritySyncStandbySlotsSyntax', thus defeating > > > > > the whole purpose of separating the patches. What do you think on > > > > > this? 001 is okay as is or we shall block FIRST? > > > > > > > > > > > > > AFAICS, we should first finalize the synchronous replication parser > > > > changes to give an identity to the plain list mode. Once that is > > > > settled, we may need to reorder the patch based on the decision we > > > > take. If we decide to proceed with the parser changes, my > > > > understanding is that they should come first, followed by the > > > > implementation of support for the ANY/FIRST clauses. > > > > > > I agree. I do not see any other correct way to do it. > > > > > > > I've reordered the patches as mentioned here to ensure each patch is > > independently functional. The updated sequence is: > > > > 1) 0001-Refactor-syncrep-parsing-to-represent-bare-standby-l.patch : > > Introduces a new synchronous replication method SYNC_REP_DEFAULT to > > represent the bare list form parsed from standby_list. This lets > > callers clearly distinguish between three forms: > > > > - Explicit priority syntax — FIRST N (...) or N (...) > > - Quorum syntax — ANY N (...) > > - Simple list syntax — no FIRST or ANY keyword > > > > 2) 0002-Add-ANY-N-semantics-to-synchronized_standby_slots.patch : > > Extends synchronized_standby_slots with ANY N quorum semantics. > > > > 3) 0003-Add-FIRST-N-and-N-.-priority-syntax-to-synchronized_.patch : > > Adds support for FIRST N and N (...) priority syntax to > > synchronized_standby_slots > > > > > > > > A few trivial comments on 001: > > > > > > 1) > > > +# B) ANY 1 (sb1_slot, sb2_slot) (quorum mode) > > > +# - Proceeds when at least N slots have caught up > > > +# - Skips missing/invalid/logical slots and lagging slots > > > (inactive or active) > > > +# to find N caught-up slots > > > > > > We can mention 'ANY N' instead of 'ANY 1' so that the explanation > > > using 'N' makes more sense. > > > > > > 2) > > > +sub poll_query_until_quiet > > > +{ > > > + my ($node, $dbname, $query, $expected, $timeout_secs) = @_; > > > + > > > + $expected = 't' unless defined($expected); > > > + $timeout_secs = $PostgreSQL::Test::Utils::timeout_default > > > > > > We can get rid of this function now. > > > > Above two comments have been taken care of in the attached patches. > > > > Please take a look and let me know your views. > > > > Okay, it seems much better now. IMO, the changes of 001 are simple > enough to be incorporated but lets see what others have to say. > > By changing the order of patches, 0002 (ANY) now works well, the > previously reported problem is gone. I had yet to verify 003 > compeltely. Will finish the reveiw soon. > > A few minor points, though: > > patch 002: > > 1) > + * The layout mirrors SyncRepConfigData so that the same quorum/priority > > quorum/priority --> quorum and priority > > 2) > + if (!inactive) > + slot_states[num_slot_states].restart_lsn = restart_lsn; > > I think there is no harm in making this assignmenet irrespective of > 'inactive' flag. The code will look clean. > > > patch003: > > 3) > - wait_for_all = (required == synchronized_standby_slots_config->nslotnames); > + wait_for_all = > + (synchronized_standby_slots_config->syncrep_method == SYNC_REP_DEFAULT); > > We could have done the same in patch002 itself, or not? > > 4) > - /* Stop processing if the required number of slots have caught up. */ > + /* > + * Stop processing if the required number of slots have caught up. In > + * priority mode (FIRST N), this ensures we select the first N slots in > + * sequential order. In quorum mode (ANY N), we still process in order for > + * efficiency, stopping once we find any N. > + */ > > I am not able to understand this comment very well despite of knowing > the code well. Do you think we can improve it? Or was the first > sentence enough (the existing one)? That explained it well. > > ~~ > Ashutosh, while testing further, I noticed that 'synchronized_standby_slots' does not filter duplicate entries. As an example, if user ends up giving one entry twice in priority configuration, then we will end up waiting on one slot twice rather than waiting on 2 different slots. Example: alter system set synchronized_standby_slots = 'FIRST 2 (standby_1, standby_1, standby_2, standby_3)'; select pg_reload_conf(); insert into tab1 values (10), (20), (30); select pg_logical_slot_get_binary_changes('sub1', NULL, NULL, 'proto_version', '4', 'publication_names', 'pub1'); The last statement works even though standby_2 and standby_3 do not exist. It consumes standby_1 twice and thinks that the required number of slots has caught-up. OTOH, if we use the same configuration for 'synchronous_standby_names', it correctly waits for standby_2 and does not count on standby_1 twice. alter system set synchronous_standby_names = 'FIRST 2 (standby_1, standby_1, standby_2, standby_3)'; insert into tab1 values (10), (20), (30); ----> This will wait on standby_2 This is perhaps because 'synchronous_standby_names ' waits on active WAL senders rather than repeated strings in configuration. But our code changes wait on the names present in 'synchronized_standby_slots' without filtering out duplicates. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-05-21T09:12:15Z
Hi Shveta, On Fri, May 15, 2026 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > Ashutosh, while testing further, I noticed that > 'synchronized_standby_slots' does not filter duplicate entries. As an > example, if user ends up giving one entry twice in priority > configuration, then we will end up waiting on one slot twice rather > than waiting on 2 different slots. > Thank you for raising this concern. It is indeed an issue that needs fixing. We will ensure it is addressed in the next patch version. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-03T11:00:23Z
Hi Shveta, On Fri, May 15, 2026 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > > Ashutosh, while testing further, I noticed that > 'synchronized_standby_slots' does not filter duplicate entries. As an > example, if user ends up giving one entry twice in priority > configuration, then we will end up waiting on one slot twice rather > than waiting on 2 different slots. > > Example: > alter system set synchronized_standby_slots = 'FIRST 2 (standby_1, > standby_1, standby_2, standby_3)'; > select pg_reload_conf(); > insert into tab1 values (10), (20), (30); > select pg_logical_slot_get_binary_changes('sub1', NULL, NULL, > 'proto_version', '4', 'publication_names', 'pub1'); > > The last statement works even though standby_2 and standby_3 do not > exist. It consumes standby_1 twice and thinks that the required number > of slots has caught-up. > > OTOH, if we use the same configuration for > 'synchronous_standby_names', it correctly waits for standby_2 and does > not count on standby_1 twice. > > alter system set synchronous_standby_names = 'FIRST 2 (standby_1, > standby_1, standby_2, standby_3)'; > insert into tab1 values (10), (20), (30); ----> This will wait on standby_2 > > This is perhaps because 'synchronous_standby_names ' waits on active > WAL senders rather than repeated strings in configuration. But our > code changes wait on the names present in 'synchronized_standby_slots' > without filtering out duplicates. > May I know what your expectation is here? Would you like the check hook for synchronized_standby_slots to automatically resolve duplicates into a unique set of values, or should it detect duplicate entries and raise an error so that the user can correct the configuration? If we automatically resolve duplicates, the user would still see the GUC configured exactly as they specified, even though it would not function the same way internally. For example, if a user sets: FIRST 2 (s1, s1, s1, s2) it might internally be resolved to: FIRST 2 (s1, s2) However, when the user runs SHOW, it would still display the original configuration. This could give the user an incorrect impression of how the setting is actually being interpreted. Because of this, I feel we should treat duplicate entries as an invalid configuration and raise an error. As far as synchronous_standby_names is concerned, I can see that configurations such as: FIRST 2 (s1, s1, s1, s1) are currently accepted, which I don't think is correct either and should have been rejected, possibly resulted in the server startup failure. -- With Regards, Ashutosh Sharma, -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-06-04T03:43:52Z
On Wed, Jun 3, 2026 at 4:30 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi Shveta, > > On Fri, May 15, 2026 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > Ashutosh, while testing further, I noticed that > > 'synchronized_standby_slots' does not filter duplicate entries. As an > > example, if user ends up giving one entry twice in priority > > configuration, then we will end up waiting on one slot twice rather > > than waiting on 2 different slots. > > > > Example: > > alter system set synchronized_standby_slots = 'FIRST 2 (standby_1, > > standby_1, standby_2, standby_3)'; > > select pg_reload_conf(); > > insert into tab1 values (10), (20), (30); > > select pg_logical_slot_get_binary_changes('sub1', NULL, NULL, > > 'proto_version', '4', 'publication_names', 'pub1'); > > > > The last statement works even though standby_2 and standby_3 do not > > exist. It consumes standby_1 twice and thinks that the required number > > of slots has caught-up. > > > > OTOH, if we use the same configuration for > > 'synchronous_standby_names', it correctly waits for standby_2 and does > > not count on standby_1 twice. > > > > alter system set synchronous_standby_names = 'FIRST 2 (standby_1, > > standby_1, standby_2, standby_3)'; > > insert into tab1 values (10), (20), (30); ----> This will wait on standby_2 > > > > This is perhaps because 'synchronous_standby_names ' waits on active > > WAL senders rather than repeated strings in configuration. But our > > code changes wait on the names present in 'synchronized_standby_slots' > > without filtering out duplicates. > > > > May I know what your expectation is here? Would you like the check > hook for synchronized_standby_slots to automatically resolve > duplicates into a unique set of values, or should it detect duplicate > entries and raise an error so that the user can correct the > configuration? > > If we automatically resolve duplicates, the user would still see the > GUC configured exactly as they specified, even though it would not > function the same way internally. For example, if a user sets: > > FIRST 2 (s1, s1, s1, s2) > > it might internally be resolved to: > > FIRST 2 (s1, s2) > > However, when the user runs SHOW, it would still display the original > configuration. This could give the user an incorrect impression of how > the setting is actually being interpreted. Because of this, I feel we > should treat duplicate entries as an invalid configuration and raise > an error. > > As far as synchronous_standby_names is concerned, I can see that > configurations such as: > > FIRST 2 (s1, s1, s1, s1) > > are currently accepted, which I don't think is correct either and > should have been rejected, possibly resulted in the server startup > failure. > My preference, and original intent, was to accept duplicate entries and skip them internally. Doc can be updated to say 'duplicate entries are skipped'. A server startup failure due to duplicate entries in a GUC does not seem right to me. If the alter-system command fails due to duplicate entries, that is still fine, but a startup failure seems excessive. But let's see what others have to say on this. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-04T07:36:09Z
Hi, On Thu, Jun 4, 2026 at 9:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Wed, Jun 3, 2026 at 4:30 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi Shveta, > > > > On Fri, May 15, 2026 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > > Ashutosh, while testing further, I noticed that > > > 'synchronized_standby_slots' does not filter duplicate entries. As an > > > example, if user ends up giving one entry twice in priority > > > configuration, then we will end up waiting on one slot twice rather > > > than waiting on 2 different slots. > > > > > > Example: > > > alter system set synchronized_standby_slots = 'FIRST 2 (standby_1, > > > standby_1, standby_2, standby_3)'; > > > select pg_reload_conf(); > > > insert into tab1 values (10), (20), (30); > > > select pg_logical_slot_get_binary_changes('sub1', NULL, NULL, > > > 'proto_version', '4', 'publication_names', 'pub1'); > > > > > > The last statement works even though standby_2 and standby_3 do not > > > exist. It consumes standby_1 twice and thinks that the required number > > > of slots has caught-up. > > > > > > OTOH, if we use the same configuration for > > > 'synchronous_standby_names', it correctly waits for standby_2 and does > > > not count on standby_1 twice. > > > > > > alter system set synchronous_standby_names = 'FIRST 2 (standby_1, > > > standby_1, standby_2, standby_3)'; > > > insert into tab1 values (10), (20), (30); ----> This will wait on standby_2 > > > > > > This is perhaps because 'synchronous_standby_names ' waits on active > > > WAL senders rather than repeated strings in configuration. But our > > > code changes wait on the names present in 'synchronized_standby_slots' > > > without filtering out duplicates. > > > > > > > May I know what your expectation is here? Would you like the check > > hook for synchronized_standby_slots to automatically resolve > > duplicates into a unique set of values, or should it detect duplicate > > entries and raise an error so that the user can correct the > > configuration? > > > > If we automatically resolve duplicates, the user would still see the > > GUC configured exactly as they specified, even though it would not > > function the same way internally. For example, if a user sets: > > > > FIRST 2 (s1, s1, s1, s2) > > > > it might internally be resolved to: > > > > FIRST 2 (s1, s2) > > > > However, when the user runs SHOW, it would still display the original > > configuration. This could give the user an incorrect impression of how > > the setting is actually being interpreted. Because of this, I feel we > > should treat duplicate entries as an invalid configuration and raise > > an error. > > > > As far as synchronous_standby_names is concerned, I can see that > > configurations such as: > > > > FIRST 2 (s1, s1, s1, s1) > > > > are currently accepted, which I don't think is correct either and > > should have been rejected, possibly resulted in the server startup > > failure. > > > > My preference, and original intent, was to accept duplicate entries > and skip them internally. Doc can be updated to say 'duplicate entries > are skipped'. A server startup failure due to duplicate entries in a > GUC does not seem right to me. If the alter-system command fails due > to duplicate entries, that is still fine, but a startup failure seems > excessive. But let's see what others have to say on this. > Okay, the attached patch adds the capability to automatically remove duplicate entries from the synchronized_standby_slots list. In N of M mode, if N > M after removing duplicate entries, an error is raised. This behavior has been documented, and test cases verifying the change have been added. A few other minor comments from [1] have also been addressed. Please have a look at the attached patches with these changes. [1] - https://www.postgresql.org/message-id/CAJpy0uCKGCkfCXCd%3DtsDH5e85x155LsdbZW46WpWfsZJUe82bw%40mail.gmail.com -- With Regards, Ashutosh Sharma. -
RE: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2026-06-04T08:24:24Z
On Thursday, June 4, 2026 3:36 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > On Thu, Jun 4, 2026 at 9:14 AM shveta malik <shveta.malik@gmail.com> > wrote: > > > > On Wed, Jun 3, 2026 at 4:30 PM Ashutosh Sharma > <ashu.coek88@gmail.com> wrote: > > > On Fri, May 15, 2026 at 9:28 AM shveta malik <shveta.malik@gmail.com> > wrote: > > > > > > > > > > > > Ashutosh, while testing further, I noticed that > > > > 'synchronized_standby_slots' does not filter duplicate entries. As an > > > > example, if user ends up giving one entry twice in priority > > > > configuration, then we will end up waiting on one slot twice rather > > > > than waiting on 2 different slots. > > > > > > > > Example: > > > > alter system set synchronized_standby_slots = 'FIRST 2 (standby_1, > > > > standby_1, standby_2, standby_3)'; > > > > select pg_reload_conf(); > > > > insert into tab1 values (10), (20), (30); > > > > select pg_logical_slot_get_binary_changes('sub1', NULL, NULL, > > > > 'proto_version', '4', 'publication_names', 'pub1'); > > > > > > > > The last statement works even though standby_2 and standby_3 do not > > > > exist. It consumes standby_1 twice and thinks that the required number > > > > of slots has caught-up. > > > > > > > > OTOH, if we use the same configuration for > > > > 'synchronous_standby_names', it correctly waits for standby_2 and does > > > > not count on standby_1 twice. > > > > > > > > alter system set synchronous_standby_names = 'FIRST 2 (standby_1, > > > > standby_1, standby_2, standby_3)'; > > > > insert into tab1 values (10), (20), (30); ----> This will wait on standby_2 > > > > > > > > This is perhaps because 'synchronous_standby_names ' waits on active > > > > WAL senders rather than repeated strings in configuration. But our > > > > code changes wait on the names present in > 'synchronized_standby_slots' > > > > without filtering out duplicates. > > > > > > > > > > May I know what your expectation is here? Would you like the check > > > hook for synchronized_standby_slots to automatically resolve > > > duplicates into a unique set of values, or should it detect duplicate > > > entries and raise an error so that the user can correct the > > > configuration? > > > > > > If we automatically resolve duplicates, the user would still see the > > > GUC configured exactly as they specified, even though it would not > > > function the same way internally. For example, if a user sets: > > > > > > FIRST 2 (s1, s1, s1, s2) > > > > > > it might internally be resolved to: > > > > > > FIRST 2 (s1, s2) > > > > > > However, when the user runs SHOW, it would still display the original > > > configuration. This could give the user an incorrect impression of how > > > the setting is actually being interpreted. Because of this, I feel we > > > should treat duplicate entries as an invalid configuration and raise > > > an error. > > > > > > As far as synchronous_standby_names is concerned, I can see that > > > configurations such as: > > > > > > FIRST 2 (s1, s1, s1, s1) > > > > > > are currently accepted, which I don't think is correct either and > > > should have been rejected, possibly resulted in the server startup > > > failure. > > > > > > > My preference, and original intent, was to accept duplicate entries > > and skip them internally. Doc can be updated to say 'duplicate entries > > are skipped'. A server startup failure due to duplicate entries in a > > GUC does not seem right to me. If the alter-system command fails due > > to duplicate entries, that is still fine, but a startup failure seems > > excessive. But let's see what others have to say on this. > > > > Okay, the attached patch adds the capability to automatically remove > duplicate entries from the synchronized_standby_slots list. Thanks for updating the patch. I agree with Shveta that reporting an ERROR is not ideal. I also think it (ERROR) would be inconsistent with existing GUCs, as most of them, such as synchronous_standby_names, search_path, and session_preload_libraries, do not enforce uniqueness. The most similar GUC, synchronous_standby_names, also clarifies this in the documentation: " There is no mechanism to enforce uniqueness of standby names. In case of duplicates one of the matching standbys will be considered as higher priority, though exactly which one is indeterminate."[1] > In N of M > mode, if N > M after removing duplicate entries, an error is raised. I'm not entirely sure about this case. It seems similar to when the number of specified slots is less than N (in ANY N or FIRST N), given that we want to skip duplicate slots. In that situation, the natural behavior to me would be to simply block replication rather than raise an error. And synchronous_standby_names would also simply block the transaction in this case. [1] https://www.postgresql.org/docs/devel/runtime-config-replication.html#GUC-SYNCHRONOUS-STANDBY-NAMES Best Regards, Hou zj -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-04T09:26:59Z
Hi, Thanks for your feedback. On Thu, Jun 4, 2026 at 1:54 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote: > > On Thursday, June 4, 2026 3:36 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Thu, Jun 4, 2026 at 9:14 AM shveta malik <shveta.malik@gmail.com> > > wrote: > > > > > > On Wed, Jun 3, 2026 at 4:30 PM Ashutosh Sharma > > <ashu.coek88@gmail.com> wrote: > > > > On Fri, May 15, 2026 at 9:28 AM shveta malik <shveta.malik@gmail.com> > > wrote: > > > > > > > > > > > > > > > Ashutosh, while testing further, I noticed that > > > > > 'synchronized_standby_slots' does not filter duplicate entries. As an > > > > > example, if user ends up giving one entry twice in priority > > > > > configuration, then we will end up waiting on one slot twice rather > > > > > than waiting on 2 different slots. > > > > > > > > > > Example: > > > > > alter system set synchronized_standby_slots = 'FIRST 2 (standby_1, > > > > > standby_1, standby_2, standby_3)'; > > > > > select pg_reload_conf(); > > > > > insert into tab1 values (10), (20), (30); > > > > > select pg_logical_slot_get_binary_changes('sub1', NULL, NULL, > > > > > 'proto_version', '4', 'publication_names', 'pub1'); > > > > > > > > > > The last statement works even though standby_2 and standby_3 do not > > > > > exist. It consumes standby_1 twice and thinks that the required number > > > > > of slots has caught-up. > > > > > > > > > > OTOH, if we use the same configuration for > > > > > 'synchronous_standby_names', it correctly waits for standby_2 and does > > > > > not count on standby_1 twice. > > > > > > > > > > alter system set synchronous_standby_names = 'FIRST 2 (standby_1, > > > > > standby_1, standby_2, standby_3)'; > > > > > insert into tab1 values (10), (20), (30); ----> This will wait on standby_2 > > > > > > > > > > This is perhaps because 'synchronous_standby_names ' waits on active > > > > > WAL senders rather than repeated strings in configuration. But our > > > > > code changes wait on the names present in > > 'synchronized_standby_slots' > > > > > without filtering out duplicates. > > > > > > > > > > > > > May I know what your expectation is here? Would you like the check > > > > hook for synchronized_standby_slots to automatically resolve > > > > duplicates into a unique set of values, or should it detect duplicate > > > > entries and raise an error so that the user can correct the > > > > configuration? > > > > > > > > If we automatically resolve duplicates, the user would still see the > > > > GUC configured exactly as they specified, even though it would not > > > > function the same way internally. For example, if a user sets: > > > > > > > > FIRST 2 (s1, s1, s1, s2) > > > > > > > > it might internally be resolved to: > > > > > > > > FIRST 2 (s1, s2) > > > > > > > > However, when the user runs SHOW, it would still display the original > > > > configuration. This could give the user an incorrect impression of how > > > > the setting is actually being interpreted. Because of this, I feel we > > > > should treat duplicate entries as an invalid configuration and raise > > > > an error. > > > > > > > > As far as synchronous_standby_names is concerned, I can see that > > > > configurations such as: > > > > > > > > FIRST 2 (s1, s1, s1, s1) > > > > > > > > are currently accepted, which I don't think is correct either and > > > > should have been rejected, possibly resulted in the server startup > > > > failure. > > > > > > > > > > My preference, and original intent, was to accept duplicate entries > > > and skip them internally. Doc can be updated to say 'duplicate entries > > > are skipped'. A server startup failure due to duplicate entries in a > > > GUC does not seem right to me. If the alter-system command fails due > > > to duplicate entries, that is still fine, but a startup failure seems > > > excessive. But let's see what others have to say on this. > > > > > > > Okay, the attached patch adds the capability to automatically remove > > duplicate entries from the synchronized_standby_slots list. > > Thanks for updating the patch. > > I agree with Shveta that reporting an ERROR is not ideal. I also think it (ERROR) would > be inconsistent with existing GUCs, as most of them, such as > synchronous_standby_names, search_path, and session_preload_libraries, do not > enforce uniqueness. > > The most similar GUC, synchronous_standby_names, also clarifies this in the > documentation: > > " There is no mechanism to enforce uniqueness of standby names. In case of > duplicates one of the matching standbys will be considered as higher priority, > though exactly which one is indeterminate."[1] > > > In N of M > > mode, if N > M after removing duplicate entries, an error is raised. > > I'm not entirely sure about this case. It seems similar to when the number of > specified slots is less than N (in ANY N or FIRST N), given that we want to skip > duplicate slots. In that situation, the natural behavior to me would be to > simply block replication rather than raise an error. And > synchronous_standby_names would also simply block the transaction in this case. > For duplicate entries themselves, I agree with the direction of not raising an error. Silently normalizing duplicates is reasonable for this GUC, especially if we document it clearly. A repeated slot name does not add any new information, so treating it as “same slot listed twice by mistake” is practical. But for N > M after deduplication, I would still lean toward raising an error. Why I’d separate those cases: 1) Duplicate entries looks like a harmless normalization problem. ANY 2 (a, a, b) can be normalized to ANY 2 (a, b) without changing the user’s apparent intent much. 2) N > M after deduplication is not a transient runtime state. ANY 2 (a, a) becomes one unique slot. That configuration can never succeed unless the config itself changes. Blocking forever turns a static configuration mistake into an operational liveness problem. 3) N > M after deduplication is different from ordinary “not enough standbys are currently available”. If we configure ANY 2 (a, b) and only a is currently caught up, blocking makes sense because the situation may resolve at runtime. If we configure ANY 2 (a, a) and duplicates are ignored, there is no possible future runtime in which it succeeds without editing the GUC. That is why I think erroring is better. On the synchronous_standby_names comparison, I do not think it is fully analogous. The quoted documentation is about there being no reliable way to enforce uniqueness of standby names in the live system, because those names are matched against runtime standbys and the result can be indeterminate. Here, synchronized_standby_slots names concrete replication slots, which are stable object identifiers. Duplicate config entries are detectable and normalizable deterministically at GUC parse time. That gives us a cleaner option than synchronous_standby_names has. So my preferred behavior would be: 1) duplicate names: normalize, do not error 2) after normalization, if num_sync > unique_slots: error immediately -- With Regards, Ashutosh Sharma. -
RE: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2026-06-05T03:04:08Z
On Thursday, June 4, 2026 5:27 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > On Thu, Jun 4, 2026 at 1:54 PM Zhijie Hou (Fujitsu) > <houzj.fnst@fujitsu.com> wrote: > > > > On Thursday, June 4, 2026 3:36 PM Ashutosh Sharma > <ashu.coek88@gmail.com> wrote: > > > On Thu, Jun 4, 2026 at 9:14 AM shveta malik <shveta.malik@gmail.com> > > > wrote: > > > > My preference, and original intent, was to accept duplicate entries > > > > and skip them internally. Doc can be updated to say 'duplicate entries > > > > are skipped'. A server startup failure due to duplicate entries in a > > > > GUC does not seem right to me. If the alter-system command fails due > > > > to duplicate entries, that is still fine, but a startup failure seems > > > > excessive. But let's see what others have to say on this. > > > > > > > > > > Okay, the attached patch adds the capability to automatically remove > > > duplicate entries from the synchronized_standby_slots list. > > > > Thanks for updating the patch. > > > > I agree with Shveta that reporting an ERROR is not ideal. I also think it (ERROR) would > > be inconsistent with existing GUCs, as most of them, such as > > synchronous_standby_names, search_path, and session_preload_libraries, do not > > enforce uniqueness. > > > > The most similar GUC, synchronous_standby_names, also clarifies this in the > > documentation: > > > > " There is no mechanism to enforce uniqueness of standby names. In case of > > duplicates one of the matching standbys will be considered as higher priority, > > though exactly which one is indeterminate."[1] > > > > > In N of M > > > mode, if N > M after removing duplicate entries, an error is raised. > > > > I'm not entirely sure about this case. It seems similar to when the number of > > specified slots is less than N (in ANY N or FIRST N), given that we want to > skip > > duplicate slots. In that situation, the natural behavior to me would be to > > simply block replication rather than raise an error. And > > synchronous_standby_names would also simply block the transaction in this > case. > > > > For duplicate entries themselves, I agree with the direction of not > raising an error. Silently normalizing duplicates is reasonable for > this GUC, especially if we document it clearly. A repeated slot name > does not add any new information, so treating it as “same slot listed > twice by mistake” is practical. > > But for N > M after deduplication, I would still lean toward raising an error. > > Why I’d separate those cases: > > 1) Duplicate entries looks like a harmless normalization problem. ANY > 2 (a, a, b) can be normalized to ANY 2 (a, b) without changing the > user’s apparent intent much. > > 2) N > M after deduplication is not a transient runtime state. ANY 2 > (a, a) becomes one unique slot. That configuration can never succeed > unless the config itself changes. Blocking forever turns a static > configuration mistake into an operational liveness problem. > > 3) N > M after deduplication is different from ordinary “not enough > standbys are currently available”. If we configure ANY 2 (a, b) and > only a is currently caught up, blocking makes sense because the > situation may resolve at runtime. If we configure ANY 2 (a, a) and > duplicates are ignored, there is no possible future runtime in which > it succeeds without editing the GUC. That is why I think erroring is > better. > > On the synchronous_standby_names comparison, I do not think it is > fully analogous. The quoted documentation is about there being no > reliable way to enforce uniqueness of standby names in the live > system, because those names are matched against runtime standbys and > the result can be indeterminate. Here, synchronized_standby_slots > names concrete replication slots, which are stable object identifiers. > Duplicate config entries are detectable and normalizable > deterministically at GUC parse time. That gives us a cleaner option > than synchronous_standby_names has. Thanks for the explanation. What I was wondering is: ignoring duplicates, what should be the behavior of "ANY 2 (standby)" when N > M? I studied a bit for the behavior of synchronous_standby_names to understand the difference. synchronous_standby_names does support syntax like "ANY 2 (standby)" where N > M. Because even in that case, a transaction can still commit if there are two standbys with the same name ("standby" in this example). I'm not sure how common that use case is, but it may explain why no error is reported. Given that, I'm not opposed to reporting an error in synchronized_standby_slots when N > M. The situation is different here since there cannot be two slots with the same name, making this a completely invalid use case. Best Regards, Hou zj -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-06-05T03:36:32Z
On Fri, Jun 5, 2026 at 8:34 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote: > > On Thursday, June 4, 2026 5:27 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Thu, Jun 4, 2026 at 1:54 PM Zhijie Hou (Fujitsu) > > <houzj.fnst@fujitsu.com> wrote: > > > > > > On Thursday, June 4, 2026 3:36 PM Ashutosh Sharma > > <ashu.coek88@gmail.com> wrote: > > > > On Thu, Jun 4, 2026 at 9:14 AM shveta malik <shveta.malik@gmail.com> > > > > wrote: > > > > > My preference, and original intent, was to accept duplicate entries > > > > > and skip them internally. Doc can be updated to say 'duplicate entries > > > > > are skipped'. A server startup failure due to duplicate entries in a > > > > > GUC does not seem right to me. If the alter-system command fails due > > > > > to duplicate entries, that is still fine, but a startup failure seems > > > > > excessive. But let's see what others have to say on this. > > > > > > > > > > > > > Okay, the attached patch adds the capability to automatically remove > > > > duplicate entries from the synchronized_standby_slots list. > > > > > > Thanks for updating the patch. > > > > > > I agree with Shveta that reporting an ERROR is not ideal. I also think it (ERROR) would > > > be inconsistent with existing GUCs, as most of them, such as > > > synchronous_standby_names, search_path, and session_preload_libraries, do not > > > enforce uniqueness. > > > > > > The most similar GUC, synchronous_standby_names, also clarifies this in the > > > documentation: > > > > > > " There is no mechanism to enforce uniqueness of standby names. In case of > > > duplicates one of the matching standbys will be considered as higher priority, > > > though exactly which one is indeterminate."[1] > > > > > > > In N of M > > > > mode, if N > M after removing duplicate entries, an error is raised. > > > > > > I'm not entirely sure about this case. It seems similar to when the number of > > > specified slots is less than N (in ANY N or FIRST N), given that we want to > > skip > > > duplicate slots. In that situation, the natural behavior to me would be to > > > simply block replication rather than raise an error. And > > > synchronous_standby_names would also simply block the transaction in this > > case. > > > > > > > For duplicate entries themselves, I agree with the direction of not > > raising an error. Silently normalizing duplicates is reasonable for > > this GUC, especially if we document it clearly. A repeated slot name > > does not add any new information, so treating it as “same slot listed > > twice by mistake” is practical. > > > > But for N > M after deduplication, I would still lean toward raising an error. > > > > Why I’d separate those cases: > > > > 1) Duplicate entries looks like a harmless normalization problem. ANY > > 2 (a, a, b) can be normalized to ANY 2 (a, b) without changing the > > user’s apparent intent much. > > > > 2) N > M after deduplication is not a transient runtime state. ANY 2 > > (a, a) becomes one unique slot. That configuration can never succeed > > unless the config itself changes. Blocking forever turns a static > > configuration mistake into an operational liveness problem. > > > > 3) N > M after deduplication is different from ordinary “not enough > > standbys are currently available”. If we configure ANY 2 (a, b) and > > only a is currently caught up, blocking makes sense because the > > situation may resolve at runtime. If we configure ANY 2 (a, a) and > > duplicates are ignored, there is no possible future runtime in which > > it succeeds without editing the GUC. That is why I think erroring is > > better. > > > > On the synchronous_standby_names comparison, I do not think it is > > fully analogous. The quoted documentation is about there being no > > reliable way to enforce uniqueness of standby names in the live > > system, because those names are matched against runtime standbys and > > the result can be indeterminate. Here, synchronized_standby_slots > > names concrete replication slots, which are stable object identifiers. > > Duplicate config entries are detectable and normalizable > > deterministically at GUC parse time. That gives us a cleaner option > > than synchronous_standby_names has. > > Thanks for the explanation. > > What I was wondering is: ignoring duplicates, what should be the behavior of > "ANY 2 (standby)" when N > M? > > I studied a bit for the behavior of synchronous_standby_names to understand the > difference. synchronous_standby_names does support syntax like "ANY 2 (standby)" > where N > M. Because even in that case, a transaction can still commit if there > are two standbys with the same name ("standby" in this example). I'm not sure > how common that use case is, but it may explain why no error is reported. > > Given that, I'm not opposed to reporting an error in synchronized_standby_slots > when N > M. The situation is different here since there cannot be two slots with > the same name, making this a completely invalid use case. > I also think, we can report error when N>M. IIRC, we were also reporting earlier (without removing duplicates). Upon removing duplicates, we can follow the same behaviour instead of walsender being stuck indefinitely. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-06-05T10:02:41Z
On Thu, Jun 4, 2026 at 2:57 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > So my preferred behavior would be: > > 1) duplicate names: normalize, do not error > 2) after normalization, if num_sync > unique_slots: error immediately > Thanks for tha pathces. I have attached a patch (txt file) with a few trivial changes, take it if you find the changes acceptable. thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-08T06:08:48Z
Hi, On Fri, Jun 5, 2026 at 3:32 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Jun 4, 2026 at 2:57 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > > > So my preferred behavior would be: > > > > 1) duplicate names: normalize, do not error > > 2) after normalization, if num_sync > unique_slots: error immediately > > > > Thanks for tha pathces. I have attached a patch (txt file) with a few > trivial changes, take it if you find the changes acceptable. > Thanks, I will review it and share my feedback. -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-08T08:53:28Z
Hi, On Mon, Jun 8, 2026 at 11:38 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > On Fri, Jun 5, 2026 at 3:32 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > Thanks for tha pathces. I have attached a patch (txt file) with a few > > trivial changes, take it if you find the changes acceptable. > > > > Thanks, I will review it and share my feedback. > PFA the patches that incorporate the changes from the top-up patch shared by Shveta in [1]. These changes primarily consist of documentation updates, comment improvements, and indentation fixes at a few places. [1] - https://www.postgresql.org/message-id/CAJpy0uAdBxGpc4wtj-LcTGMNkVCYu4eMbDr27snEO_SrN2cV4A%40mail.gmail.com -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Amit Kapila <amit.kapila16@gmail.com> — 2026-06-08T09:51:44Z
On Fri, Jun 5, 2026 at 9:06 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Fri, Jun 5, 2026 at 8:34 AM Zhijie Hou (Fujitsu) > <houzj.fnst@fujitsu.com> wrote: > > > > On Thursday, June 4, 2026 5:27 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > On Thu, Jun 4, 2026 at 1:54 PM Zhijie Hou (Fujitsu) > > > <houzj.fnst@fujitsu.com> wrote: > > > > > > > > On Thursday, June 4, 2026 3:36 PM Ashutosh Sharma > > > <ashu.coek88@gmail.com> wrote: > > > > > On Thu, Jun 4, 2026 at 9:14 AM shveta malik <shveta.malik@gmail.com> > > > > > wrote: > > > > > > My preference, and original intent, was to accept duplicate entries > > > > > > and skip them internally. Doc can be updated to say 'duplicate entries > > > > > > are skipped'. A server startup failure due to duplicate entries in a > > > > > > GUC does not seem right to me. If the alter-system command fails due > > > > > > to duplicate entries, that is still fine, but a startup failure seems > > > > > > excessive. But let's see what others have to say on this. > > > > > > > > > > > > > > > > Okay, the attached patch adds the capability to automatically remove > > > > > duplicate entries from the synchronized_standby_slots list. > > > > > > > > Thanks for updating the patch. > > > > > > > > I agree with Shveta that reporting an ERROR is not ideal. I also think it (ERROR) would > > > > be inconsistent with existing GUCs, as most of them, such as > > > > synchronous_standby_names, search_path, and session_preload_libraries, do not > > > > enforce uniqueness. > > > > > > > > The most similar GUC, synchronous_standby_names, also clarifies this in the > > > > documentation: > > > > > > > > " There is no mechanism to enforce uniqueness of standby names. In case of > > > > duplicates one of the matching standbys will be considered as higher priority, > > > > though exactly which one is indeterminate."[1] > > > > > > > > > In N of M > > > > > mode, if N > M after removing duplicate entries, an error is raised. > > > > > > > > I'm not entirely sure about this case. It seems similar to when the number of > > > > specified slots is less than N (in ANY N or FIRST N), given that we want to > > > skip > > > > duplicate slots. In that situation, the natural behavior to me would be to > > > > simply block replication rather than raise an error. And > > > > synchronous_standby_names would also simply block the transaction in this > > > case. > > > > > > > > > > For duplicate entries themselves, I agree with the direction of not > > > raising an error. Silently normalizing duplicates is reasonable for > > > this GUC, especially if we document it clearly. A repeated slot name > > > does not add any new information, so treating it as “same slot listed > > > twice by mistake” is practical. > > > > > > But for N > M after deduplication, I would still lean toward raising an error. > > > > > > Why I’d separate those cases: > > > > > > 1) Duplicate entries looks like a harmless normalization problem. ANY > > > 2 (a, a, b) can be normalized to ANY 2 (a, b) without changing the > > > user’s apparent intent much. > > > > > > 2) N > M after deduplication is not a transient runtime state. ANY 2 > > > (a, a) becomes one unique slot. That configuration can never succeed > > > unless the config itself changes. Blocking forever turns a static > > > configuration mistake into an operational liveness problem. > > > > > > 3) N > M after deduplication is different from ordinary “not enough > > > standbys are currently available”. If we configure ANY 2 (a, b) and > > > only a is currently caught up, blocking makes sense because the > > > situation may resolve at runtime. If we configure ANY 2 (a, a) and > > > duplicates are ignored, there is no possible future runtime in which > > > it succeeds without editing the GUC. That is why I think erroring is > > > better. > > > > > > On the synchronous_standby_names comparison, I do not think it is > > > fully analogous. The quoted documentation is about there being no > > > reliable way to enforce uniqueness of standby names in the live > > > system, because those names are matched against runtime standbys and > > > the result can be indeterminate. Here, synchronized_standby_slots > > > names concrete replication slots, which are stable object identifiers. > > > Duplicate config entries are detectable and normalizable > > > deterministically at GUC parse time. That gives us a cleaner option > > > than synchronous_standby_names has. > > > > Thanks for the explanation. > > > > What I was wondering is: ignoring duplicates, what should be the behavior of > > "ANY 2 (standby)" when N > M? > > > > I studied a bit for the behavior of synchronous_standby_names to understand the > > difference. synchronous_standby_names does support syntax like "ANY 2 (standby)" > > where N > M. Because even in that case, a transaction can still commit if there > > are two standbys with the same name ("standby" in this example). I'm not sure > > how common that use case is, but it may explain why no error is reported. > > > > Given that, I'm not opposed to reporting an error in synchronized_standby_slots > > when N > M. The situation is different here since there cannot be two slots with > > the same name, making this a completely invalid use case. > > > > I also think, we can report error when N>M. > +1 for reporting an ERROR for this case. -- With Regards, Amit Kapila. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-06-10T06:45:52Z
Please find a few comments on June8 version fo patches: 1) patch001: SYNC_REP_DEFAULT: do we need to give one-line comment for this somewhere as unlike PRIORITY and QUORUM, it is not self-explanatory. patch002: 2) It is better to avoid mentioning it as 'synchronized standby slots'. We can make it as 'synchronized_standby_slots' in below comments: + /* Parse the synchronized standby slots configuration */ + * Report problem states for synchronized standby slots that prevented the 3) For these error-messages too, we need to mention GUC name to give better clarity. + GUC_check_errmsg("number of synchronized standby slots (%d) must not exceed the number of unique listed slots (%d)", + syncrep_parse_result->num_sync, + syncrep_parse_result->nmembers); How about: GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); GUC_check_errmsg("invalid value for parameter \"%s\: synchronization requirement (%d) exceeds the number of unique listed slots (%d)", "synchronized_standby_slots", syncrep_parse_result->num_sync, syncrep_parse_result->nmembers); Or GUC_check_errmsg("invalid value for parameter \"%s\: required number of synchronized standby slots (%d) exceeds the number of unique listed slots (%d)", "synchronized_standby_slots", syncrep_parse_result->num_sync, syncrep_parse_result->nmembers); 3) + GUC_check_errmsg("number of synchronized standby slots (%d) must be greater than zero", + syncrep_parse_result->num_sync) How about: GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); GUC_check_errmsg("invalid value for parameter \"%s\: required number of synchronized standby slots (%d) must be greater than zero", "synchronized_standby_slots", syncrep_parse_result->num_sync); --- Or, better yet, we can split the messages and details for both, example: GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); GUC_check_errmsg("invalid value for parameter \"%s\", "synchronized_standby_slots"); GUC_check_errdetail("The required number of synchronized standby slots (%d) exceeds the number of unique listed slots (%d)", syncrep_parse_result->num_sync, syncrep_parse_result->nmembers);) 4) +ReportUnavailableSyncStandbySlots(SyncStandbySlotsStateInfo * slot_states We can get rid of space before slot_states. I think pgindent might have put it in my patch. Sorry for the trouble. 5) 003: - wait_for_all = (required == synchronized_standby_slots_config->nslotnames); + wait_for_all = + (synchronized_standby_slots_config->syncrep_method == SYNC_REP_DEFAULT); This change can be moved to 002, right? thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-12T10:10:23Z
Hi Shveta, Thanks again for your review comments and suggestions. Please see my comments inline below: On Wed, Jun 10, 2026 at 12:16 PM shveta malik <shveta.malik@gmail.com> wrote: > > Please find a few comments on June8 version fo patches: > > 1) > patch001: > > SYNC_REP_DEFAULT: do we need to give one-line comment for this > somewhere as unlike PRIORITY and QUORUM, it is not self-explanatory. > Yes, it does makes sense to include a one-line comment. I've added it in the attached patch. > > patch002: > 2) > It is better to avoid mentioning it as 'synchronized standby slots'. > We can make it as 'synchronized_standby_slots' in below comments: > > + /* Parse the synchronized standby slots configuration */ > > + * Report problem states for synchronized standby slots that prevented the > Good point. I've made the suggested change in the attached patch > 3) > For these error-messages too, we need to mention GUC name to give > better clarity. > > + GUC_check_errmsg("number of synchronized standby slots (%d) must not > exceed the number of unique listed slots (%d)", > + syncrep_parse_result->num_sync, > + syncrep_parse_result->nmembers); > > > How about: > GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); > GUC_check_errmsg("invalid value for parameter \"%s\: synchronization > requirement (%d) exceeds the number of unique listed slots (%d)", > "synchronized_standby_slots", > syncrep_parse_result->num_sync, > syncrep_parse_result->nmembers); > Updated as suggested in the attached patch. > 3) > + GUC_check_errmsg("number of synchronized standby slots (%d) must be > greater than zero", > + syncrep_parse_result->num_sync) > > How about: > GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); > GUC_check_errmsg("invalid value for parameter \"%s\: required number > of synchronized standby slots (%d) must be greater than zero", > "synchronized_standby_slots", > syncrep_parse_result->num_sync); > > > Updated as suggested in the attached patch. > 4) > +ReportUnavailableSyncStandbySlots(SyncStandbySlotsStateInfo * slot_states > > We can get rid of space before slot_states. I think pgindent might > have put it in my patch. Sorry for the trouble. > > 5) > 003: > - wait_for_all = (required == synchronized_standby_slots_config->nslotnames); > + wait_for_all = > + (synchronized_standby_slots_config->syncrep_method == SYNC_REP_DEFAULT); > > This change can be moved to 002, right? Done. PFA patch containing all the above changes. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-06-12T13:03:32Z
On Fri, 12 Jun 2026 at 15:40, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi Shveta, > > Thanks again for your review comments and suggestions. Please see my > comments inline below: > > On Wed, Jun 10, 2026 at 12:16 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > Please find a few comments on June8 version fo patches: > > > > 1) > > patch001: > > > > SYNC_REP_DEFAULT: do we need to give one-line comment for this > > somewhere as unlike PRIORITY and QUORUM, it is not self-explanatory. > > > > Yes, it does makes sense to include a one-line comment. I've added it > in the attached patch. > > > > > patch002: > > 2) > > It is better to avoid mentioning it as 'synchronized standby slots'. > > We can make it as 'synchronized_standby_slots' in below comments: > > > > + /* Parse the synchronized standby slots configuration */ > > > > + * Report problem states for synchronized standby slots that prevented the > > > > Good point. I've made the suggested change in the attached patch > > > 3) > > For these error-messages too, we need to mention GUC name to give > > better clarity. > > > > + GUC_check_errmsg("number of synchronized standby slots (%d) must not > > exceed the number of unique listed slots (%d)", > > + syncrep_parse_result->num_sync, > > + syncrep_parse_result->nmembers); > > > > > > How about: > > GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); > > GUC_check_errmsg("invalid value for parameter \"%s\: synchronization > > requirement (%d) exceeds the number of unique listed slots (%d)", > > "synchronized_standby_slots", > > syncrep_parse_result->num_sync, > > syncrep_parse_result->nmembers); > > > > Updated as suggested in the attached patch. > > > 3) > > + GUC_check_errmsg("number of synchronized standby slots (%d) must be > > greater than zero", > > + syncrep_parse_result->num_sync) > > > > How about: > > GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); > > GUC_check_errmsg("invalid value for parameter \"%s\: required number > > of synchronized standby slots (%d) must be greater than zero", > > "synchronized_standby_slots", > > syncrep_parse_result->num_sync); > > > > > > > > Updated as suggested in the attached patch. > > > 4) > > +ReportUnavailableSyncStandbySlots(SyncStandbySlotsStateInfo * slot_states > > > > We can get rid of space before slot_states. I think pgindent might > > have put it in my patch. Sorry for the trouble. > > > > 5) > > 003: > > - wait_for_all = (required == synchronized_standby_slots_config->nslotnames); > > + wait_for_all = > > + (synchronized_standby_slots_config->syncrep_method == SYNC_REP_DEFAULT); > > > > This change can be moved to 002, right? > > Done. > > PFA patch containing all the above changes. > Hi Ashutosh, I have reviewed the patches. Here are some comments: 1. Should we update the doc for function "pg_logical_slot_get_changes". It says: ``` If the specified slot is a logical failover slot then the function will not return until all physical slots specified in <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link> have confirmed WAL receipt. ``` This line "return until all physical slots specified in" seems wrong after introduction of "FIRST/ANY" 2. Similarly, should we update the doc for function "pg_replication_slot_advance"? It also says: ``` If the specified slot is a logical failover slot then the function will not return until all physical slots specified in <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link> have confirmed WAL receipt. ``` 3. Comment in syncrep_gram.y states: * syncrep_gram.y - Parser for synchronous_standby_names Since synchronosed_standby_slots is reusing this, should we update this comment? 4. Similarly for syncrep_scanner.l, we have comment: * syncrep_scanner.l * a lexical scanner for synchronous_standby_names Should we update it? 5. enum "SyncStandbySlotsState" and stuct "SyncStandbySlotsStateInfo" should be mentioned in typedefs.list, so that pgindent works properly. Thanks, Shlok Kyal -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-15T10:39:26Z
Hi Shlok, Thanks for reviewing the patch and sharing your feedback - all your comments are well noted. Please find my responses below. On Fri, Jun 12, 2026 at 6:33 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote: > > Hi Ashutosh, > > I have reviewed the patches. Here are some comments: > > 1. Should we update the doc for function "pg_logical_slot_get_changes". It says: > ``` > If the specified slot is a logical failover slot then the function will > not return until all physical slots specified in > <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link> > have confirmed WAL receipt. > ``` > This line "return until all physical slots specified in" seems wrong > after introduction of "FIRST/ANY" > Agreed, it indeed needs correction, it has been addressed in the attached patch. > 2. Similarly, should we update the doc for function > "pg_replication_slot_advance"? It also says: > ``` > If the specified slot is a > logical failover slot then the function will not return until all > physical slots specified in > <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link> > have confirmed WAL receipt. > ``` Same as comment 1, this has been corrected in the attached patch as well > > 3. Comment in syncrep_gram.y states: > * syncrep_gram.y - Parser for synchronous_standby_names > > Since synchronosed_standby_slots is reusing this, should we update this comment? > Agreed, update was due. This has been taken care of in the attached patch. > 4. Similarly for syncrep_scanner.l, we have comment: > * syncrep_scanner.l > * a lexical scanner for synchronous_standby_names > > Should we update it? Agreed and updated in the attached patch. > > 5. enum "SyncStandbySlotsState" and stuct "SyncStandbySlotsStateInfo" should be > mentioned in typedefs.list, so that pgindent works properly. > These have been added to the typdefs.list PFA attached patches with all these changes. -- With Regards, Ashutosh Sharma
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-06-16T04:13:19Z
Thanks for the patches. 1) I see that there are 2 files now with 053*, we shall move ours to 054. ls t/053* t/053_standby_login_event_trigger.pl t/053_synchronized_standby_slots_quorum.pl 2) I feel this test can be removed: +# PART F: Plain list with first-prefixed slot name still means ALL mode It is not a test for synchronized_standby_slots, IMO, it is testing the syncrep parser. Or let me know if my understanding is wrong? thanks Shveta
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-06-16T06:39:47Z
On Mon, 15 Jun 2026 at 16:09, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi Shlok, > > Thanks for reviewing the patch and sharing your feedback - all your > comments are well noted. Please find my responses below. > > On Fri, Jun 12, 2026 at 6:33 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote: > > > > Hi Ashutosh, > > > > I have reviewed the patches. Here are some comments: > > > > 1. Should we update the doc for function "pg_logical_slot_get_changes". It says: > > ``` > > If the specified slot is a logical failover slot then the function will > > not return until all physical slots specified in > > <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link> > > have confirmed WAL receipt. > > ``` > > This line "return until all physical slots specified in" seems wrong > > after introduction of "FIRST/ANY" > > > > Agreed, it indeed needs correction, it has been addressed in the attached patch. > > > 2. Similarly, should we update the doc for function > > "pg_replication_slot_advance"? It also says: > > ``` > > If the specified slot is a > > logical failover slot then the function will not return until all > > physical slots specified in > > <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link> > > have confirmed WAL receipt. > > ``` > > Same as comment 1, this has been corrected in the attached patch as well > > > > > 3. Comment in syncrep_gram.y states: > > * syncrep_gram.y - Parser for synchronous_standby_names > > > > Since synchronosed_standby_slots is reusing this, should we update this comment? > > > > Agreed, update was due. This has been taken care of in the attached patch. > > > 4. Similarly for syncrep_scanner.l, we have comment: > > * syncrep_scanner.l > > * a lexical scanner for synchronous_standby_names > > > > Should we update it? > > Agreed and updated in the attached patch. > > > > > 5. enum "SyncStandbySlotsState" and stuct "SyncStandbySlotsStateInfo" should be > > mentioned in typedefs.list, so that pgindent works properly. > > > > These have been added to the typdefs.list > > PFA attached patches with all these changes. > Hi Ashutosh, I checked the CFbot and saw the test was failing [1]. It is failing for the test: not ok 4 - guc_privs 171 ms diff -U3 /__w/postgresql/postgresql/src/test/modules/unsafe_tests/expected/guc_privs.out /__w/postgresql/postgresql/build/testrun/unsafe_tests/regress/results/guc_privs.out --- /__w/postgresql/postgresql/src/test/modules/unsafe_tests/expected/guc_privs.out 2026-06-15 11:08:26.979460240 +0000 +++ /__w/postgresql/postgresql/build/testrun/unsafe_tests/regress/results/guc_privs.out 2026-06-15 11:20:01.829423596 +0000 @@ -590,8 +590,7 @@ -- Cannot set synchronized_standby_slots to an invalid slot name ALTER SYSTEM SET synchronized_standby_slots='invalid*'; ERROR: invalid value for parameter "synchronized_standby_slots": "invalid*" -DETAIL: replication slot name "invalid*" contains invalid character -HINT: Replication slot names may only contain lower case letters, numbers, and the underscore character. +DETAIL: syntax error at or near "*" -- Can set synchronized_standby_slots to a non-existent slot name ALTER SYSTEM SET synchronized_standby_slots='missing'; -- Reset the GUC In HEAD, validate_sync_standby_slots() called SplitIdentifierString() and then ReplicationSlotValidateNameInternal() for each element, which produced the slot-name-specific error for inputs such as 'invalid*'. With patch synchronized_standby_slots validation use the syncrep parser and a syntax error is reported instead. I think we should update the corresponding expected file. [1]: https://github.com/postgresql-cfbot/postgresql/actions/runs/27541996426/job/81406065089 Thanks, Shlok Kyal
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-16T10:42:43Z
Hi, On Tue, Jun 16, 2026 at 12:09 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote: > > I checked the CFbot and saw the test was failing [1]. > > It is failing for the test: > not ok 4 - guc_privs 171 ms > Thanks for reporting this. The attached patches address the issue and also incorporate the feedback from Shevta in [1]. [1] - https://www.postgresql.org/message-id/CAJpy0uCKaqD-UfCX%3DQ5WkdtxUS8ZCJwLduegi5Etdx7gkV-02g%40mail.gmail.com -- With Regards, Ashutosh Sharma.
-
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-06-18T05:44:34Z
On Tue, Jun 16, 2026 at 4:12 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi, > > Thanks for reporting this. The attached patches address the issue and > also incorporate the feedback from Shevta in [1]. > > [1] - https://www.postgresql.org/message-id/CAJpy0uCKaqD-UfCX%3DQ5WkdtxUS8ZCJwLduegi5Etdx7gkV-02g%40mail.gmail.com > Thanks Ashutosh. 1) + +# FIRST duplicates must also not create extra priority positions. +$primary->adjust_conf('postgresql.conf', 'synchronized_standby_slots', + "'FIRST 2 (sb1_slot, sb1_slot, sb2_slot)'"); +$primary->reload; I think either of ANY or FIRST is enough to cover the duplicate removal path. We can remove any one of these tests if you agree. 2) I tried running tests. Ran it 6 times. 3 times, it finished in 7-8 seconds, while in the rest of the runs, it took 25 seconds each. Please run this on your machine and let me know if the behaviour is same. In the log file of the run (and other similar runs), where execution time is longer, most of the time is spent here: primary.log: 2026-06-18 10:50:03.687 IST postmaster[49574] LOG: received SIGHUP, reloading configuration files 2026-06-18 10:50:03.688 IST postmaster[49574] LOG: parameter "synchronized_standby_slots" changed to "" 2026-06-18 10:50:21.514 IST walsender[49792] 054_synchronized_standby_slots_quorum.pl LOG: released physical replication slot "sb1_slot" 2026-06-18 10:50:21.522 IST client backend[49801] 054_synchronized_standby_slots_quorum.pl LOG: statement: SELECT pg_logical_slot_get_changes('logical_failover', NULL, NULL); Standby's log: 2026-06-18 10:50:02.470 IST client backend[49768] 054_synchronized_standby_slots_quorum.pl LOG: statement: WAIT FOR LSN '0/030003C8' WITH (MODE 'standby_replay', timeout '180s', no_throw); 2026-06-18 10:50:03.644 IST client backend[49790] 054_synchronized_standby_slots_quorum.pl LOG: statement: WAIT FOR LSN '0/03000448' WITH (MODE 'standby_replay', timeout '180s', no_throw); 2026-06-18 10:50:21.696 IST postmaster[49614] LOG: received fast shutdown request ~~ Notice the time jump from '10:50:02' to ' 10:50:21' thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-18T07:12:08Z
Hi Shveta, Thanks again for your review comments, please find my responses inline below: On Thu, Jun 18, 2026 at 11:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > 1) > + > +# FIRST duplicates must also not create extra priority positions. > +$primary->adjust_conf('postgresql.conf', 'synchronized_standby_slots', > + "'FIRST 2 (sb1_slot, sb1_slot, sb2_slot)'"); > +$primary->reload; > > I think either of ANY or FIRST is enough to cover the duplicate > removal path. We can remove any one of these tests if you agree. > It can be. From a code coverage perspective, I don't see any impact. However, FIRST and ANY represent different semantics. Although they share the same underlying logic for filtering duplicate entries, I felt it would still be beneficial to have a test case that explicitly demonstrates that duplicate removal works correctly with both options. That said, if you feel this makes the test file a bit heavier than necessary, I'm happy to remove it and keep the test suite lighter. > 2) > I tried running tests. Ran it 6 times. 3 times, it finished in 7-8 > seconds, while in the rest of the runs, it took 25 seconds each. > > Please run this on your machine and let me know if the behaviour is same. > > In the log file of the run (and other similar runs), where execution > time is longer, most of the time is spent here: > > primary.log: > 2026-06-18 10:50:03.687 IST postmaster[49574] LOG: received SIGHUP, > reloading configuration files > 2026-06-18 10:50:03.688 IST postmaster[49574] LOG: parameter > "synchronized_standby_slots" changed to "" > 2026-06-18 10:50:21.514 IST walsender[49792] > 054_synchronized_standby_slots_quorum.pl LOG: released physical > replication slot "sb1_slot" > 2026-06-18 10:50:21.522 IST client backend[49801] > 054_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > pg_logical_slot_get_changes('logical_failover', NULL, NULL); > > Standby's log: > 2026-06-18 10:50:02.470 IST client backend[49768] > 054_synchronized_standby_slots_quorum.pl LOG: statement: WAIT FOR LSN > '0/030003C8' WITH (MODE 'standby_replay', timeout '180s', no_throw); > 2026-06-18 10:50:03.644 IST client backend[49790] > 054_synchronized_standby_slots_quorum.pl LOG: statement: WAIT FOR LSN > '0/03000448' WITH (MODE 'standby_replay', timeout '180s', no_throw); > 2026-06-18 10:50:21.696 IST postmaster[49614] LOG: received fast > shutdown request > ~~ > > Notice the time jump from '10:50:02' to ' 10:50:21' > I ran the test case 7–8 times, and each execution completed in roughly 6–9 seconds on my machine. Please check the attached file with test results. -- With Regards, Ashutosh Sharma. -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
shveta malik <shveta.malik@gmail.com> — 2026-06-19T06:14:03Z
On Thu, Jun 18, 2026 at 12:42 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > Hi Shveta, > > Thanks again for your review comments, please find my responses inline below: > > On Thu, Jun 18, 2026 at 11:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > 1) > > + > > +# FIRST duplicates must also not create extra priority positions. > > +$primary->adjust_conf('postgresql.conf', 'synchronized_standby_slots', > > + "'FIRST 2 (sb1_slot, sb1_slot, sb2_slot)'"); > > +$primary->reload; > > > > I think either of ANY or FIRST is enough to cover the duplicate > > removal path. We can remove any one of these tests if you agree. > > > > It can be. From a code coverage perspective, I don't see any impact. > However, FIRST and ANY represent different semantics. Although they > share the same underlying logic for filtering duplicate entries, I > felt it would still be beneficial to have a test case that explicitly > demonstrates that duplicate removal works correctly with both options. > > That said, if you feel this makes the test file a bit heavier than > necessary, I'm happy to remove it and keep the test suite lighter. > > > 2) > > I tried running tests. Ran it 6 times. 3 times, it finished in 7-8 > > seconds, while in the rest of the runs, it took 25 seconds each. > > > > Please run this on your machine and let me know if the behaviour is same. > > > > In the log file of the run (and other similar runs), where execution > > time is longer, most of the time is spent here: > > > > primary.log: > > 2026-06-18 10:50:03.687 IST postmaster[49574] LOG: received SIGHUP, > > reloading configuration files > > 2026-06-18 10:50:03.688 IST postmaster[49574] LOG: parameter > > "synchronized_standby_slots" changed to "" > > 2026-06-18 10:50:21.514 IST walsender[49792] > > 054_synchronized_standby_slots_quorum.pl LOG: released physical > > replication slot "sb1_slot" > > 2026-06-18 10:50:21.522 IST client backend[49801] > > 054_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > > pg_logical_slot_get_changes('logical_failover', NULL, NULL); > > > > Standby's log: > > 2026-06-18 10:50:02.470 IST client backend[49768] > > 054_synchronized_standby_slots_quorum.pl LOG: statement: WAIT FOR LSN > > '0/030003C8' WITH (MODE 'standby_replay', timeout '180s', no_throw); > > 2026-06-18 10:50:03.644 IST client backend[49790] > > 054_synchronized_standby_slots_quorum.pl LOG: statement: WAIT FOR LSN > > '0/03000448' WITH (MODE 'standby_replay', timeout '180s', no_throw); > > 2026-06-18 10:50:21.696 IST postmaster[49614] LOG: received fast > > shutdown request > > ~~ > > > > Notice the time jump from '10:50:02' to ' 10:50:21' > > > > I ran the test case 7–8 times, and each execution completed in roughly > 6–9 seconds on my machine. Please check the attached file with test > results. > Okay, then it could be my slow VM issue. The patch is in good shape. I will pause my review here and resume when the committer (or others) starts participating. thanks Shveta -
Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication
Ashutosh Sharma <ashu.coek88@gmail.com> — 2026-06-29T09:13:00Z
On Fri, Jun 19, 2026 at 11:44 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Jun 18, 2026 at 12:42 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > > Hi Shveta, > > > > Thanks again for your review comments, please find my responses inline below: > > > > On Thu, Jun 18, 2026 at 11:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > 1) > > > + > > > +# FIRST duplicates must also not create extra priority positions. > > > +$primary->adjust_conf('postgresql.conf', 'synchronized_standby_slots', > > > + "'FIRST 2 (sb1_slot, sb1_slot, sb2_slot)'"); > > > +$primary->reload; > > > > > > I think either of ANY or FIRST is enough to cover the duplicate > > > removal path. We can remove any one of these tests if you agree. > > > > > > > It can be. From a code coverage perspective, I don't see any impact. > > However, FIRST and ANY represent different semantics. Although they > > share the same underlying logic for filtering duplicate entries, I > > felt it would still be beneficial to have a test case that explicitly > > demonstrates that duplicate removal works correctly with both options. > > > > That said, if you feel this makes the test file a bit heavier than > > necessary, I'm happy to remove it and keep the test suite lighter. > > > > > 2) > > > I tried running tests. Ran it 6 times. 3 times, it finished in 7-8 > > > seconds, while in the rest of the runs, it took 25 seconds each. > > > > > > Please run this on your machine and let me know if the behaviour is same. > > > > > > In the log file of the run (and other similar runs), where execution > > > time is longer, most of the time is spent here: > > > > > > primary.log: > > > 2026-06-18 10:50:03.687 IST postmaster[49574] LOG: received SIGHUP, > > > reloading configuration files > > > 2026-06-18 10:50:03.688 IST postmaster[49574] LOG: parameter > > > "synchronized_standby_slots" changed to "" > > > 2026-06-18 10:50:21.514 IST walsender[49792] > > > 054_synchronized_standby_slots_quorum.pl LOG: released physical > > > replication slot "sb1_slot" > > > 2026-06-18 10:50:21.522 IST client backend[49801] > > > 054_synchronized_standby_slots_quorum.pl LOG: statement: SELECT > > > pg_logical_slot_get_changes('logical_failover', NULL, NULL); > > > > > > Standby's log: > > > 2026-06-18 10:50:02.470 IST client backend[49768] > > > 054_synchronized_standby_slots_quorum.pl LOG: statement: WAIT FOR LSN > > > '0/030003C8' WITH (MODE 'standby_replay', timeout '180s', no_throw); > > > 2026-06-18 10:50:03.644 IST client backend[49790] > > > 054_synchronized_standby_slots_quorum.pl LOG: statement: WAIT FOR LSN > > > '0/03000448' WITH (MODE 'standby_replay', timeout '180s', no_throw); > > > 2026-06-18 10:50:21.696 IST postmaster[49614] LOG: received fast > > > shutdown request > > > ~~ > > > > > > Notice the time jump from '10:50:02' to ' 10:50:21' > > > > > > > I ran the test case 7–8 times, and each execution completed in roughly > > 6–9 seconds on my machine. Please check the attached file with test > > results. > > > > Okay, then it could be my slow VM issue. > > The patch is in good shape. I will pause my review here and resume > when the committer (or others) starts participating. > Sure, please feel free to provide your feedback or suggestions whenever you have them. -- With Regards, Ashutosh Sharma.