Re: Support for N synchronous standby servers

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 priority calculation 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 loop itself, we can calculate the priority as well as assigment of new standbys with lower priority.

Let me know if you see any issue with this.

2. Comment inside the function SyncRepReleaseWaiters,
/*
* We should have found ourselves at least, except if it is not expected
* to find any synchronous nodes.
*/
Assert(num_sync > 0);

I think "except if it is not expected to find any synchronous nodes" 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 without
any knowledge of user.
I see that some discussion has happened regarding this but I think just adding documentation for this is not enough.

I am not sure what issue is observed in adding check during GUC initialization but if there is unavoidable issue during GUC initialization
then 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 and it is acceptable
to have s_s_num more than s_s_names. But I was thinking should not give atleast some notice message to user 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.

Can we make this as
When this parameter is set to <literal>0</>, all the standby
nodes will be considered as asynchronous irrespective of value of synchronous_standby_names.

7. Are considered as synchronous the first elements of
<xref linkend="guc-synchronous-standby-names"> in number of
<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 the first synchronous-standby-num standby fails.

Let me know incase if something is not clear.

Thanks and Regards,
Kumar Rajeev Rastogi.