On Mon, Mar 23, 2026 at 9:51 AM surya poondla <[email protected]> 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 <[email protected]> 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.
v20260323-0001-Add-FIRST-N-and-ANY-N-syntax-support-to-synchronized.patch
Description: Binary data
