| From: | Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> |
|---|---|
| To: | Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> |
| Cc: | shveta malik <shveta(dot)malik(at)gmail(dot)com>, 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-15 10:39:26 |
| Message-ID: | CAE9k0PkxvD_Rbz1qm3ZHOX_n-JXaOiPV2wusvb6KVwQqifpt6w@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Shlok,
Thanks for reviewing the patch and sharing your feedback - all your
comments are well noted. Please find my responses below.
On Fri, Jun 12, 2026 at 6:33 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Ashutosh,
>
> I have reviewed the patches. Here are some comments:
>
> 1. Should we update the doc for function "pg_logical_slot_get_changes". It says:
> ```
> If the specified slot is a logical failover slot then the function will
> not return until all physical slots specified in
> <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link>
> have confirmed WAL receipt.
> ```
> This line "return until all physical slots specified in" seems wrong
> after introduction of "FIRST/ANY"
>
Agreed, it indeed needs correction, it has been addressed in the attached patch.
> 2. Similarly, should we update the doc for function
> "pg_replication_slot_advance"? It also says:
> ```
> If the specified slot is a
> logical failover slot then the function will not return until all
> physical slots specified in
> <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link>
> have confirmed WAL receipt.
> ```
Same as comment 1, this has been corrected in the attached patch as well
>
> 3. Comment in syncrep_gram.y states:
> * syncrep_gram.y - Parser for synchronous_standby_names
>
> Since synchronosed_standby_slots is reusing this, should we update this comment?
>
Agreed, update was due. This has been taken care of in the attached patch.
> 4. Similarly for syncrep_scanner.l, we have comment:
> * syncrep_scanner.l
> * a lexical scanner for synchronous_standby_names
>
> Should we update it?
Agreed and updated in the attached patch.
>
> 5. enum "SyncStandbySlotsState" and stuct "SyncStandbySlotsStateInfo" should be
> mentioned in typedefs.list, so that pgindent works properly.
>
These have been added to the typdefs.list
PFA attached patches with all these changes.
--
With Regards,
Ashutosh Sharma
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-Refactor-syncrep-parsing-to-represent-bare-standby-l.patch | application/octet-stream | 3.2 KB |
| 0002-Add-ANY-N-semantics-to-synchronized_standby_slots.patch | application/octet-stream | 49.6 KB |
| 0003-Add-FIRST-N-and-N-.-priority-syntax-to.patch | application/octet-stream | 23.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ashutosh Bapat | 2026-06-15 10:46:50 | Re: [PATCH] proposal to surface index used by replica identity |
| Previous Message | Heikki Linnakangas | 2026-06-15 10:28:43 | Re: Fix warning: ‘startpos’ may be used uninitialized in function ‘results_differ’ |