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