8000 Don't reduce output request size on non-Unix-socket connections. · postgres/postgres@f09fea3 · GitHub
[go: up one dir, main page]
Skip to content

Commit f09fea3

Browse files
tglsfdctglsfdc
committed
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 d053a87. 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
1 parent 4adbaa3 commit f09fea3

File tree

3 files changed

+33
-7
lines changed

3 files changed

+33
-7
lines changed

‎src/backend/libpq/be-secure-gssapi.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,9 +120,9 @@ be_gssapi_write(Port *port, void *ptr, size_t len)
120120
* again, so if it offers a len less than that, something is wrong.
121121
*
122122
* Note: it may seem attractive to report partial write completion once
123-
* we've successfully sent any encrypted packets. However, that can cause
124-
* problems for callers; notably, pqPutMsgEnd's heuristic to send only
125-
* full 8K blocks interacts badly with such a hack. We won't save much,
123+
* we've successfully sent any encrypted packets. However, doing that
124+
* expands the state space of this processing and has been responsible for
125+
* bugs in the past (cf. commit d053a879b). We won't save much,
126126
* typically, by letting callers discard data early, so don't risk it.
127127
*/
128128
if (len < PqGSSSendConsumed)

‎src/interfaces/libpq/fe-misc.c

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -601,9 +601,35 @@ pqPutMsgEnd(PGconn *conn)
601601
/* Make message eligible to send */
602602
conn->outCount = conn->outMsgEnd;
603603

604+
/* If appropriate, try to push out some data */
604605
if (conn->outCount >= 8192)
605606
{
606-
int toSend = conn->outCount - (conn->outCount % 8192);
607+
int toSend = conn->outCount;
608+
609+
/*
610+
* On Unix-pipe connections, it seems profitable to prefer sending
611+
* pipe-buffer-sized packets not randomly-sized ones, so retain the
612+
* last partial-8K chunk in our buffer for now. On TCP connections,
613+
* the advantage of that is far less clear. Moreover, it flat out
614+
* isn't safe when using SSL or GSSAPI, because those code paths have
615+
* API stipulations that if they fail to send all the data that was
616+
* offered in the previous write attempt, we mustn't offer less data
617+
* in this write attempt. The previous write attempt might've been
618+
* pqFlush attempting to send everything in the buffer, so we mustn't
619+
* offer less now. (Presently, we won't try to use SSL or GSSAPI on
620+
* Unix connections, so those checks are just Asserts. They'll have
621+
* to become part of the regular if-test if we ever change that.)
622+
*/
623+
if (conn->raddr.addr.ss_family == AF_UNIX)
624+
{
625+
#ifdef USE_SSL
626+
Assert(!conn->ssl_in_use);
627+
#endif
628+
#ifdef ENABLE_GSS
629+
Assert(!conn->gssenc);
630+
#endif
631+
toSend -= toSend % 8192;
632+
}
607633

608634
if (pqSendSome(conn, toSend) < 0)
609635
return EOF;

‎src/interfaces/libpq/fe-secure-gssapi.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,9 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
112112
* again, so if it offers a len less than that, something is wrong.
113113
*
114114
* Note: it may seem attractive to report partial write completion once
115-
* we've successfully sent any encrypted packets. However, that can cause
116-
* problems for callers; notably, pqPutMsgEnd's heuristic to send only
117-
* full 8K blocks interacts badly with such a hack. We won't save much,
115+
* we've successfully sent any encrypted packets. However, doing that
116+
* expands the state space of this processing and has been responsible for
117+
* bugs in the past (cf. commit d053a879b). We won't save much,
118118
* typically, by letting callers discard data early, so don't risk it.
119119
*/
120120
if (len < PqGSSSendConsumed)

0 commit comments

Comments
 (0)
0