Re: Support for N synchronous standby servers - take 2

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?

> 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?

>
> ! /*
> ! * syncrep_yyparse sets the global syncrep_parse_result as side effect.
> ! * But this function is required to just check, so frees it
> ! * once parsing parameter.
> ! */
> ! SyncRepFreeConfig(syncrep_parse_result);
>
> How about below change in comment?
> /so frees it once parsing parameter/so frees it after parsing the parameter

Will apply this to the patch.

Thanks for the review!

Regards,

--
Fujii Masao