Re: Walsender timeouts and large transactions

From: Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Sokolov Yura <funny(dot)falcon(at)postgrespro(dot)ru>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Petr Jelinek <pjmodos(at)pjmodos(dot)net>
Subject: Re: Walsender timeouts and large transactions
Date: 2017-11-03 14:54:09
Message-ID: e2939d26-f5cb-6581-0ca3-a1b0556ed729@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Antonin Houska 2017-11-03 15:33:45 Re: WIP: Aggregation push-down
Previous Message Andres Freund 2017-11-03 14:53:30 Re: [HACKERS] pgsql: Fix freezing of a dead HOT-updated tuple