Hi, Thanks for the review, please find comments inline:
On Fri, Mar 13, 2026 at 9:53 AM shveta malik <[email protected]> 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.
v20260313-0001-Add-FIRST-N-and-ANY-N-syntax-support-to-synchronized.patch
Description: Binary data
