Re: Support for N synchronous standby servers

On 09 August 2014 11:33, Michael Paquier Wrote:

> Please find attached a patch to add support of synchronous replication
> for multiple standby servers. This is controlled by the addition of a
> new GUC parameter called synchronous_standby_num, that makes server
> wait for transaction commit on the first N standbys defined in
> synchronous_standby_names. The implementation is really straight-
> forward, and has just needed a couple of modifications in walsender.c
> for pg_stat_get_wal_senders and syncrep.c.

I have just started looking into this patch.
Please find below my first level of observation from the patch:

1. Allocation of memory for sync_nodes in function SyncRepGetSynchronousNodes should be equivalent to allowed_sync_nodes instead of max_wal_senders. As anyway we are not going to store sync stdbys more than allowed_sync_nodes.
sync_nodes = (int *) palloc(allowed_sync_nodes * sizeof(int));

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.

3. Can we optimize the function SyncRepGetSynchronousNodes in such a way that it gets the number of standby nodes from s_s_names itself. With this it will be usful to stop scanning the moment we get first s_s_num potential standbys.

Thanks and Regards,
Kumar Rajeev Rastogi