Re: Walsender timeouts and large transactions

On Wed, Nov 1, 2017 at 8:19 PM, Petr Jelinek
<petr.jelinek@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.

-    /* 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.

Other than that this looks like a reasonable change to me, but I'm not
an expert on this code.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company