On Wed, 12 Nov 2025 16:20:13 +0900 Yugo Nagata <[email protected]> wrote:
> On Tue, 11 Nov 2025 20:16:11 +0900 > Fujii Masao <[email protected]> wrote: > > > On Tue, Nov 11, 2025 at 2:43 PM Tom Lane <[email protected]> wrote: > > > > > > Fujii Masao <[email protected]> writes: > > > > To address this, callers need a way to distinguish between > > > > PGRES_FATAL_ERROR > > > > and OOM. Functions that loop until PQgetResult() returns NULL should > > > > continue > > > > if the result is PGRES_FATAL_ERROR, but should break if it's an OOM. > > > > > > Not sure about that. We might or might not be able to make progress > > > if the caller keeps calling PQgetResult. But if it stops after an > > > OOM report then we might as well just close the connection, because > > > there is no hope of getting back in sync. > > > > > > I'm inclined to think that it's correct that we should return > > > OOM_result not NULL if we couldn't make a PGresult, but further > > > analysis is needed to make sure that libpq can make progress > > > afterwards. I don't think we should expect applications to > > > involve themselves in that recovery logic, because they won't, > > > and most certainly won't test it carefully. > > > > > > The whole project might be doomed to failure really. I think > > > that one very likely failure if we are approaching OOM is that > > > we can't enlarge libpq's input buffer enough to accept all of > > > a (long) incoming server message. What hope have we got of > > > getting out of that situation? > > > > When pqCheckInBufferSpace() fails to enlarge the input buffer and > > PQgetResult() > > returns PGRES_FATAL_ERROR, I noticed that PQgetResult() sets asyncStatus to > > PGASYNC_IDLE. This means subsequent calls to PQgetResult() will return NULL > > even in OOM case, so functions looping until NULL won't end up in an > > infinite loop. > > Of course, issuing another query on the same connection afterward could be > > problematic, though. > > > > I'm still not sure this behavior is correct, but I'm just wondering if > > asyncStatus should be reset to PGASYNC_IDLE also when OOM_result is > > returned. > > I agree that PQgetResult() should return NULL after returning OOM_result, and > resetting asyncStatus to PGASYNC_IDLE would work in the normal mode. > > I also agree that continuing to use the same connection seems problematic, > especially in pipeline mode; the pipeline status remains PQ_PIPELINE_OK, > PQgetResult() > never returns PGRES_PIPELINE_SYNC, and the sent commands are left in the > command queue. > > Therefore, I wonder about closing the connection and resetting the status > when OOM_result is retunred, by callling pqDropConnection() as > handleFatalError() does. > In this case, the connection status should also be set to CONNECTINO_BAD. > > This would prevent applications from continueing to use potentially > problamatic > connection without providing the way to distinguish OOM from other errors. > > What do you think? > > I've attached an updated patch in this direction. I'm sorry, the patch attached to my previous post was incorrect. I've attached the correct one. Regards, Yugo Nagata -- Yugo Nagata <[email protected]>
>From caff1c18a06e7876848d2719bad8b2e90774e3ff Mon Sep 17 00:00:00 2001 From: Yugo Nagata <[email protected]> Date: Tue, 11 Nov 2025 01:51:01 +0900 Subject: [PATCH v2] Make PQgetResult() not return NULL on out-of-memory error Previously, PQgetResult() returned NULL not only when no results remained for a sent query, but also when an out-of-memory error occurred, except when PGconn itself was NULL. As a result, users could not distinguish between query completion and an out-of-memory error when PQgetResult() returned NULL. This commit changes PQgetResult() to not return NULL in the case of an out-of-memory error by modifying getCopyResult() so that it never returns NULL, but instead returns OOM_result, as pqPipelineProcessQueue() does. --- src/interfaces/libpq/fe-exec.c | 45 +++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index 0b1e37ec30b..8ca306687bf 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -905,6 +905,10 @@ pqPrepareAsyncResult(PGconn *conn) */ res = unconstify(PGresult *, &OOM_result); + pqDropConnection(conn, true); + conn->status = CONNECTION_BAD; /* No more connection to backend */ + conn->asyncStatus = PGASYNC_IDLE; + /* * Don't advance errorReported. Perhaps we'll be able to report * the text later. @@ -2061,8 +2065,7 @@ PQisBusy(PGconn *conn) /* * PQgetResult * Get the next PGresult produced by a query. Returns NULL if no - * query work remains or an error has occurred (e.g. out of - * memory). + * query work remains. * * In pipeline mode, once all the result of a query have been returned, * PQgetResult returns NULL to let the user know that the next @@ -2170,7 +2173,12 @@ PQgetResult(PGconn *conn) pqCommandQueueAdvance(conn, false, res->resultStatus == PGRES_PIPELINE_SYNC); - if (conn->pipelineStatus != PQ_PIPELINE_OFF) + if ((const PGresult *) res == &OOM_result) + { + /* In the OOM case, set the state to IDLE */ + conn->asyncStatus = PGASYNC_IDLE; + } + else if (conn->pipelineStatus != PQ_PIPELINE_OFF) { /* * We're about to send the results of the current query. Set @@ -2200,8 +2208,16 @@ PQgetResult(PGconn *conn) break; case PGASYNC_READY_MORE: res = pqPrepareAsyncResult(conn); - /* Set the state back to BUSY, allowing parsing to proceed. */ - conn->asyncStatus = PGASYNC_BUSY; + if ((const PGresult *) res == &OOM_result) + { + /* In the OOM case, set the state to IDLE */ + conn->asyncStatus = PGASYNC_IDLE; + } + else + { + /* Set the state back to BUSY, allowing parsing to proceed. */ + conn->asyncStatus = PGASYNC_BUSY; + } break; case PGASYNC_COPY_IN: res = getCopyResult(conn, PGRES_COPY_IN); @@ -2234,6 +2250,8 @@ PQgetResult(PGconn *conn) static PGresult * getCopyResult(PGconn *conn, ExecStatusType copytype) { + PGresult *res; + /* * If the server connection has been lost, don't pretend everything is * hunky-dory; instead return a PGRES_FATAL_ERROR result, and reset the @@ -2254,7 +2272,22 @@ getCopyResult(PGconn *conn, ExecStatusType copytype) return pqPrepareAsyncResult(conn); /* Otherwise, invent a suitable PGresult */ - return PQmakeEmptyPGresult(conn, copytype); + res = PQmakeEmptyPGresult(conn, copytype); + + /* + * Return the static OOM_result if out-of-memory. See the comments + * in pqPrepareAsyncResult(). + */ + if (!res) + { + res = unconstify(PGresult *, &OOM_result); + + pqDropConnection(conn, true); + conn->status = CONNECTION_BAD; /* No more connection to backend */ + conn->asyncStatus = PGASYNC_IDLE; + } + + return res; } -- 2.43.0
