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
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 |