Re: [HACKERS] Walsender timeouts and large transactions

Ouch.. I'd doubly mistaked.

> I found that the patch is the latest one and will look this
> soon. Sorry for the ignorance.

Thats...wrong. Sorry. There's no new patch since the Reboer's
comment.

I think this is just a bug fix and needs no more argument on its
functionality.  (and might ought to be backpatched?)

At Fri, 3 Nov 2017 15:54:09 +0100, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote in <e2939d26-f5cb-6581-0ca3-a1b0556ed729@2ndquadrant.com>
> > This change falsifies the comments.  Maybe initialize now just after
> > resetSetringInfo() is done.
> 
> Eh, right, I can do that.

It is reasonable. (Or rewrite the comment?)

At Fri, 3 Nov 2017 15:54:09 +0100, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote in <e2939d26-f5cb-6581-0ca3-a1b0556ed729@2ndquadrant.com>
> > 
> > -    /* fast path */
> > -    /* Try to flush pending output to the client */
> > -    if (pq_flush_if_writable() != 0)
> > -        WalSndShutdown();
> > +    /* Try taking fast path unless we get too close to walsender timeout. */
> > +    if (now < TimestampTzPlusMilliseconds(last_reply_timestamp,
> > +                                          wal_sender_timeout / 2))
> > +    {
> > +        CHECK_FOR_INTERRUPTS();
> > 
> > -    if (!pq_is_send_pending())
> > -        return;
> > +        /* Try to flush pending output to the client */
> > +        if (pq_flush_if_writable() != 0)
> > +            WalSndShutdown();
> > +
> > +        if (!pq_is_send_pending())
> > +            return;
> > +    }
> > 
> > I think it's only the if (!pq_is_send_pending()) return; that needs to
> > be conditional here, isn't it?  The pq_flush_if_writable() can be done
> > unconditionally.
> > 
> 
> Well, even the CHECK_FOR_INTERRUPTS() can be called unconditionally yes.
> It just seems like it's needless call as we'll call both in for loop
> anyway if we take the "slow" path. I admit it's not exactly big win
> though. If you think it would improve readability I can move it.

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.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center