Re: libpq: Process buffered SSL read bytes to support records >8kB on async API - Mailing list pgsql-hackers

From Andres Freund
Subject Re: libpq: Process buffered SSL read bytes to support records >8kB on async API
Date
Msg-id kl6inl3iyycxqpiedmnnzaz2ytid5dw6knwwqk5plg6ej34oi6@fbyu4ll7ebzn
Whole thread Raw
In response to Re: libpq: Process buffered SSL read bytes to support records >8kB on async API  (Jacob Champion <jacob.champion@enterprisedb.com>)
Responses Re: libpq: Process buffered SSL read bytes to support records >8kB on async API
List pgsql-hackers
Hi,

On 2025-07-15 15:31:25 -0700, Jacob Champion wrote:
> So I think pqReadData() needs to make the same guarantees for SSL/GSS
> that it does for plain TCP: when it returns, either 1) there are no
> bytes left pending or 2) the socket itself is marked readable.
> Otherwise I think we'll continue to chase weird corner cases.

It's not really the same issue, as at least my reproducer requires modifying
libpq (and thus clearly isn't a bug), but it thought it might still be
interesting. For one, I'm not sure this can only be triggered with the
modifications.


If one modifies libpq to use openssl readahead (which does result in speedups,
because otherwise openssl think it's useful to do lots of 5 byte reads from
the socket), I see occasional hangs in libpq.

diff --git i/src/interfaces/libpq/fe-secure-openssl.c w/src/interfaces/libpq/fe-secure-openssl.c
index 51dd7b9fec0..a8deb28594c 100644
--- i/src/interfaces/libpq/fe-secure-openssl.c
+++ w/src/interfaces/libpq/fe-secure-openssl.c
@@ -864,6 +864,9 @@ initialize_SSL(PGconn *conn)
      */
     SSL_CTX_set_mode(SSL_context, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);

+    SSL_CTX_set_read_ahead(SSL_context, 1);
+    SSL_CTX_set_default_read_buffer_len(SSL_context, 64*1024);
+
     /*
      * If the root cert file exists, load it so we can perform certificate
      * verification. If sslmode is "verify-full" we will also do further


ninja src/bin/psql/psql && src/bin/psql/psql -Xq -h localhost -c '\copy pg_class to stdout'

hangs reliably for me (if it's going to a file it's less reliable, so there's
a timing aspect here).


At that point psql is not interruptible, which isn't great either...

Backtrace:

#0  __internal_syscall_cancel (a1=a1@entry=140726731206072, a2=a2@entry=1, a3=-1, a4=a4@entry=0, a5=a5@entry=0,
a6=a6@entry=0,nr=7)
 
    at ./nptl/cancellation.c:44
#1  0x00007f1e25e3f6ad in __syscall_cancel (a1=a1@entry=140726731206072, a2=a2@entry=1, a3=<optimized out>,
a4=a4@entry=0,a5=a5@entry=0, a6=a6@entry=0, nr=7)
 
    at ./nptl/cancellation.c:75
#2  0x00007f1e25eb39c6 in __GI___poll (fds=fds@entry=0x7ffd7ed2edb8, nfds=nfds@entry=1, timeout=<optimized out>) at
../sysdeps/unix/sysv/linux/poll.c:29
#3  0x00007f1e26248dea in poll (__fds=0x7ffd7ed2edb8, __nfds=1, __timeout=<optimized out>) at
/usr/include/x86_64-linux-gnu/bits/poll2.h:44
#4  PQsocketPoll (sock=<optimized out>, forRead=<optimized out>, forWrite=<optimized out>, end_time=<optimized out>)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-misc.c:1175
#5  0x00007f1e262491a8 in pqSocketCheck (conn=conn@entry=0x55f69b7e68f0, forRead=1, forWrite=0, end_time=-1)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-misc.c:1114
#6  0x00007f1e262492b3 in pqWaitTimed (forRead=<optimized out>, forWrite=<optimized out>, conn=0x55f69b7e68f0,
end_time=<optimizedout>)
 
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-misc.c:1039
#7  0x00007f1e262492f1 in pqWait (forRead=forRead@entry=1, forWrite=forWrite@entry=0, conn=conn@entry=0x55f69b7e68f0)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-misc.c:1021
#8  0x00007f1e26234c46 in pqGetCopyData3 (conn=conn@entry=0x55f69b7e68f0, buffer=buffer@entry=0x7ffd7ed2efb8,
async=async@entry=0)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-protocol3.c:1846
#9  0x00007f1e26243d98 in PQgetCopyData (conn=conn@entry=0x55f69b7e68f0, buffer=buffer@entry=0x7ffd7ed2efb8,
async=async@entry=0)
    at ../../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-exec.c:2840
#10 0x000055f65d28f9c6 in handleCopyOut (conn=0x55f69b7e68f0, copystream=copystream@entry=0x7f1e25f985c0
<_IO_2_1_stdout_>,res=res@entry=0x7ffd7ed2f008)
 
    at ../../../../../home/andres/src/postgresql/src/bin/psql/copy.c:442
#11 0x000055f65d27b07d in HandleCopyResult (resultp=resultp@entry=0x7ffd7ed2f158, copystream=0x7f1e25f985c0
<_IO_2_1_stdout_>)
    at ../../../../../home/andres/src/postgresql/src/bin/psql/common.c:950
#12 0x000055f65d27dab8 in ExecQueryAndProcessResults (query=query@entry=0x55f69b8b3470 "COPY  pg_attribute TO STDOUT
",
    elapsed_msec=elapsed_msec@entry=0x7ffd7ed2f1b8, svpt_gone_p=svpt_gone_p@entry=0x7ffd7ed2f1b7,
is_watch=is_watch@entry=false,min_rows=min_rows@entry=0,
 
    opt=opt@entry=0x0, printQueryFout=0x0) at ../../../../../home/andres/src/postgresql/src/bin/psql/common.c:1934
#13 0x000055f65d27cdc9 in SendQuery (query=0x55f69b8b3470 "COPY  pg_attribute TO STDOUT ")
    at ../../../../../home/andres/src/postgresql/src/bin/psql/common.c:1212
#14 0x000055f65d28f6dd in do_copy (args=args@entry=0x55f69b8b1f10 "pg_attribute to stdout")
    at ../../../../../home/andres/src/postgresql/src/bin/psql/copy.c:370
#15 0x000055f65d299f24 in exec_command_copy (scan_state=scan_state@entry=0x55f69b8a71f0,
active_branch=active_branch@entry=true)
    at ../../../../../home/andres/src/postgresql/src/bin/psql/command.c:951
#16 0x000055f65d2a2065 in exec_command (cmd=cmd@entry=0x55f69b8b3310 "copy",
scan_state=scan_state@entry=0x55f69b8a71f0,cstack=cstack@entry=0x55f69b887ba0,
 
    query_buf=query_buf@entry=0x0, previous_buf=previous_buf@entry=0x0) at
../../../../../home/andres/src/postgresql/src/bin/psql/command.c:338
#17 0x000055f65d2a2b59 in HandleSlashCmds (scan_state=scan_state@entry=0x55f69b8a71f0,
cstack=cstack@entry=0x55f69b887ba0,query_buf=query_buf@entry=0x0,
 
    previous_buf=previous_buf@entry=0x0) at ../../../../../home/andres/src/postgresql/src/bin/psql/command.c:241
#18 0x000055f65d29305f in main (argc=<optimized out>, argv=<optimized out>) at
../../../../../home/andres/src/postgresql/src/bin/psql/startup.c:409


It does seem ... weird ... that pqGetCopyData3() just processes whatever is
already in the libpq buffer and then waits for the socket to become readable,
before ever calling pqReadData(). What guarantees, even without the change
above, that a) there's no pending data inside openssl b) that we're not
waiting for a write?

If I make pqGetCopyData3() call pqReadData() before the pqWait() and continue
if it returns 1, the hang vanishes.


The same
            if (pqWait(true, false, conn) ||
                pqReadData(conn) < 0)
                return -2;

pattern (without a preceding pqReadData()) exists in several other places :(



> What I'm not sure about is unforeseen consequences -- could fixing
> this expose other latent bugs? If we're concerned about that, we might
> have to tier the backports to make sure everything remains stable.
> Here are the three bugs discussed so far, with the fix for 3 possibly
> subsuming 1 and 2 but also affecting more code:
>
> 1) pqSocketCheck() checks for pending SSL bytes but not GSS bytes
> 2) PQconsumeInput() does not drain all pending bytes
> 3) pqReadData() does not drain all pending bytes

What are the limits for the maximum amount of data this could make us buffer
in addition to what we are buffering right now?  It's not entirely obvious to
me that a loop around pqReadData() as long as there is pending data couldn't
make us buffer a lot of data.

Do you have a WIP patch?

Greetings,

Andres Freund



pgsql-hackers by date:

Previous
From: Tomas Vondra
Date:
Subject: Re: index prefetching
Next
From: Peter Geoghegan
Date:
Subject: Re: index prefetching