Hi, Ashutosh
On Mon, 23 Mar 2026 at 16:28, Ashutosh Sharma <[email protected]> wrote: > 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. 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.
