Original message (sorry, don't have a copy to reply to :( ):
http://www.postgresql.org/message-id/CAB7nPqThX=WvuGA1J-_CQ293dK3FmUivuYkNvHR0W5xjEc=oFQ@mail.gmail.com
Commitfest URL: https://commitfest.postgresql.org/action/patch_view?id=1579
Summary:
I'd like someone to chime in on the potential performance impact. Other than that, looks good modulo a few grammar
fixes.
Patch applies cleanly and passes make check.
Searching for both SyncRepLock and sync_standby_priority I find no other code this patch needs to modify.
SyncRepGetSynchronousNode():
IMHO, the top comment should mention that if there are multiple standbys of the same priority it returns the first
one.
This switches from using a single if() with multiple conditions &&'d together to a bunch of if() continue's. I don't
knowif those will perform the same, and AFAIK this is pretty performance critical.
<grammarZealot>In the comments, "Process" should be "Proceed".</grammarZealot>
The original search logic in SyncRepReleaseWaiters is correctly copied and negated.
pg_stat_get_wal_senders():
The original version only loops through walsnds[] one time; the new code loops twice, both times while holding
SyncRepLock(shared).This will block transaction commit if syncrep is enabled. That's not great, but presumably this
functionisn't going to be called terribly often, so maybe it's OK. (On a side note, perhaps we should add a warning to
thedocumentation for pg_stat_replication warning not to hammer it...)
<grammar>
+ /* Get first the priorities on each standby as long as we hold a lock */
should be
+ /* First get the priority of each standby as long as we hold a lock */
or even just
+ /* First get the priority of each standby */
--
Jim C. Nasby, Data Architect jim@nasby.net
512.569.9461 (cell) http://jim.nasby.net
