Don't reduce output request size on non-Unix-socket connections.

Traditionally, libpq's pqPutMsgEnd has rounded down the amount-to-send
to be a multiple of 8K when it is eagerly writing some data.  This
still seems like a good idea when sending through a Unix socket, as
pipes typically have a buffer size of 8K or some fraction/multiple of
that.  But there's not much argument for it on a TCP connection, since
(a) standard MTU values are not commensurate with that, and (b) the
kernel typically applies its own packet splitting/merging logic.

Worse, our SSL and GSSAPI code paths both have API stipulations that
if they fail to send all the data that was offered in the previous
write attempt, we mustn't offer less data in the next attempt; else
we may get "SSL error: bad length" or "GSSAPI caller failed to
retransmit all data needing to be retried".  The previous write
attempt might've been pqFlush attempting to send everything in the
buffer, so pqPutMsgEnd can't safely write less than the full buffer
contents.  (Well, we could add some more state to track exactly how
much the previous write attempt was, but there's little value evident
in such extra complication.)  Hence, apply the round-down only on
AF_UNIX sockets, where we never use SSL or GSSAPI.

Interestingly, we had a very closely related bug report before,
which I attempted to fix in commit d053a879b.  But the test case
we had then seemingly didn't trigger this pqFlush-then-pqPutMsgEnd
scenario, or at least we failed to recognize this variant of the bug.

Bug: #18907
Reported-by: Dorjpalam Batbaatar <htgn.dbat.95@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18907-d41b9bcf6f29edda@postgresql.org
Backpatch-through: 13

diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
index 657beaa5a70e3c9831b396684e72d9b7c6c03b10..2839972e6669ae6977f7ec31907d83ee623a740a 100644 (file)
--- a/src/backend/libpq/be-secure-gssapi.c
+++ b/src/backend/libpq/be-secure-gssapi.c
@@ -120,9 +120,9 @@ be_gssapi_write(Port *port, void *ptr, size_t len)
         * again, so if it offers a len less than that, something is wrong.
         *
         * Note: it may seem attractive to report partial write completion once
-        * we've successfully sent any encrypted packets.  However, that can cause
-        * problems for callers; notably, pqPutMsgEnd's heuristic to send only
-        * full 8K blocks interacts badly with such a hack.  We won't save much,
+        * we've successfully sent any encrypted packets.  However, doing that
+        * expands the state space of this processing and has been responsible for
+        * bugs in the past (cf. commit d053a879b).  We won't save much,
         * typically, by letting callers discard data early, so don't risk it.
         */
        if (len < PqGSSSendConsumed)

diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 02dd799c4558d961e70e250b31537884e8d8abe6..59ed2ff98df802ea5075c655442e66f219a80163 100644 (file)
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -539,9 +539,35 @@ pqPutMsgEnd(PGconn *conn)
        /* Make message eligible to send */
        conn->outCount = conn->outMsgEnd;
 
+       /* If appropriate, try to push out some data */
        if (conn->outCount >= 8192)
        {
-               int                     toSend = conn->outCount - (conn->outCount % 8192);
+               int                     toSend = conn->outCount;
+
+               /*
+                * On Unix-pipe connections, it seems profitable to prefer sending
+                * pipe-buffer-sized packets not randomly-sized ones, so retain the
+                * last partial-8K chunk in our buffer for now.  On TCP connections,
+                * the advantage of that is far less clear.  Moreover, it flat out
+                * isn't safe when using SSL or GSSAPI, because those code paths have
+                * API stipulations that if they fail to send all the data that was
+                * offered in the previous write attempt, we mustn't offer less data
+                * in this write attempt.  The previous write attempt might've been
+                * pqFlush attempting to send everything in the buffer, so we mustn't
+                * offer less now.  (Presently, we won't try to use SSL or GSSAPI on
+                * Unix connections, so those checks are just Asserts.  They'll have
+                * to become part of the regular if-test if we ever change that.)
+                */
+               if (conn->raddr.addr.ss_family == AF_UNIX)
+               {
+#ifdef USE_SSL
+                       Assert(!conn->ssl_in_use);
+#endif
+#ifdef ENABLE_GSS
+                       Assert(!conn->gssenc);
+#endif
+                       toSend -= toSend % 8192;
+               }
 
                if (pqSendSome(conn, toSend) < 0)
                        return EOF;

diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
index 8000d0b57836c151026ea4e448a96893c7cdf5d7..a8560e20b5a05cc1e96f8dab5e832377b964836c 100644 (file)
--- a/src/interfaces/libpq/fe-secure-gssapi.c
+++ b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -112,9 +112,9 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
         * again, so if it offers a len less than that, something is wrong.
         *
         * Note: it may seem attractive to report partial write completion once
-        * we've successfully sent any encrypted packets.  However, that can cause
-        * problems for callers; notably, pqPutMsgEnd's heuristic to send only
-        * full 8K blocks interacts badly with such a hack.  We won't save much,
+        * we've successfully sent any encrypted packets.  However, doing that
+        * expands the state space of this processing and has been responsible for
+        * bugs in the past (cf. commit d053a879b).  We won't save much,
         * typically, by letting callers discard data early, so don't risk it.
         */
        if (len < PqGSSSendConsumed)