| From: | Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> |
|---|---|
| To: | shveta malik <shveta(dot)malik(at)gmail(dot)com> |
| Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com>, SATYANARAYANA NARLAPURAM <satyanarlapuram(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication |
| Date: | 2026-06-12 10:10:23 |
| Message-ID: | CAE9k0P=uMVGV5TXTVdjKzu+0gFLMKHNY2usnAzK0L2cjbmPHBg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Shveta,
Thanks again for your review comments and suggestions. Please see my
comments inline below:
On Wed, Jun 10, 2026 at 12:16 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> Please find a few comments on June8 version fo patches:
>
> 1)
> patch001:
>
> SYNC_REP_DEFAULT: do we need to give one-line comment for this
> somewhere as unlike PRIORITY and QUORUM, it is not self-explanatory.
>
Yes, it does makes sense to include a one-line comment. I've added it
in the attached patch.
>
> patch002:
> 2)
> It is better to avoid mentioning it as 'synchronized standby slots'.
> We can make it as 'synchronized_standby_slots' in below comments:
>
> + /* Parse the synchronized standby slots configuration */
>
> + * Report problem states for synchronized standby slots that prevented the
>
Good point. I've made the suggested change in the attached patch
> 3)
> For these error-messages too, we need to mention GUC name to give
> better clarity.
>
> + GUC_check_errmsg("number of synchronized standby slots (%d) must not
> exceed the number of unique listed slots (%d)",
> + syncrep_parse_result->num_sync,
> + syncrep_parse_result->nmembers);
>
>
> How about:
> GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
> GUC_check_errmsg("invalid value for parameter \"%s\: synchronization
> requirement (%d) exceeds the number of unique listed slots (%d)",
> "synchronized_standby_slots",
> syncrep_parse_result->num_sync,
> syncrep_parse_result->nmembers);
>
Updated as suggested in the attached patch.
> 3)
> + GUC_check_errmsg("number of synchronized standby slots (%d) must be
> greater than zero",
> + syncrep_parse_result->num_sync)
>
> How about:
> GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
> GUC_check_errmsg("invalid value for parameter \"%s\: required number
> of synchronized standby slots (%d) must be greater than zero",
> "synchronized_standby_slots",
> syncrep_parse_result->num_sync);
>
>
>
Updated as suggested in the attached patch.
> 4)
> +ReportUnavailableSyncStandbySlots(SyncStandbySlotsStateInfo * slot_states
>
> We can get rid of space before slot_states. I think pgindent might
> have put it in my patch. Sorry for the trouble.
>
> 5)
> 003:
> - wait_for_all = (required == synchronized_standby_slots_config->nslotnames);
> + wait_for_all =
> + (synchronized_standby_slots_config->syncrep_method == SYNC_REP_DEFAULT);
>
> This change can be moved to 002, right?
Done.
PFA patch containing all the above changes.
--
With Regards,
Ashutosh Sharma.
| Attachment | Content-Type | Size |
|---|---|---|
| 0003-Add-FIRST-N-and-N-.-priority-syntax-to.patch | application/octet-stream | 23.1 KB |
| 0002-Add-ANY-N-semantics-to-synchronized_standby_slots.patch | application/octet-stream | 44.1 KB |
| 0001-Refactor-syncrep-parsing-to-represent-bare-standby-l.patch | application/octet-stream | 3.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Alvaro Herrera | 2026-06-12 10:25:29 | Re: REPACK CONCURRENTLY fails on tables with generated columns |
| Previous Message | Renaud Métrich | 2026-06-12 10:05:24 | [PATCH v1] Add ssl_alt_cert_file/ssl_alt_key_file for dual RSA+ECDSA certificate support |