On 29 November 2017 at 23:59, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote:
> Hi, > > On 17/11/17 08:35, Kyotaro HORIGUCHI wrote: > > > > Moving around the code allow us to place ps_is_send_pending() in > > the while condition, which seems to be more proper place to do > > that. I haven't added test for this particular case. > > > > I tested this that > > > > - cleanly applies on the current master HEAD and passes make > > check and subscription test. > > > > - walsender properly chooses the slow-path even if > > pq_is_send_pending() is always false. (happens on a fast enough > > network) > > > > - walsender waits properly waits on socket and process-reply time > > in WaitLatchOrSocket. > > > > - walsender exits by timeout on network stall. > > > > So, I think the patch is functionally perfect. > > > > I'm a reviewer of this patch but I think I'm not allowed to mark > > this "Ready for Commiter" since the last change is made by me. > > > > Thanks for working on this, but there are couple of problems with your > modifications which mean that it does not actually fix the original > issue anymore (transaction taking long time to decode while sending > changes over network works fine will result in walsender timout). > > The firs one is that you put pq_is_send_pending() in the while so the > while is again never executed if there is no network send pending which > makes the if above meaningless. Also you missed ProcessRepliesIfAny() > when moving code around. That's needed for timeout calculations to work > correctly. > > So one more revision attached with those things fixed. This version > fixes the original issue as well. > > I'm happy with what I see here. Commit message needs tweaking, but any committer would do that anyway. To me it looks like it's time to get this committed, marking as such. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
