"'Isidor Zeuner'" <postgresql@xxxxxxxxxxx> writes:
> using the latest git source code, I found that libpq will let the
> connection stall when getRowDescriptions breaks on an out of memory
> condition. I think this should better be handled differently to allow
> application code to handle such situations gracefully.
The basic assumption in there is that if we wait and retry, eventually
there will be enough memory.
I think the greatest problem with that approach is that there is no
(at least no documented) way for the application to find out that it
should be releasing memory.
I agree that that's not ideal, since the
application may not be releasing memory elsewhere. But what you propose
doesn't seem like an improvement: you're converting a maybe-failure into
a guaranteed-failure, and one that's much more difficult to recover from
than an ordinary query error.
I think it was an improvement considering that it puts the application
code back in control. Given that the application has some way to
handle connection and query errors, it can do something reasonable
about the situation. Before, the application had no way to find out
there is a (maybe-)failure at all.
Also, this patch breaks async operation, in which a failure return from
getRowDescriptions normally means that we have to wait for more data
to arrive. The test would really need to be inserted someplace else.
In any case, getRowDescriptions is really an improbable place for an
out-of-memory to occur: it would be much more likely to happen while
absorbing the body of a large query result.
My assumption was that there is not much logic to handle such
situations because it is improbable.
I am currently using PostGreSQL under memory-constrained conditions,
so I might be getting back with more such cases if they surface.
There already is some logic
in getAnotherTuple for dealing with that case, which I suggest is a
better model for what to do than "break the connection". But probably
making things noticeably better here would require going through all
the code to check for other out-of-memory cases, and developing some
more uniform method of representing an already-known-failed query
result. (For instance, it looks like getAnotherTuple might not work
very well if it fails to get memory for one tuple and then succeeds
on later ones. We probably ought to have some explicit state that
says "we are absorbing the remaining data traffic for a query result
that we already ran out of memory for".)
I like this approach. I changed the out-of-memory handling to switch
to a PGASYNC_MEMORY_FULL state, which will skip all messages until the
command is complete. Patch is attached.
Best regards,
Isidor Zeuner
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index fb4033d..3ca2e6b 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -157,10 +157,24 @@ pqParseInput3(PGconn *conn)
}
else if (conn->asyncStatus != PGASYNC_BUSY)
{
+ if (conn->asyncStatus == PGASYNC_MEMORY_FULL)
+ {
+ if (id == 'C')
+ {
+ pqClearAsyncResult(conn);
+ conn->result = PQmakeEmptyPGresult(conn, PGRES_FATAL_ERROR);
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("out of memory during parsing\n"));
+ pqSaveErrorResult(conn);
+ conn->asyncStatus = PGASYNC_READY;
+ }
+ conn->inCursor += msgLength;
+ }
/* If not IDLE state, just wait ... */
- if (conn->asyncStatus != PGASYNC_IDLE)
+ else if (conn->asyncStatus != PGASYNC_IDLE)
+ {
return;
-
+ }
/*
* Unexpected message in IDLE state; need to recover somehow.
* ERROR messages are displayed using the notice processor;
@@ -170,7 +184,7 @@ pqParseInput3(PGconn *conn)
* it is about to close the connection, so we don't want to just
* discard it...)
*/
- if (id == 'E')
+ else if (id == 'E')
{
if (pqGetErrorNotice3(conn, false /* treat as notice */ ))
return;
@@ -268,9 +282,14 @@ pqParseInput3(PGconn *conn)
if (conn->result == NULL ||
conn->queryclass == PGQUERY_DESCRIBE)
{
+ int const before = conn->inCursor;
/* First 'T' in a query sequence */
if (getRowDescriptions(conn))
- return;
+ {
+ conn->asyncStatus = PGASYNC_MEMORY_FULL;
+ conn->inCursor = before + msgLength;
+ break;
+ }
/*
* If we're doing a Describe, we're ready to pass the
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 31b517c..333fc7b 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -219,7 +219,8 @@ typedef enum
PGASYNC_READY, /* result ready for PQgetResult */
PGASYNC_COPY_IN, /* Copy In data transfer in progress */
PGASYNC_COPY_OUT, /* Copy Out data transfer in progress */
- PGASYNC_COPY_BOTH /* Copy In/Out data transfer in progress */
+ PGASYNC_COPY_BOTH, /* Copy In/Out data transfer in progress */
+ PGASYNC_MEMORY_FULL /* not enough memory for parsing */
} PGAsyncStatusType;
/* PGQueryClass tracks which query protocol we are now executing */
--
Sent via pgsql-general mailing list (pgsql-general@xxxxxxxxxxxxxx)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-general
