Re: Review of Refactoring code for sync node detection

Thanks for the comments!

On Sun, Nov 16, 2014 at 8:32 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 31 October 2014 00:27, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Fri, Oct 31, 2014 at 6:59 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>>>
>>> If we stick with this version I'd argue it makes more sense to just stick
>>> the sync_node = and priority = statements into the if block and ditch the
>>> continue. </nitpick>
>>
>> Let's go with the cleaner version then, I'd prefer code that can be read
>> easily for this code path. Switching back is not much complicated either.
>
> I like where you are headed with this feature set. I will do my best
> to comment and review so we get something in 9.5.
>
> Some review comments
>
> * The new function calls them a Node rather than a Standby. That is a
> good change, but it needs to be applied consistently, so we no longer
> use the word Standby anywhere near the sync rep code. I'd prefer if we
> continue to use the abbreviation sync for synchronous, since its less
> typing and avoids spelling mistakes in code and comments.
Yes I guess this makes sense.

> * Rationale...
> Robert Haas wrote
>> Interestingly, the syncrep code has in some of its code paths the idea
>> that a synchronous node is unique, while other code paths assume that
>> there can be multiple synchronous nodes. If that's fine I think that
>> it would be better to just make the code multiple-sync node aware, by
>> having a single function call in walsender.c and syncrep.c that
>> returns an integer array of WAL sender positions (WalSndCtl). as that
>> seems more extensible long-term.
>
> I thought this was the rationale for the patch, but it doesn't do
> this. If you disagree with Robert, it would be best to say so now,
> since I would guess he's expecting the patch to be doing as he asked.
> Or perhaps I misunderstand.

This comment is originally from me :)
http://www.postgresql.org/message-id/CAB7nPqTtmSo0YX1_3_PykpKbdGUi3UeFd0_=2-6VRQo5N_TB+A@mail.gmail.com
Changing with my older opinion, I wrote the patch on this thread with
in mind the idea to keep the code as simple as possible, so for now it
is better to still think that we have a single sync node. Let's work
the multiple node thing once we have a better spec of how to do it,
visibly using a dedicated micro-language within s_s_names.

> * Multiple sync standbys were originally avoided because of the code
> cost and complexity at ReleaseWaiters(). I'm very keen to keep the
> code at that point very robust, so simplicity is essential. It would
> also be useful to have some kind of micro-benchmark that allows us to
> understand the overhead of various configs. Jim mentions he isn't sure
> of the performance there. I don't see too much a problem with this
> patch, but the later patches will require this, so we may as well do
> this soon.
Yes I have been playing with that with the patch series of the
previous commit fest, with something not that much kludgy.

> * We probably don't need a comment like /* Cleanup */ inserted above a
> pfree. ;-)
Sure. I'll send an updated patch tomorrow.
Regards,
--
Michael