public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 4/4] Update functions to pass only errinfo struct.. 10+ messages / 6 participants [nested] [flat]
* [PATCH 4/4] Update functions to pass only errinfo struct.. @ 2020-06-22 22:13 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Justin Pryzby @ 2020-06-22 22:13 UTC (permalink / raw) ..this is a more complete change, and probably a good idea since parallel workers have an partially-populated LVRelStats, and it's better to avoid the idea that it's usable (dead_tuples in particular is a bogus pointer). --- src/backend/access/heap/vacuumlazy.c | 58 ++++++++++++++-------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 6bd2867660..db3bd86338 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -344,10 +344,10 @@ static void lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, - LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats); + LVDeadTuples *dead_tuples, double reltuples, LVSavedPosition *errinfo); static void lazy_cleanup_index(Relation indrel, IndexBulkDeleteResult **stats, - double reltuples, bool estimated_count, LVRelStats *vacrelstats); + double reltuples, bool estimated_count, LVSavedPosition *errinfo); static int lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer, int tupindex, LVRelStats *vacrelstats, Buffer *vmbuffer); static bool should_attempt_truncation(VacuumParams *params, @@ -367,13 +367,13 @@ static void lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult * int nindexes); static void parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVDeadTuples *dead_tuples, - int nindexes, LVRelStats *vacrelstats); + int nindexes, LVSavedPosition *errinfo); static void vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVSharedIndStats *shared_indstats, - LVDeadTuples *dead_tuples, LVRelStats *vacrelstats); + LVDeadTuples *dead_tuples, LVSavedPosition *errinfo); static void lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, LVRelStats *vacrelstats, LVParallelState *lps, int nindexes); @@ -1798,7 +1798,7 @@ lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, for (idx = 0; idx < nindexes; idx++) lazy_vacuum_index(Irel[idx], &stats[idx], vacrelstats->dead_tuples, - vacrelstats->old_live_tuples, vacrelstats); + vacrelstats->old_live_tuples, &vacrelstats->errinfo); } /* Increase and report the number of index scans */ @@ -2163,7 +2163,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats, * indexes in the case where no workers are launched. */ parallel_vacuum_index(Irel, stats, lps->lvshared, - vacrelstats->dead_tuples, nindexes, vacrelstats); + vacrelstats->dead_tuples, nindexes, &vacrelstats->errinfo); /* * Next, accumulate buffer and WAL usage. (This must wait for the workers @@ -2198,7 +2198,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats, static void parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVDeadTuples *dead_tuples, - int nindexes, LVRelStats *vacrelstats) + int nindexes, LVSavedPosition *errinfo) { /* * Increment the active worker count if we are able to launch any worker. @@ -2232,7 +2232,7 @@ parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats, /* Do vacuum or cleanup of the index */ vacuum_one_index(Irel[idx], &(stats[idx]), lvshared, shared_indstats, - dead_tuples, vacrelstats); + dead_tuples, errinfo); } /* @@ -2273,7 +2273,7 @@ vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, skip_parallel_vacuum_index(Irel[i], lps->lvshared)) vacuum_one_index(Irel[i], &(stats[i]), lps->lvshared, shared_indstats, vacrelstats->dead_tuples, - vacrelstats); + &vacrelstats->errinfo); } /* @@ -2293,7 +2293,7 @@ vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats, static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, LVShared *lvshared, LVSharedIndStats *shared_indstats, - LVDeadTuples *dead_tuples, LVRelStats *vacrelstats) + LVDeadTuples *dead_tuples, LVSavedPosition *errinfo) { IndexBulkDeleteResult *bulkdelete_res = NULL; @@ -2313,10 +2313,10 @@ vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, /* Do vacuum or cleanup of the index */ if (lvshared->for_cleanup) lazy_cleanup_index(indrel, stats, lvshared->reltuples, - lvshared->estimated_count, vacrelstats); + lvshared->estimated_count, errinfo); else lazy_vacuum_index(indrel, stats, dead_tuples, - lvshared->reltuples, vacrelstats); + lvshared->reltuples, errinfo); /* * Copy the index bulk-deletion result returned from ambulkdelete and @@ -2392,7 +2392,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, lazy_cleanup_index(Irel[idx], &stats[idx], vacrelstats->new_rel_tuples, vacrelstats->tupcount_pages < vacrelstats->rel_pages, - vacrelstats); + &vacrelstats->errinfo); } } @@ -2407,7 +2407,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats, */ static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, - LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats) + LVDeadTuples *dead_tuples, double reltuples, LVSavedPosition *errinfo) { IndexVacuumInfo ivinfo; const char *msg; @@ -2425,8 +2425,8 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, ivinfo.strategy = vac_strategy; /* Update error traceback information */ - olderrinfo = vacrelstats->errinfo; - update_vacuum_error_info(&vacrelstats->errinfo, + olderrinfo = *errinfo; + update_vacuum_error_info(errinfo, VACUUM_ERRCB_PHASE_VACUUM_INDEX, InvalidBlockNumber, RelationGetRelationName(indrel)); @@ -2442,12 +2442,12 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, ereport(elevel, (errmsg(msg, - vacrelstats->errinfo.indname, + errinfo->indname, dead_tuples->num_tuples), errdetail_internal("%s", pg_rusage_show(&ru0)))); /* Revert to the previous phase information for error traceback */ - update_vacuum_error_info(&vacrelstats->errinfo, + update_vacuum_error_info(errinfo, olderrinfo.phase, olderrinfo.blkno, olderrinfo.indname); @@ -2462,7 +2462,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, static void lazy_cleanup_index(Relation indrel, IndexBulkDeleteResult **stats, - double reltuples, bool estimated_count, LVRelStats *vacrelstats) + double reltuples, bool estimated_count, LVSavedPosition *errinfo) { IndexVacuumInfo ivinfo; const char *msg; @@ -2481,8 +2481,8 @@ lazy_cleanup_index(Relation indrel, ivinfo.strategy = vac_strategy; /* Update error traceback information */ - olderrinfo = vacrelstats->errinfo; - update_vacuum_error_info(&vacrelstats->errinfo, + olderrinfo = *errinfo; + update_vacuum_error_info(errinfo, VACUUM_ERRCB_PHASE_INDEX_CLEANUP, InvalidBlockNumber, RelationGetRelationName(indrel)); @@ -2490,7 +2490,7 @@ lazy_cleanup_index(Relation indrel, *stats = index_vacuum_cleanup(&ivinfo, *stats); /* Revert back to the old phase information for error traceback */ - update_vacuum_error_info(&vacrelstats->errinfo, + update_vacuum_error_info(errinfo, olderrinfo.phase, olderrinfo.blkno, olderrinfo.indname); @@ -3474,7 +3474,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) int nindexes; char *sharedquery; IndexBulkDeleteResult **stats; - LVRelStats vacrelstats; + LVSavedPosition errinfo; ErrorContextCallback errcallback; lvshared = (LVShared *) shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_SHARED, @@ -3529,14 +3529,14 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) * Initialize vacrelstats for use as error callback arg by parallel * worker. */ - vacrelstats.errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); - vacrelstats.errinfo.relname = pstrdup(RelationGetRelationName(onerel)); - vacrelstats.errinfo.indname = NULL; - vacrelstats.errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ + errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); + errinfo.relname = pstrdup(RelationGetRelationName(onerel)); + errinfo.indname = NULL; + errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ /* Setup error traceback support for ereport() */ errcallback.callback = vacuum_error_callback; - errcallback.arg = &vacrelstats.errinfo; + errcallback.arg = &errinfo; errcallback.previous = error_context_stack; error_context_stack = &errcallback; @@ -3545,7 +3545,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) /* Process indexes to perform vacuum/cleanup */ parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes, - &vacrelstats); + &errinfo); /* Report buffer/WAL usage during parallel execution */ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false); -- 2.17.0 --oLBj+sq0vYjzfsbl-- ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2024-09-10 18:49 Jacob Champion <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Jacob Champion @ 2024-09-10 18:49 UTC (permalink / raw) To: Lars Kanis <[email protected]>; +Cc: [email protected] On Sun, Sep 8, 2024 at 1:08 PM Lars Kanis <[email protected]> wrote: > I'm the maintainer of ruby-pg the ruby interface to the PostgreSQL > database. This binding uses the asynchronous API of libpq by default to > facilitate the ruby IO wait and scheduling mechanisms. > > This works well with the vanilla postgresql server, but it leads to > starvation with other types of servers using the postgresql wire > protocol 3. This is because the current functioning of the libpq async > interface depends on a maximum size of SSL records of 8kB. Thanks for the report! I wanted evidence that this wasn't a ruby-pg-specific problem, so I set up a test case with Python/psycopg2. I was able to reproduce a hang when all of the following were true: - psycopg2's async mode was enabled - the client performs a PQconsumeInput/PQisBusy loop, waiting on socket read events when the connection is busy (I used psycopg2.extras.wait_select() for this) - the server splits a large message over many large TLS records - the server packs the final ReadyForQuery message into the same record as the split message's final fragment Gory details of the packet sizes, if it's helpful: - max TLS record size is 12k, because it made the math easier - server sends DataRow of 32006 bytes, followed by DataRow of 806 bytes, followed by CommandComplete/ReadyForQuery - so there are three TLS records on the wire containing 1) DataRow 1 fragment 1 (12k bytes) 2) DataRow 1 fragment 2 (12k bytes) 3) DataRow 1 fragment 3 (7430 bytes) + DataRow 2 (806 bytes) + CommandComplete + ReadyForQuery > To fix this issue the attached patch calls pqReadData() repeatedly in > PQconsumeInput() until there is no buffered SSL data left to be read. > Another solution could be to process buffered SSL read bytes in > PQisBusy() instead of PQconsumeInput() . I agree that PQconsumeInput() needs to ensure that the transport buffers are all drained. But I'm not sure this is a complete solution; doesn't GSS have the same problem? And are there any other sites that need to make the same guarantee before returning? I need to switch away from this for a bit. Would you mind adding this to the next Commitfest as a placeholder? https://commitfest.postgresql.org/50/ Thanks, --Jacob ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2024-09-11 19:08 Lars Kanis <[email protected]> parent: Jacob Champion <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Lars Kanis @ 2024-09-11 19:08 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: [email protected] Thank you Jacob for verifying this issue! > Gory details of the packet sizes, if it's helpful: > - max TLS record size is 12k, because it made the math easier > - server sends DataRow of 32006 bytes, followed by DataRow of 806 > bytes, followed by CommandComplete/ReadyForQuery > - so there are three TLS records on the wire containing > 1) DataRow 1 fragment 1 (12k bytes) > 2) DataRow 1 fragment 2 (12k bytes) > 3) DataRow 1 fragment 3 (7430 bytes) + DataRow 2 (806 bytes) > + CommandComplete + ReadyForQuery How did you verify the issue on the server side - with YugabyteDB or with a modified Postgres server? I'd like to verify the GSSAPI part and I'm familiar with the Postgres server only. > I agree that PQconsumeInput() needs to ensure that the transport > buffers are all drained. But I'm not sure this is a complete solution; > doesn't GSS have the same problem? And are there any other sites that > need to make the same guarantee before returning? Which other sites do you mean? The synchronous transfer already works, since the select() is short-circuit in case of pending bytes: [1] > I need to switch away from this for a bit. Would you mind adding this > to the next Commitfest as a placeholder? No problem; registered: https://commitfest.postgresql.org/50/5251/ -- Regards, Lars [1] https://github.com/postgres/postgres/blob/77761ee5dddc0518235a51c533893e81e5f375b9/src/interfaces/li... ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2024-09-11 23:00 Jacob Champion <[email protected]> parent: Lars Kanis <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Jacob Champion @ 2024-09-11 23:00 UTC (permalink / raw) To: Lars Kanis <[email protected]>; +Cc: [email protected] On Wed, Sep 11, 2024 at 12:08 PM Lars Kanis <[email protected]> wrote: > How did you verify the issue on the server side - with YugabyteDB or > with a modified Postgres server? I'd like to verify the GSSAPI part and > I'm familiar with the Postgres server only. Neither, unfortunately -- I have a protocol testbed that I use for this kind of stuff. I'm happy to share once I get it cleaned up, but it's unlikely to help you in this case since I haven't implemented gssenc support. Patching the Postgres server itself seems like a good way to go. > > And are there any other sites that > > need to make the same guarantee before returning? > > Which other sites do you mean? I'm mostly worried that other parts of libpq might assume that a single call to pqReadData will drain the buffers. If not, great! -- but I haven't had time to check all the call sites. > > I need to switch away from this for a bit. Would you mind adding this > > to the next Commitfest as a placeholder? > > No problem; registered: https://commitfest.postgresql.org/50/5251/ Thank you! --Jacob ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2026-07-03 09:25 Heikki Linnakangas <[email protected]> parent: Jacob Champion <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Heikki Linnakangas @ 2026-07-03 09:25 UTC (permalink / raw) To: solai v <[email protected]>; +Cc: Mark Dilger <[email protected]>; Jacob Champion <[email protected]>; Andres Freund <[email protected]>; Lars Kanis <[email protected]>; [email protected]; Merlin Moncure <[email protected]> On 14/05/2026 07:32, solai v wrote: > Hi, > i tested the latest V3 patch series on current postgreSQL HEAD with SSL > enabled.The patches applied cleanly,build succeeded,and the server > started successfully.I ran the async libpq test repeatedly and also > tested large COPY TO STDOUT operations over SSL.I did not observe > hangs,crashes,or visible regressions during testing. Thanks for the testing! Here's another patch version, with some small comment and commit message changes, and I split the changes slightly differently between the two patches. End result is otherwise the same. I plan to commit this in the next few days. This question still remains: > In pqReadData we have this: > > >> /* >> * A return value of 0 could mean just that no data is now available, or >> * it could mean EOF --- that is, the server has closed the connection. >> * Since we have the socket in nonblock mode, the only way to tell the >> * difference is to see if select() is saying that the file is ready. >> * Grumble. Fortunately, we don't expect this path to be taken much, >> * since in normal practice we should not be trying to read data unless >> * the file selected for reading already. >> * >> * In SSL mode it's even worse: SSL_read() could say WANT_READ and then >> * data could arrive before we make the pqReadReady() test, but the second >> * SSL_read() could still say WANT_READ because the data received was not >> * a complete SSL record. So we must play dumb and assume there is more >> * data, relying on the SSL layer to detect true EOF. >> */ >> >> #ifdef USE_SSL >> if (conn->ssl_in_use) >> return 0; >> #endif > > > Should we do the same for GSS as we do for SSL here? If someone more familiar with GSS would take a look at that, that'd be great. I assume that it has the same issue, and we should do the same for GSS as for SSL here. That's additional to these patches, though, so it doesn't prevent committing these now. - Heikki Attachments: [text/x-patch] v4-0001-libpq-Extend-read-pending-check-from-SSL-to-GSS.patch (7.0K, ../../[email protected]/2-v4-0001-libpq-Extend-read-pending-check-from-SSL-to-GSS.patch) download | inline diff: From fcfebe04c828a5f1ecfb52795cc63fcec7064908 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <[email protected]> Date: Fri, 3 Jul 2026 12:07:18 +0300 Subject: [PATCH v4 1/2] libpq: Extend "read pending" check from SSL to GSS An extra check for pending bytes in the SSL layer has been part of pqReadReady() for a very long time (79ff2e96d). But when GSS transport encryption was added, it didn't receive the same treatment. (As 79ff2e96d notes, "The bug that I fixed in this patch is exceptionally hard to reproduce reliably.") Without that check, it's possible to hit a hang in gssencmode, if the server splits a large libpq message such that the final message in a streamed response is part of the same wrapped token as the split message: DataRowDataRowDataRowDataRowDataRowData -- token boundary -- RowDataRowCommandCompleteReadyForQuery If the split message takes up enough memory to nearly fill libpq's receive buffer, libpq may return from pqReadData() before the later messages are pulled out of the PqGSSRecvBuffer. Without additional socket activity from the server, pqReadReady() (via pqSocketCheck()) will never again return true, hanging the connection. Pull the pending-bytes check into the pqsecure API layer, where both SSL and GSS now implement it. Note that this does not fix the root problem! Third party clients of libpq have no way to call pqsecure_read_is_pending() in their own polling. This just brings the GSS implementation up to par with the existing SSL workaround; a broader fix is left to a subsequent commit. In preparation for the broader fix, this patch already changes the *_read_pending() functions to return the number of bytes in the buffer rather than just a boolean. The current callers don't need that, but the subsequent fix will. Author: Jacob Champion <[email protected]> Discussion: https://postgr.es/m/CAOYmi%2BmpymrgZ76Jre2dx_PwRniS9YZojwH0rZnTuiGHCsj0rA%40mail.gmail.com Backpatch-through: 14 --- src/interfaces/libpq/fe-misc.c | 6 ++--- src/interfaces/libpq/fe-secure-gssapi.c | 7 +++++ src/interfaces/libpq/fe-secure-openssl.c | 34 +++++++++++++++++++++--- src/interfaces/libpq/fe-secure.c | 22 +++++++++++++++ src/interfaces/libpq/libpq-int.h | 6 +++-- 5 files changed, 66 insertions(+), 9 deletions(-) diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c index 905344d5c38..58ccca1e393 100644 --- a/src/interfaces/libpq/fe-misc.c +++ b/src/interfaces/libpq/fe-misc.c @@ -1099,14 +1099,12 @@ pqSocketCheck(PGconn *conn, int forRead, int forWrite, pg_usec_time_t end_time) return -1; } -#ifdef USE_SSL - /* Check for SSL library buffering read bytes */ - if (forRead && conn->ssl_in_use && pgtls_read_pending(conn)) + /* Check for SSL/GSS library buffering read bytes */ + if (forRead && pqsecure_bytes_pending(conn) != 0) { /* short-circuit the select */ return 1; } -#endif } /* We will retry as long as we get EINTR */ diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c index 72f438dfa9c..cc60240582d 100644 --- a/src/interfaces/libpq/fe-secure-gssapi.c +++ b/src/interfaces/libpq/fe-secure-gssapi.c @@ -471,6 +471,13 @@ gss_read(PGconn *conn, void *recv_buffer, size_t length, ssize_t *ret) return PGRES_POLLING_OK; } +ssize_t +pg_GSS_bytes_pending(PGconn *conn) +{ + Assert(PqGSSResultLength >= PqGSSResultNext); + return (ssize_t) (PqGSSResultLength - PqGSSResultNext); +} + /* * Negotiate GSSAPI transport for a connection. When complete, returns * PGRES_POLLING_OK. Will return PGRES_POLLING_READING or diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 6b44eeb68eb..1b22ba7b7f6 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -230,10 +230,38 @@ rloop: return n; } -bool -pgtls_read_pending(PGconn *conn) +ssize_t +pgtls_bytes_pending(PGconn *conn) { - return SSL_pending(conn->ssl) > 0; + int pending; + + /* + * OpenSSL readahead is documented to break SSL_pending(). + */ + Assert(!SSL_get_read_ahead(conn->ssl)); + + pending = SSL_pending(conn->ssl); + if (pending < 0) + { + /* shouldn't be possible */ + Assert(false); + libpq_append_conn_error(conn, "OpenSSL reports negative bytes pending"); + return -1; + } + else if (pending == INT_MAX) + { + /* + * If we ever found a legitimate way to hit this, we'd need to loop + * around in the caller to call pgtls_bytes_pending() again. Throw an + * error rather than complicate the code in that way, because + * SSL_read() should be bounded to the size of a single TLS record, + * and conn->inBuffer can't currently go past INT_MAX in size anyway. + */ + libpq_append_conn_error(conn, "OpenSSL reports INT_MAX bytes pending"); + return -1; + } + + return (ssize_t) pending; } ssize_t diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index 31d5b48d3f9..907cdb9ea39 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -243,6 +243,28 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len) return n; } +/* + * Return the number of bytes available in the transport buffer. + * + * If pqsecure_read() is called for this number of bytes, it's guaranteed to + * return successfully without reading from the underlying socket. + */ +ssize_t +pqsecure_bytes_pending(PGconn *conn) +{ +#ifdef USE_SSL + if (conn->ssl_in_use) + return pgtls_bytes_pending(conn); +#endif +#ifdef ENABLE_GSS + if (conn->gssenc) + return pg_GSS_bytes_pending(conn); +#endif + + /* Plaintext connections have no transport buffer. */ + return 0; +} + /* * Write data to a secure connection. * diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 461b39620c3..3f921207a14 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -827,6 +827,7 @@ extern int pqWriteReady(PGconn *conn); extern PostgresPollingStatusType pqsecure_open_client(PGconn *); extern void pqsecure_close(PGconn *); extern ssize_t pqsecure_read(PGconn *, void *ptr, size_t len); +extern ssize_t pqsecure_bytes_pending(PGconn *); extern ssize_t pqsecure_write(PGconn *, const void *ptr, size_t len); extern ssize_t pqsecure_raw_read(PGconn *, void *ptr, size_t len); extern ssize_t pqsecure_raw_write(PGconn *, const void *ptr, size_t len); @@ -863,9 +864,9 @@ extern void pgtls_close(PGconn *conn); extern ssize_t pgtls_read(PGconn *conn, void *ptr, size_t len); /* - * Is there unread data waiting in the SSL read buffer? + * Return the number of bytes available in the transport buffer. */ -extern bool pgtls_read_pending(PGconn *conn); +extern ssize_t pgtls_bytes_pending(PGconn *conn); /* * Write data to a secure connection. @@ -913,6 +914,7 @@ extern PostgresPollingStatusType pqsecure_open_gss(PGconn *conn); */ extern ssize_t pg_GSS_write(PGconn *conn, const void *ptr, size_t len); extern ssize_t pg_GSS_read(PGconn *conn, void *ptr, size_t len); +extern ssize_t pg_GSS_bytes_pending(PGconn *conn); #endif /* === in fe-trace.c === */ -- 2.47.3 [text/x-patch] v4-0002-libpq-Drain-all-pending-bytes-from-SSL-GSS-during.patch (12.0K, ../../[email protected]/3-v4-0002-libpq-Drain-all-pending-bytes-from-SSL-GSS-during.patch) download | inline diff: From 8383a00e7a2fe1f131416fb63e5e34fedf658243 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <[email protected]> Date: Fri, 3 Jul 2026 12:20:26 +0300 Subject: [PATCH v4 2/2] libpq: Drain all pending bytes from SSL/GSS during pqReadData() The previous commit strengthened a workaround for a hang when large messages are split across TLS records/GSS tokens. Because that workaround is implemented in libpq internals, it can only help us when libpq itself is polling on the socket. In nonblocking situations, where the client above libpq is expected to poll, the same bugs can show up. As a contrived example, consider a large protocol-2.0 error coming back from a server during PQconnectPoll(), split in an odd way across two records: -- TLS record (8192-byte payload) -- EEEE[...repeated a total of 8192 times] -- TLS record (8193-byte payload) -- EEEE[...repeated a total of 8192 times]\0 The first record will fill the first half of the libpq receive buffer, which is 16k long by default. The second record completely fills the last half with its first 8192 bytes, leaving the terminating NULL in the OpenSSL buffer. Since we still haven't seen the terminator at our level, PQconnectPoll() will return PGRES_POLLING_READING, expecting to come back when the server has sent "the rest" of the data. But there is nothing left to read from the socket; OpenSSL had to pull all of the data in the 8193-byte record off of the wire to decrypt it. A real server would probably not split up the records this way, nor keep the connection open after sending a fatal connection error. But servers that regularly use larger TLS records can get the libpq receive buffer into the same state if DataRows are big enough, as reported on the list. While the PostgreSQL server doesn't use larger TLS records like that, other non-PostgreSQL servers that implement the wire protocol are known to do that, as well as proxies that sit between the server and the client This is a layering violation. libpq makes decisions based on data in the application buffer, above the transport buffer (whether SSL or GSS), but clients are polling the socket below the transport buffer. One way to fix this in a backportable way, without changing APIs too much, is to ensure data never stays in the transport buffer. Then pqReadData's postconditions will look similar for both raw sockets and SSL/GSS: any available data is either in the application buffer, or still on the socket. Building on the prior commit, make pqReadData() to drain all pending data from the transport layer into conn->inBuffer, expanding the buffer as necessary. This is not particularly efficient from an architectural perspective (the pqsecure_read() implementations take care to fit their packets into the current buffer, and that effort is now completely discarded), but it's hopefully easier to reason about than a full rewrite would be for the back branches. Author: Jacob Champion <[email protected]> Reviewed-by: Mark Dilger <[email protected]> Reviewed-by: solai v <[email protected]> Reported-by: Lars Kanis <[email protected]> Discussion: https://postgr.es/m/2039ac58-d3e0-434b-ac1a-2a987f3b4cb1%40greiz-reinsdorf.de Backpatch-through: 14 --- src/interfaces/libpq/fe-misc.c | 145 ++++++++++++++++++++++- src/interfaces/libpq/fe-secure-openssl.c | 4 +- src/interfaces/libpq/fe-secure.c | 3 +- 3 files changed, 148 insertions(+), 4 deletions(-) diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c index 58ccca1e393..dd772838005 100644 --- a/src/interfaces/libpq/fe-misc.c +++ b/src/interfaces/libpq/fe-misc.c @@ -55,6 +55,8 @@ static int pqPutMsgBytes(const void *buf, size_t len, PGconn *conn); static int pqSendSome(PGconn *conn, int len); static int pqSocketCheck(PGconn *conn, int forRead, int forWrite, pg_usec_time_t end_time); +static int pqReadData_internal(PGconn *conn); +static int pqDrainPending(PGconn *conn); /* * PQlibVersion: return the libpq version number @@ -593,6 +595,13 @@ pqPutMsgEnd(PGconn *conn) /* ---------- * pqReadData: read more data, if any is available + * + * Upon a successful return, callers may assume that either 1) all available + * bytes have been consumed from the socket, or 2) the socket is still marked + * readable by the OS. (In other words: after a successful pqReadData, it's + * safe to tell a client to poll for readable bytes on the socket without any + * further draining of the SSL/GSS transport buffers.) + * * Possible return values: * 1: successfully loaded at least one more byte * 0: no data is presently available, but no error detected @@ -605,8 +614,7 @@ pqPutMsgEnd(PGconn *conn) int pqReadData(PGconn *conn) { - int someread = 0; - int nread; + int available; if (conn->sock == PGINVALID_SOCKET) { @@ -614,6 +622,40 @@ pqReadData(PGconn *conn) return -1; } + available = pqReadData_internal(conn); + if (available < 0) + return -1; + else if (available > 0) + { + /* + * Make sure there are no bytes stuck in layers between conn->inBuffer + * and the socket, to make it safe for clients to poll on PQsocket(). + */ + if (pqDrainPending(conn)) + return -1; + } + else + { + /* + * If we're not returning any bytes from the underlying transport, + * that must imply there aren't any in the transport buffer... + */ + Assert(pqsecure_bytes_pending(conn) == 0); + } + + return available; +} + +/* + * Workhorse for pqReadData(). It's kept separate from the pqDrainPending() + * logic to avoid adding to this function's goto complexity. + */ +static int +pqReadData_internal(PGconn *conn) +{ + int someread = 0; + int nread; + /* Left-justify any data in the buffer to make room */ if (conn->inStart < conn->inEnd) { @@ -800,6 +842,105 @@ definitelyFailed: return -1; } +/*--- + * Drain any transport data that is already buffered in userspace and add it + * to conn->inBuffer, enlarging inBuffer if necessary. The drain fails if + * inBuffer cannot be made to hold all available transport data. + * + * We assume that the underlying secure transport implementation does not + * attempt to read any more data from the socket while draining the transport + * buffer. After a successful return, pqsecure_bytes_pending() must be zero. + * + * This operation is necessary to prevent deadlock, due to a layering + * violation designed into our asynchronous client API: pqReadData() and all + * the parsing routines above it receive data from the SSL/GSS transport + * buffer, but clients poll on the raw PQsocket() handle. So data can be + * "lost" in the intermediate layer if we don't take it out here. + * + * To illustrate what we're trying to prevent, say that the server is sending + * two messages at once in response to a query (Aaaa and Bb), the libpq buffer + * is five characters in size, and TLS records max out at three-character + * payloads. Here's what would happen if pqReadData() didn't call + * pqDrainPending(): + * + * Client libpq SSL Socket + * | | | | + * | [ ] [ ] [ ] [1] Buffers are empty, client is + * x --------------------------> | polling on socket + * | | | | + * | [ ] [ ] [xxx] [2] First record is received; poll + * | <-------------------------- | signals read-ready + * | | | | + * x ---> [ ] [ ] [xxx] [3] Client calls PQconsumeInput() + * | | | | + * | [ ] -> [ ] [xxx] [4] libpq calls pqReadData() to fill + * | | | | the receive buffer + * | [ ] [Aaa] <-- [ ] [5] SSL pulls payload off the wire + * | | | | and decrypts it + * | [Aaa ] <- [ ] [ ] [6] pqsecure_read() takes all data + * | | | | + * | <--- [Aaa ] [ ] [ ] [7] PQconsumeInput() returns with a + * x --------------------------> | partial message, PQisBusy() is + * | | | | still true, client polls again + * | [Aaa ] [ ] [xxx] [8] Second record is received; poll + * | <-------------------------- | signals read-ready + * | | | | + * x ---> [Aaa ] [ ] [xxx] [9] Client calls PQconsumeInput() + * | | | | + * | [Aaa ] -> [ ] [xxx] [10] libpq calls pqReadData() to fill + * | | | | the receive buffer + * | [Aaa ] [aBb] <-- [ ] [11] SSL decrypts + * | | | | + * | [AaaaB] <- [b ] [ ] [12] pqsecure_read() fills its + * | | | | buffer, taking only two bytes + * | <--- [AaaaB] [b ] [ ] [13] PQconsumeInput() returns with a + * | | | | complete message buffered; + * | | | | PQisBusy() is false + * x ---> [AaaaB] [b ] [ ] [14] Client calls PQgetResult() + * | | | | + * | <--- [B ] [b ] [ ] [15] Aaaa is returned; PQisBusy() is + * x --------------------------> | true and client polls again + * . | | . + * . [B ] [b ] . [16] No packets, and client hangs. + * . | | . + * + * The pqDrainPending() call fixes the above scenario at step [13]. Before + * returning to the Client, it first expands the libpq buffer and moves the + * remaining data from the SSL buffer to the libpq buffer. + * + * The function returns 0 on success and -1 on error. Success means that + * there was no data pending or it was successfully drained to conn->inBuffer. + * On error, conn->errorMessage is set. + */ +static int +pqDrainPending(PGconn *conn) +{ + ssize_t bytes_pending; + ssize_t nread; + + bytes_pending = pqsecure_bytes_pending(conn); + if (bytes_pending <= 0) + return bytes_pending; + + /* Expand the input buffer if necessary. */ + if (pqCheckInBufferSpace(conn->inEnd + (size_t) bytes_pending, conn)) + return -1; /* errorMessage already set */ + + nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd, + bytes_pending); + conn->inEnd += nread; + + /* When there are bytes pending, the read function is not supposed to fail */ + if (nread != bytes_pending) + { + libpq_append_conn_error(conn, + "drained only %zu of %zd pending bytes in transport buffer", + nread, bytes_pending); + return -1; + } + return 0; +} + /* * pqSendSome: send data waiting in the output buffer. * diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 1b22ba7b7f6..f8b2184a1ce 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -236,7 +236,9 @@ pgtls_bytes_pending(PGconn *conn) int pending; /* - * OpenSSL readahead is documented to break SSL_pending(). + * OpenSSL readahead is documented to break SSL_pending(). Plus, we can't + * afford to have OpenSSL take bytes off the socket without processing + * them; that breaks the postconditions for pqsecure_drain_pending(). */ Assert(!SSL_get_read_ahead(conn->ssl)); diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index 907cdb9ea39..70faf8b2fe0 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -247,7 +247,8 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len) * Return the number of bytes available in the transport buffer. * * If pqsecure_read() is called for this number of bytes, it's guaranteed to - * return successfully without reading from the underlying socket. + * return successfully without reading from the underlying socket. See + * pqDrainPending() for a more complete discussion of the concepts involved. */ ssize_t pqsecure_bytes_pending(PGconn *conn) -- 2.47.3 ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2026-07-07 15:58 Heikki Linnakangas <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Heikki Linnakangas @ 2026-07-07 15:58 UTC (permalink / raw) To: solai v <[email protected]>; Jacob Champion <[email protected]>; +Cc: Mark Dilger <[email protected]>; Andres Freund <[email protected]>; Lars Kanis <[email protected]>; [email protected]; Merlin Moncure <[email protected]> On 03/07/2026 12:25, Heikki Linnakangas wrote: > On 14/05/2026 07:32, solai v wrote: >> Hi, >> i tested the latest V3 patch series on current postgreSQL HEAD with >> SSL enabled.The patches applied cleanly,build succeeded,and the server >> started successfully.I ran the async libpq test repeatedly and also >> tested large COPY TO STDOUT operations over SSL.I did not observe >> hangs,crashes,or visible regressions during testing. > > Thanks for the testing! > > Here's another patch version, with some small comment and commit message > changes, and I split the changes slightly differently between the two > patches. End result is otherwise the same. > > I plan to commit this in the next few days. Committed and backpatched to all supported versions. Thanks everyone! - Heikki ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2026-07-08 19:16 Peter Eisentraut <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Peter Eisentraut @ 2026-07-08 19:16 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; solai v <[email protected]>; Jacob Champion <[email protected]>; +Cc: Mark Dilger <[email protected]>; Andres Freund <[email protected]>; Lars Kanis <[email protected]>; [email protected]; Merlin Moncure <[email protected]> On 07.07.26 17:58, Heikki Linnakangas wrote: > On 03/07/2026 12:25, Heikki Linnakangas wrote: >> On 14/05/2026 07:32, solai v wrote: >>> Hi, >>> i tested the latest V3 patch series on current postgreSQL HEAD with >>> SSL enabled.The patches applied cleanly,build succeeded,and the >>> server started successfully.I ran the async libpq test repeatedly and >>> also tested large COPY TO STDOUT operations over SSL.I did not >>> observe hangs,crashes,or visible regressions during testing. >> >> Thanks for the testing! >> >> Here's another patch version, with some small comment and commit >> message changes, and I split the changes slightly differently between >> the two patches. End result is otherwise the same. >> >> I plan to commit this in the next few days. > > Committed and backpatched to all supported versions. Thanks everyone! In pqDrainPending(), there is nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd, bytes_pending); conn->inEnd += nread; But pqsecure_read() can return -1 for error. So adding that to conn->inEnd at that point seems wrong. There is error handling in the following code, but this would still kind of corrupt the conn->inEnd value? /* When there are bytes pending, the read function is not supposed to fail */ if (nread != bytes_pending) { libpq_append_conn_error(conn, "drained only %zu of %zd pending bytes in transport buffer", nread, bytes_pending); return -1; } I'm not sure if that comment means that a -1 return cannot happen? Or just that the whole error check should not happen? Also note that the format placeholder for nread is wrong. If you do get a -1, it will print some large unsigned value. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2026-07-09 12:55 Heikki Linnakangas <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Heikki Linnakangas @ 2026-07-09 12:55 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; solai v <[email protected]>; Jacob Champion <[email protected]>; +Cc: Mark Dilger <[email protected]>; Andres Freund <[email protected]>; Lars Kanis <[email protected]>; [email protected]; Merlin Moncure <[email protected]> On 08/07/2026 22:16, Peter Eisentraut wrote: > In pqDrainPending(), there is > > nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd, > bytes_pending); > conn->inEnd += nread; > > But pqsecure_read() can return -1 for error. So adding that to conn- > >inEnd at that point seems wrong. > > There is error handling in the following code, but this would still kind > of corrupt the conn->inEnd value? > > /* When there are bytes pending, the read function is not supposed > to fail */ > if (nread != bytes_pending) > { > libpq_append_conn_error(conn, > "drained only %zu of %zd pending bytes > in transport buffer", > nread, bytes_pending); > return -1; > } > > I'm not sure if that comment means that a -1 return cannot happen? Or > just that the whole error check should not happen? The comment in pgsecure_bytes_pending() says: > * If pqsecure_read() is called for this number of bytes, it's guaranteed to > * return successfully without reading from the underlying socket. See > * pqDrainPending() for a more complete discussion of the concepts involved. so as long as pqsecure_read() and pqsecure_read_pending() honor that contract, pqsecure_read() should not return an error. I agree that's a bit sloppy though, we should still check the return value. The intention was also that it cannot do a short read, but that wasn't quite clear from the comment either. > Also note that the format placeholder for nread is wrong. If you do get > a -1, it will print some large unsigned value. Good catch. Patch attached to clean up all that. - Heikki Attachments: [text/x-patch] 0001-libpq-Make-error-checks-in-the-new-buffer-draining-c.patch (2.6K, ../../[email protected]/2-0001-libpq-Make-error-checks-in-the-new-buffer-draining-c.patch) download | inline diff: From be68e94f81d8bb830cfda6eee6a20a41ec335933 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <[email protected]> Date: Thu, 9 Jul 2026 15:51:30 +0300 Subject: [PATCH 1/1] libpq: Make error checks in the new buffer draining code more robust Check explicitly for pqsecure_read() returning an error. It shouldn't fail, and we would've caught it in the check for a short read, but better to be explicit so that the error message is more informative. We also shouldn't update 'inEnd' when the read fails, although that too is just pro forma as we will bail out and close the connection on error. Reported-by: Peter Eisentraut <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected] --- src/interfaces/libpq/fe-misc.c | 11 ++++++++--- src/interfaces/libpq/fe-secure.c | 5 +++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c index dd772838005..f11b58bf9c7 100644 --- a/src/interfaces/libpq/fe-misc.c +++ b/src/interfaces/libpq/fe-misc.c @@ -928,13 +928,18 @@ pqDrainPending(PGconn *conn) nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd, bytes_pending); - conn->inEnd += nread; - /* When there are bytes pending, the read function is not supposed to fail */ + /* + * When there are bytes pending, pqsecure_read() is not supposed to fail + * or do a short read, but let's check anyway to be safe. + */ + if (nread < 0) + return -1; + conn->inEnd += nread; if (nread != bytes_pending) { libpq_append_conn_error(conn, - "drained only %zu of %zd pending bytes in transport buffer", + "drained only %zd of %zd pending bytes in transport buffer", nread, bytes_pending); return -1; } diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index 70faf8b2fe0..1a8e2e6746e 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -247,8 +247,9 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len) * Return the number of bytes available in the transport buffer. * * If pqsecure_read() is called for this number of bytes, it's guaranteed to - * return successfully without reading from the underlying socket. See - * pqDrainPending() for a more complete discussion of the concepts involved. + * return successfully with the same number of bytes, without reading from the + * underlying socket. See pqDrainPending() for a more complete discussion of + * the concepts involved. */ ssize_t pqsecure_bytes_pending(PGconn *conn) -- 2.47.3 ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2026-07-09 15:06 Daniel Gustafsson <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 0 replies; 10+ messages in thread From: Daniel Gustafsson @ 2026-07-09 15:06 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: solai v <[email protected]>; Jacob Champion <[email protected]>; Mark Dilger <[email protected]>; Andres Freund <[email protected]>; Lars Kanis <[email protected]>; PostgreSQL Hackers <[email protected]>; Merlin Moncure <[email protected]> > On 7 Jul 2026, at 17:58, Heikki Linnakangas <[email protected]> wrote: > Committed and backpatched to all supported versions. Thanks everyone! Sorry for coming to this thread late. When compiling against a local LibreSSL on macOS I get an error on INT_MAX missing: ../src/interfaces/libpq/fe-secure-openssl.c:253:22: error: use of undeclared identifier 'INT_MAX' else if (pending == INT_MAX) ^ 1 error generated. AFAICS it's because openssl/conf.h includes openssl/ossl_typ.h which in turn includes <limits.h> but the LibreSSL counterpart does not. That being said, it clearly works in the buildfarm where we have LibreSSL animals on OpenBSD so it must get pulled in indirectly from somewhere else there. This might be isolated to macOS builds, but I don't have other systems with LibreSSL handy so not sure. My LibreSSL installation is a libressl_portable locally compiled which further may explain differences? Any objections to the attached to make compilation pass when building against LibreSSL on macOS (and possibly elsewhere)? -- Daniel Gustafsson Attachments: [application/octet-stream] 0001-ssl-Include-limits.h-to-get-INT_MAX-when-using-Libre.patch (1.0K, ../../[email protected]/2-0001-ssl-Include-limits.h-to-get-INT_MAX-when-using-Libre.patch) download | inline diff: From 731f58640688005edfaf981b446ae1392e1996ba Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Thu, 9 Jul 2026 17:00:46 +0200 Subject: [PATCH] ssl: Include limits.h to get INT_MAX when using LibreSSL When compiling against OpenSSL, the <limits.h> header is indirectly included via openssl/ossl_typ.h from openssl/conf.h, but the LibreSSL version of ossl_typ.h does not include <limits.h> which cause compiler failure due to missing symbol (since ffd080d94fe). Fix by explicitly including <limits.h>. --- src/interfaces/libpq/fe-secure-openssl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index f8b2184a1ce..c7651c98ab5 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -25,6 +25,7 @@ #include <signal.h> #include <fcntl.h> #include <ctype.h> +#include <limits.h> #include "libpq-fe.h" #include "fe-auth.h" -- 2.39.3 (Apple Git-146) ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: libpq: Process buffered SSL read bytes to support records >8kB on async API @ 2026-07-09 15:52 Heikki Linnakangas <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Heikki Linnakangas @ 2026-07-09 15:52 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; solai v <[email protected]>; Jacob Champion <[email protected]>; Daniel Gustafsson <[email protected]>; +Cc: Mark Dilger <[email protected]>; Andres Freund <[email protected]>; Lars Kanis <[email protected]>; [email protected]; Merlin Moncure <[email protected]> On 09/07/2026 15:55, Heikki Linnakangas wrote: > On 08/07/2026 22:16, Peter Eisentraut wrote: >> In pqDrainPending(), there is >> >> nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd, >> bytes_pending); >> conn->inEnd += nread; >> >> But pqsecure_read() can return -1 for error. So adding that to conn- >> >inEnd at that point seems wrong. >> >> There is error handling in the following code, but this would still >> kind of corrupt the conn->inEnd value? >> >> /* When there are bytes pending, the read function is not >> supposed to fail */ >> if (nread != bytes_pending) >> { >> libpq_append_conn_error(conn, >> "drained only %zu of %zd pending >> bytes in transport buffer", >> nread, bytes_pending); >> return -1; >> } >> >> I'm not sure if that comment means that a -1 return cannot happen? Or >> just that the whole error check should not happen? > > The comment in pgsecure_bytes_pending() says: > >> * If pqsecure_read() is called for this number of bytes, it's >> guaranteed to >> * return successfully without reading from the underlying socket. See >> * pqDrainPending() for a more complete discussion of the concepts >> involved. > > so as long as pqsecure_read() and pqsecure_read_pending() honor that > contract, pqsecure_read() should not return an error. I agree that's a > bit sloppy though, we should still check the return value. > > The intention was also that it cannot do a short read, but that wasn't > quite clear from the comment either. > >> Also note that the format placeholder for nread is wrong. If you do >> get a -1, it will print some large unsigned value. > > Good catch. > > Patch attached to clean up all that. Committed that, and Daniel's #include fix for LibreSSL too. - Heikki ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2026-07-09 15:52 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-06-22 22:13 [PATCH 4/4] Update functions to pass only errinfo struct.. Justin Pryzby <[email protected]> 2024-09-10 18:49 Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Jacob Champion <[email protected]> 2024-09-11 19:08 ` Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Lars Kanis <[email protected]> 2024-09-11 23:00 ` Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Jacob Champion <[email protected]> 2026-07-03 09:25 ` Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Heikki Linnakangas <[email protected]> 2026-07-07 15:58 ` Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Heikki Linnakangas <[email protected]> 2026-07-08 19:16 ` Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Peter Eisentraut <[email protected]> 2026-07-09 12:55 ` Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Heikki Linnakangas <[email protected]> 2026-07-09 15:52 ` Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Heikki Linnakangas <[email protected]> 2026-07-09 15:06 ` Re: libpq: Process buffered SSL read bytes to support records >8kB on async API Daniel Gustafsson <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox