Re: psql leaks memory on query cancellation
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Craig Ringer <craig@2ndquadrant.com>
Cc: Darafei Komяpa Praliaskouski <me@komzpa.net>, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Date: 2018-04-12T21:10:15Z
Lists: pgsql-hackers
Attachments
- smarter-libpq-error-handling.patch (text/x-diff) patch
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
Commits
-
In libpq, free any partial query result before collecting a server error.
- bbec33c2d026 9.3.23 landed
- 3dd36aa4b36c 9.4.18 landed
- 2278e94ae941 9.5.13 landed
- 131f6a958360 9.6.9 landed
- d25c2ee9c038 11.0 landed
- d014b38df60f 10.4 landed