Re: Support for N synchronous standby servers - take 2

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?

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.

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

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.

5.
! /*
! * 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

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