Re: Review of Refactoring code for sync node detection - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: Review of Refactoring code for sync node detection
Date
Msg-id CAB7nPqQCK8HS8xd-Mx2ku9Y+=acz3Z4g-OWa4E-V3HtS+dSd8Q@mail.gmail.com
Whole thread Raw
In response to Re: Review of Refactoring code for sync node detection  (Heikki Linnakangas <hlinnakangas@vmware.com>)
Responses Re: Review of Refactoring code for sync node detection  (Heikki Linnakangas <hlinnakangas@vmware.com>)
List pgsql-hackers


On Thu, Dec 11, 2014 at 10:07 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
* 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.

That's fair, and it simplifies the whole process as well as the API able to fetch the synchronous WAL sender.
 
* 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).

Actually if you do it this way I think that it would be worth adding the small optimization Fujii-san mentioned upthread: if priority is equal to 1, we leave the loop earlier and return immediately the pointer. All those things gathered give the patch attached, that I actually tested FWIW with multiple standbys and multiple entries in s_s_names.

Regards,
--
Michael
Attachment

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: [Bug] Inconsistent result for inheritance and FOR UPDATE.
Next
From: Etsuro Fujita
Date:
Subject: Re: [Bug] Inconsistent result for inheritance and FOR UPDATE.