Re: Support for N synchronous standby servers - take 2

On Tue, Apr 5, 2016 at 3:15 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Tue, Apr 5, 2016 at 4:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Mon, Apr 4, 2016 at 1:58 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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.
>

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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com