Re: Support for N synchronous standby servers - take 2 - Mailing list pgsql-hackers
| From | Amit Kapila |
|---|---|
| Subject | Re: Support for N synchronous standby servers - take 2 |
| Date | |
| Msg-id | CAA4eK1+bpAmxNG_TDu1aydXbW38cKcOdpJysd0213EPrBXGMfA@mail.gmail.com Whole thread Raw |
| In response to | Re: Support for N synchronous standby servers - take 2 (Fujii Masao <masao.fujii@gmail.com>) |
| Responses |
Re: Support for N synchronous standby servers - take 2
(Fujii Masao <masao.fujii@gmail.com>)
|
| List | pgsql-hackers |
On Tue, Apr 5, 2016 at 3:15 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Tue, Apr 5, 2016 at 4:31 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Mon, Apr 4, 2016 at 1:58 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >>
> >>
> >> Thanks for updating the patch!
> >>
> >> I applied the following changes to the patch.
> >> Attached is the revised version of the patch.
> >>
> >
> > 1.
> > {
> > {"synchronous_standby_names", PGC_SIGHUP, REPLICATION_MASTER,
> > gettext_noop("List of names of potential synchronous standbys."),
> > NULL,
> > GUC_LIST_INPUT
> > },
> > &SyncRepStandbyNames,
> > "",
> > check_synchronous_standby_names, NULL, NULL
> > },
> >
> > Isn't it better to modify the description of synchronous_standby_names in
> > guc.c based on new usage?
>
> What about "Number of synchronous standbys and list of names of
> potential synchronous ones"? Better idea?
>
>
> On Tue, Apr 5, 2016 at 4:31 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Mon, Apr 4, 2016 at 1:58 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >>
> >>
> >> Thanks for updating the patch!
> >>
> >> I applied the following changes to the patch.
> >> Attached is the revised version of the patch.
> >>
> >
> > 1.
> > {
> > {"synchronous_standby_names", PGC_SIGHUP, REPLICATION_MASTER,
> > gettext_noop("List of names of potential synchronous standbys."),
> > NULL,
> > GUC_LIST_INPUT
> > },
> > &SyncRepStandbyNames,
> > "",
> > check_synchronous_standby_names, NULL, NULL
> > },
> >
> > Isn't it better to modify the description of synchronous_standby_names in
> > guc.c based on new usage?
>
> What about "Number of synchronous standbys and list of names of
> potential synchronous ones"? Better idea?
>
Looks good.
>
> > 2.
> > pg_stat_get_wal_senders()
> > {
> > ..
> > /*
> > ! * Allocate and update the config data of synchronous replication,
> > ! * and then get the currently active synchronous standbys.
> > */
> > + SyncRepUpdateConfig();
> > LWLockAcquire(SyncRepLock, LW_SHARED);
> > ! sync_standbys = SyncRepGetSyncStandbys();
> > LWLockRelease(SyncRepLock);
> > ..
> > }
> >
> > Why is it important to update the config with patch? Earlier also any
> > update to config between calls wouldn't have been visible.
>
> Because a backend has no chance to call SyncRepUpdateConfig() and
> parse the latest value of s_s_names if SyncRepUpdateConfig() is not
> called here. This means that pg_stat_replication may return the information
> based on the old value of s_s_names.
>
> > 3.
> > <title>Planning for High Availability</title>
> >
> > <para>
> > ! <varname>synchronous_standby_names</> specifies the number of
> > ! synchronous standbys that transaction commits made when
> >
> > Is it better to say like: <varname>synchronous_standby_names</> specifies
> > the number and names of
>
> Precisely s_s_names specifies a list of names of potential sync standbys
> not sync ones.
>
> > 4.
> > + /*
> > + * Return the list of sync standbys, or NIL if no sync standby is
> > connected.
> > + *
> > + * If there are multiple standbys with the same priority,
> > + * the first one found is considered as higher priority.
> >
> > Here line indentation of second line can be improved.
>
> What about "the first one found is selected first"? Or better idea?
>
>
> > 2.
> > pg_stat_get_wal_senders()
> > {
> > ..
> > /*
> > ! * Allocate and update the config data of synchronous replication,
> > ! * and then get the currently active synchronous standbys.
> > */
> > + SyncRepUpdateConfig();
> > LWLockAcquire(SyncRepLock, LW_SHARED);
> > ! sync_standbys = SyncRepGetSyncStandbys();
> > LWLockRelease(SyncRepLock);
> > ..
> > }
> >
> > Why is it important to update the config with patch? Earlier also any
> > update to config between calls wouldn't have been visible.
>
> Because a backend has no chance to call SyncRepUpdateConfig() and
> parse the latest value of s_s_names if SyncRepUpdateConfig() is not
> called here. This means that pg_stat_replication may return the information
> based on the old value of s_s_names.
>
Thats right, but without this patch also won't pg_stat_replication can show old information? If no, why so?
> > 3.
> > <title>Planning for High Availability</title>
> >
> > <para>
> > ! <varname>synchronous_standby_names</> specifies the number of
> > ! synchronous standbys that transaction commits made when
> >
> > Is it better to say like: <varname>synchronous_standby_names</> specifies
> > the number and names of
>
> Precisely s_s_names specifies a list of names of potential sync standbys
> not sync ones.
>
Okay, but you doesn't seem to have updated this in your latest patch.
> > 4.
> > + /*
> > + * Return the list of sync standbys, or NIL if no sync standby is
> > connected.
> > + *
> > + * If there are multiple standbys with the same priority,
> > + * the first one found is considered as higher priority.
> >
> > Here line indentation of second line can be improved.
>
> What about "the first one found is selected first"? Or better idea?
>
What I was complaining about that few words from second line can be moved to previous line, but may be pgindent will take care of same, so no need to worry.
pgsql-hackers by date: