Re: Support for N synchronous standby servers - Mailing list pgsql-hackers

On 23 August 2014 11:22, Michael Paquier Wrote:

> >> 2. Logic of deciding the highest priority one seems to be in-correct.
> >>         Assume, s_s_num = 3, s_s_names = 3,4,2,1
> >>         standby nodes are in order as: 1,2,3,4,5,6,7
> >>
> >>         As per the logic in patch, node 4 with priority 2 will not
> be added in the list whereas 1,2,3 will be added.
> >>
> >>         The problem is because priority updated for next tracking is
> not the highest priority as of that iteration, it is just priority of
> last node added to the list. So it may happen that a node with
> higher priority is still there in list but we are comparing with some
> other smaller priority.
> >
> >
> > Fixed. Nice catch!
>
>
> Actually by re-reading the code I wrote yesterday I found that the fix
> in v6 for that is not correct. That's really fixed with v7 attached.

I have done some more review, below are my comments:

1. There are currently two loops on *num_sync, Can we simply the function SyncRepGetSynchronousNodes by moving the
prioritycalculation inside the upper loop    if (*num_sync == allowed_sync_nodes)    {        for (j = 0; j <
*num_sync;j++)        {Anyway we require priority only if *num_sync == allowed_sync_nodes condition matches.So in this
loopitself, we can calculate the priority as well as assigment of new standbys with lower priority.Let me know if you
seeany issue with this.     
2.    Comment inside the function SyncRepReleaseWaiters,/* * We should have found ourselves at least, except if it is
notexpected * to find any synchronous nodes. */Assert(num_sync > 0);I think "except if it is not expected to find any
synchronousnodes" is not required. As if it has come till this point means atleast this node is synchronous. 
3.     Document says that s_s_num should be lesser than max_wal_senders but code wise there is no protection for the
same.IMHO,s_s_num should be lesser than or equal to max_wal_senders otherwise COMMIT will never return back the console
withoutanyknowledge of user.I see that some discussion has happened regarding this but I think just adding
documentationfor this is not enough.I am not sure what issue is observed in adding check during GUC initialization but
ifthere is unavoidable issue during GUC initializationthen can't we try to add check at later points. 
4.  Similary interaction between parameters s_s_names and s_s_num. I see some discussion has happened regarding this
andit is acceptableto have s_s_num more than s_s_names. But I was thinking should not give atleast some notice message
touser for such case along with some documentation. 
config.sgml
5. "At any one time there will be at a number of active synchronous standbys": this sentence is not proper.

6.     When this parameter is set to <literal>0</>, all the standby       nodes will be considered as asynchronous.
Canwe make this as When this parameter is set to <literal>0</>, all the standby       nodes will be considered as
asynchronousirrespective of value of synchronous_standby_names.         
7.     Are considered as synchronous the first elements of       <xref linkend="guc-synchronous-standby-names"> in
numberof       <xref linkend="guc-synchronous-standby-num"> that are       connected. 
Starting of this sentence looks to be incomplete.
high-availability.sgml
8.  Standbys listed after this will take over the role   of synchronous standby if the first one should fail.
    Should not we make it as:    Standbys listed after this will take over the role   of synchronous standby if any of
thefirst synchronous-standby-num standby fails. 

Let me know incase if something is not clear.

Thanks and Regards,
Kumar Rajeev Rastogi.