On Wed, Mar 18, 2026 at 10:15 AM Ashutosh Sharma <[email protected]> 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 <[email protected]> wrote:
> >
> > On Mon, Mar 16, 2026 at 2:33 PM Ashutosh Sharma <[email protected]>
> > 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
