Re: psql leaks memory on query cancellation

I wrote:
> I imagine that this indicates that control-C processing allocates some
> memory it doesn't free, resulting in an "island" up at the end of memory
> that prevents glibc from releasing all the free memory it's got.  Whether
> that's an actual leak, or just memory we're holding onto in hopes of
> reusing it, isn't clear.  (valgrind might be useful.)

So I poked into this a little, and realized that the "island" is in
fact the error PGresult we get back from the server when the query is
cancelled.  We used to print that and throw it away ... but since the
addition of \errverbose, psql just sits on it, at least till the next
error.  Since that's up against the end of memory, it prevents freeing
all the space belonging to the huge query result we cancelled midway
through collection of.

The problem, therefore, is that libpq reads the error message and builds a
PGresult for it before it throws away the partially-collected query result
it was working on.  This is dumb.  Quite aside from the question of when
memory can be released back to the OS, we are putting ourselves at
unnecessary risk of OOM.

Therefore, I propose the attached patch, which simply sees to it that
we discard any partial query result at the start of error message
collection not the end.  This makes the behavior very much better,
at least on Linux.

I think this is a back-patchable bug fix; certainly so at least back
to 9.6 where \errverbose was added.  Versions before that do not show
the persistent memory bloat the OP is complaining of, so that what
we have here is arguably a performance regression.  Comments?

> BTW, I also notice that either psql or libpq seems to take a darn
> long time to release a several-GB-sized query result.  That might
> be improvable as well.

I looked into that too, and found that what I was seeing was that
if I exit my pager with "q", psql continues to run, formatting the
enormous query result and writing it to nowhere.  Since we've got
SIGPIPE disabled, nothing happens to stop it; although if you have
the presence of mind to press control-C, you do get control back
pretty quickly.  I wonder whether fe_utils/print.c ought to be
adjusted to stop on output errors, along the lines of

-        if (cancel_pressed)
+        if (cancel_pressed || ferror(fout))
            break;

However, the implications of that aren't entirely clear, so I merely
suggest that as a topic for research not an immediately proposed patch.

            regards, tom lane

diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index d3ca5d2..8345faf 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -880,6 +880,14 @@ pqGetErrorNotice3(PGconn *conn, bool isError)
     char        id;

     /*
+     * If this is an error message, pre-emptively clear any incomplete query
+     * result we may have.  We'd just throw it away below anyway, and
+     * releasing it before collecting the error might avoid out-of-memory.
+     */
+    if (isError)
+        pqClearAsyncResult(conn);
+
+    /*
      * Since the fields might be pretty long, we create a temporary
      * PQExpBuffer rather than using conn->workBuffer.  workBuffer is intended
      * for stuff that is expected to be short.  We shouldn't use
@@ -943,7 +951,7 @@ pqGetErrorNotice3(PGconn *conn, bool isError)
     {
         if (res)
             res->errMsg = pqResultStrdup(res, workBuf.data);
-        pqClearAsyncResult(conn);
+        pqClearAsyncResult(conn);    /* redundant, but be safe */
         conn->result = res;
         if (PQExpBufferDataBroken(workBuf))
             printfPQExpBuffer(&conn->errorMessage,