Re: [HACKERS] Walsender timeouts and large transactions

Hello,

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>
> Hi,
>
> thanks for checking.
>
> On 02/11/17 10:00, Robert Haas wrote:
> > On Wed, Nov 1, 2017 at 8:19 PM, Petr Jelinek
> > <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
> >> sorry for the delay but I didn't have much time in past weeks to follow
> >> this thread.
> >
> > + TimestampTz now = GetCurrentTimestamp();
> > +
> > /* output previously gathered data in a CopyData packet */
> > pq_putmessage_noblock('d', ctx->out->data, ctx->out->len);
> >
> > /*
> > * Fill the send timestamp last, so that it is taken as late as possible.
> > * This is somewhat ugly, but the protocol is set as it's already used for
> > * several releases by streaming physical replication.
> > */
> > resetStringInfo(&tmpbuf);
> > - pq_sendint64(&tmpbuf, GetCurrentTimestamp());
> > + pq_sendint64(&tmpbuf, now);
> > memcpy(&ctx->out->data[1 + sizeof(int64) + sizeof(int64)],
> > tmpbuf.data, sizeof(int64));
> >
> > This change falsifies the comments. Maybe initialize now just after
> > resetSetringInfo() is done.
>
> Eh, right, I can do that.
>
> >
> > - /* 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.

I think this is the last message in this thread so I changed the
status of the CF entry to "Waiting for Author".

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center