Re: BUG #18210: libpq: PQputCopyData sometimes fails in non-blocking mode over GSSAPI encrypted connection

I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> The error "GSSAPI caller failed to retransmit all data needing to be
>> retried" is raised here:
>>
https://github.com/postgres/postgres/blob/eeb0ebad79d9350305d9e111fbac76e20fa4b2fe/src/interfaces/libpq/fe-secure-gssapi.c#L110
>> It happens only in non-blocking mode over GSSAPI encrypted connections. It
>> isn't reliable and depends on the network timing. When sending a 7MB file in
>> alternating pieces of 27KB and 180 Byte per PQputCopyData() there is a 50%
>> chance to get the failure over the local network. It doesn't happen if TLS
>> is used instead.

> A repro script would be really really helpful here.

After consuming more caffeine, I was able to repro it with the
not-intended-for-commit hack in 0001 attached.  (On my machine,
the test just hangs up upon failing, because the hacked-up
logic in copy.c doesn't cope very well with the failure.  I don't
think that is copy.c's fault though, it's just an incomplete hack.)

I concur with the conclusion that it's really pqPutMsgEnd's fault.
By deciding not to send the last partial block that's in the
outBuffer, it runs the risk of not presenting some data that it did
present the last time, and that can trigger the "failed to retransmit
all data" error.  This happens because GSS's gss_MaxPktSize is a bit
less than 16K (it's 16320 on my machine).  So if we initially present
24K of data (3 blocks), and pg_GSS_write successfully encrypts and
sends one packet of data, then it will encrypt all the rest.  But if
its second pqsecure_raw_write call fails with EINTR, it will return
with bytes_sent = 16320 (and PqGSSSendConsumed = 8256), causing us to
reduce the outBuffer contents to 8256 bytes plus whatever partial
block we didn't try to send.  If we don't fill outBuffer to at least
16K before trying again, we'll try to send just 8192 bytes, and
kaboom.  (This is why the alternating-long-and-short-lines business
is important.)

The quick hack in 0002 attached fixes it, but I can't say that
I like this solution: it's propagating a bit of ugliness that
ought to be localized in pg_GSS_write out to callers.

I wonder if we should drop the idea of returning a positive bytecount
after a partial write, and just return the pqsecure_raw_write result,
and not reset PqGSSSendConsumed until we write everything presented.
In edge cases maybe that would result in some buffer bloat, but it
doesn't seem worse than what happens when the very first
pqsecure_raw_write returns EINTR.

In any case, the backend needs a look to see whether it requires a
similar fix.  We don't do nonblock mode there, but I don't think
that means we can never get EINTR.

            regards, tom lane

diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index dbbbdb8898..2b9c278ef7 100644
--- a/src/bin/psql/copy.c
+++ b/src/bin/psql/copy.c
@@ -505,7 +505,7 @@ handleCopyOut(PGconn *conn, FILE *copystream, PGresult **res)
  */

 /* read chunk size for COPY IN - size is not critical */
-#define COPYBUFSIZ 8192
+#define COPYBUFSIZ 32768

 bool
 handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
@@ -531,6 +531,8 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
         goto copyin_cleanup;
     }

+    (void) PQsetnonblocking(conn, 1);
+
     /* Prompt if interactive input */
     if (isatty(fileno(copystream)))
     {
@@ -649,16 +651,17 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
             }

             /*
-             * If the buffer is full, or we've reached the EOF, flush it.
-             *
-             * Make sure there's always space for four more bytes in the
-             * buffer, plus a NUL terminator.  That way, an EOF marker is
-             * never split across two fgets() calls, which simplifies the
-             * logic.
+             * Flush after every line for test purposes.
              */
-            if (buflen >= COPYBUFSIZ - 5 || (copydone && buflen > 0))
+            if (buflen > 0)
             {
-                if (PQputCopyData(conn, buf, buflen) <= 0)
+                int pcdres;
+
+                while ((pcdres = PQputCopyData(conn, buf, buflen)) == 0)
+                {
+                    /* busy-wait */
+                }
+                if (pcdres < 0)
                 {
                     OK = false;
                     break;
@@ -686,6 +689,8 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)

 copyin_cleanup:

+    (void) PQsetnonblocking(conn, 0);
+
     /*
      * Clear the EOF flag on the stream, in case copying ended due to an EOF
      * signal.  This allows an interactive TTY session to perform another COPY
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index 0deb9bffc8..711bbc1e29 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -419,6 +419,25 @@ test_query(
     'gssencmode=require',
     'sending 100K lines works');

+my $bigdata = '';
+for (my $rpt = 0; $rpt < 1000; $rpt++)
+{
+    $bigdata .= 'xyzzy' x (27*1024/5) . "\n";
+    $bigdata .= 'qwert' x (180/5) . "\n";
+}
+
+test_query(
+    $node,
+    'test1',
+    "CREATE TEMP TABLE mytab (f1 text);\n"
+      . "COPY mytab FROM STDIN;\n"
+      . $bigdata
+      . "\\.\n"
+      . "SELECT COUNT(*) FROM mytab;",
+    qr/^2000$/s,
+    'gssencmode=require',
+    'sending alternating-size lines works');
+
 # require_auth=gss succeeds if required.
 $node->connect_ok(
     $node->connstr('postgres')
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 660cdec93c..5a74a95968 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -541,7 +541,19 @@ pqPutMsgEnd(PGconn *conn)
 
     if (conn->outCount >= 8192)
     {
-        int            toSend = conn->outCount - (conn->outCount % 8192);
+        int            toSend = conn->outCount;
+
+        /*
+         * Normally we only send full 8K blocks.  However, if we don't present
+         * the last partial block we can trigger pg_GSS_write's "failed to
+         * retransmit all data" failure, since rounding toSend down could
+         * result in not presenting some data that we did present on an
+         * earlier try.  The full-block heuristic is pretty useless anyway
+         * when encrypting, since encryption overhead will throw off the
+         * number.  Hence, in GSSAPI mode just send all we have.
+         */
+        if (!conn->gssenc)
+            toSend -= toSend % 8192;
 
         if (pqSendSome(conn, toSend) < 0)
             return EOF;