Re: Review of Refactoring code for sync node detection

On 11/18/2014 11:23 PM, Michael Paquier wrote:
> On Tue, Nov 18, 2014 at 6:33 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>
>> Can we just wait on this patch until we have the whole feature?
>>
> Well, this may take some time to even define, and even if goals are clearly
> defined this may take even more time to have a prototype to discuss about.
>
>
>> We quite often break larger patches down into smaller ones, but if
>> they arrive in lots of small pieces its more difficult to understand
>> and correct issues that are happening on the larger scale. Churning
>> the same piece of code multiple times is rather mind numbing.
>>
> Hm. I think that we are losing ourselves in this thread. The primary goal
> is to remove a code duplication between syncrep.c and walsender,c that
> exists since 9.1. Would it be possible to focus only on that for now?
> That's still the topic of this CF.

Some comments on this patch:

* I don't like filling the priorities-array in
SyncRepGetSynchronousStandby(). It might be convenient for the one
caller that needs it, but it seems pretty ad hoc.

In fact, I don't really see the point of having the array at all. You
could just print the priority in the main loop in
pg_stat_get_wal_senders(). Yeah, nominally you need to hold SyncRepLock
while reading the priority, but it seems over-zealous to be so strict
about that in pg_stat_wal_senders(), since it's just an informational
view, and priority changes so rarely that it's highly unlikely to hit a
race condition there. Also note that when you change the list of
synchronous standbys in the config file, and SIGHUP, the walsender
processes will update their priorities in random order. So the idea that
you get a consistent snapshot of the priorities is a bit bogus anyway.
Also note that between filling the priorities array and the main loop in
pg_stat_get_wal_senders(), a new WAL sender might connect and set a
slot's pid. With the current coding, you'll print an uninitialized value
from the array if that happens. So the only thing that holding the
SyncRepLock really protects from is a "torn" read of the value, if
reading an int is not atomic. We could use the spinlock to protect from
that if we care.

* Would be nicer to return a pointer, than index into the wal senders
array. That's what the caller really wants.

I propose the attached (I admit I haven't tested it).

- Heikki