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(dot)jelinek(at)2ndquadrant(dot)com> wrote in <e2939d26-f5cb-6581-0ca3-a1b0556ed729(at)2ndquadrant(dot)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(dot)jelinek(at)2ndquadrant(dot)com> wrote in <e2939d26-f5cb-6581-0ca3-a1b0556ed729(at)2ndquadrant(dot)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