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.jeli...@2ndquadrant.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to