On Thu, Mar 12, 2026 at 6:07 PM Ashutosh Sharma <[email protected]> wrote:
>
> Hi Ajin,
>
> On Thu, Mar 12, 2026 at 4:47 PM Ajin Cherian <[email protected]> wrote:
> >
> > On Sun, Mar 8, 2026 at 4:16 AM Ashutosh Sharma <[email protected]>
> > wrote:
> > >
> > > Hi All,
> > >
> > > Submitting a new version of the patch based on Satya's earlier work - [1].
> > >
> > > Please take a look and let us know your thoughts.
> > >
> > > [1] -
> > > https://www.postgresql.org/message-id/CAHg%2BQDfU7rOebrLDESPpHSgdiadKbpCOmBokcbmM6Gr%2BA5VobQ%40mail.gmail.com
> > >
> >
> > Hi Ashutosh,
> >
> > I was testing this patch and it seems, if the name of the slots starts
> > with first, say firstslot or firstsub, then the patch treats it as
> > FIRST 1 priority mode.
> >
Good catch.
>
> Thanks for identifying and reporting this - confirmed, it is indeed an
> issue. The attached patch addresses it and also adds a regression test
> case for the same.
>
> Additionally, it also fixes the issue related to log message reporting
> for unavailable/invalid slots that Shveta raised yesterday.
>
> Please take a look and feel free to share any further comments.
>
Thank You Ashutosh. A few comments:
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?
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++)
3)
+ /*Record Slot State */
Space needed before Record.
4)
+ SS_SLOT_OK, /* slot is valid and can participate */
Do we need this value, we are not using it anywhere.
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?
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)
thanks
Shveta
