On Mon, Apr 4, 2016 at 1:58 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
> Thanks for updating the patch!
> I applied the following changes to the patch.
> Attached is the revised version of the patch.

{"synchronous_standby_names", PGC_SIGHUP, REPLICATION_MASTER,
gettext_noop("List of names of potential synchronous standbys."),
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?

! * 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();

Why is it important to update the config with patch?  Earlier also any
update to config between calls wouldn't have been visible.

      <title>Planning for High Availability</title>

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

+ /*
+  * Return the list of sync standbys, or NIL if no sync standby is
+  *
+  * 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.

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

Reply via email to