I have just started looking into this patch. Please find below my first level of observation from the patch:
Thanks! Updated patch attached.
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));
Fixed.
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!
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.
By doing so, we would need to scan the WAL sender array more than once (or once if we can find N sync nodes with a name matching the first entry, smth unlikely to happen). We would need as well to recalculate for a given item in the list _names what is its priority and compare it with the existing entries in the WAL sender list. So this is not worth the shot. Also, using the priority instead of s_s_names is more solid as s_s_names is now used only in SyncRepGetStandbyPriority to calculate the priority for a given WAL sender, and is a function only called by a WAL sender itself when it initializes.
