On 05/12/17 21:07, Robert Haas wrote:
> On Mon, Dec 4, 2017 at 10:59 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> To me it looks like it's time to get this committed, marking as such.
>
> This version has noticeably more code rearrangement than before, and
> I'm not sure that is actually buying us anything. Why not keep the
> changes minimal?
>
Yeah we moved things around in the end, the main reason would be that it
actually works closer to how it was originally designed to work. Meaning
that the slow path is not so slow when !pq_is_send_pending() which seems
to have been the reasoning for original coding.
It's not completely necessary to do it for fixing the bug, but why make
things slower than they need to be.
> Also, TBH, this doesn't seem to have been carefully reviewed for style:
>
> - if (!pq_is_send_pending())
> - return;
> + /* Try taking fast path unless we get too close to walsender timeout. */
> + if (now < TimestampTzPlusMilliseconds(last_reply_timestamp,
> + wal_sender_timeout / 2))
> + {
> + if (!pq_is_send_pending())
> + return;
> + }
>
> Generally we write if (a && b) { ... } not if (a) { if (b) .. }
>
It's rather ugly with && because one of the conditions is two line, but
okay here you go. I am keeping the brackets even if normally don't for
one-liners because it's completely unreadable without them IMHO.
> - }
> + };
>
Oops.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services