Hi, Thanks for the review.
On Wed, Mar 11, 2026 at 2:15 PM shveta malik <[email protected]> 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.
