Re: psql leaks memory on query cancellation

On 13 April 2018 at 05:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> 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?

That seems reasonable.

It's all dependent on tons of internal details of how libc decides to
lay out memory, how it gets memory from the kernel, etc. On some
platforms that "island" won't affect anything anyway, and on others I
expect that freeing it probably still won't release memory back to the
OS. But if it helps on even one common platform and has no downsides,
it's well worth it.

BTW, https://stackoverflow.com/q/2215259/398670 is relevant.

This makes me wonder if we could do something cheeky like mmap
MAP_ANONYMOUS'ing /dev/zero ourselves, giving ourselves a separate
memory arena for result sets that we can reset when we're done. We
don't know which result sets will be big though, so it hardly seems
worth it.

>
> 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.

Huh. I'd noticed that behaviour, but it never crossed my care
threshold. I think I tend to do just what you describe below
habitually.

> 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.

Definitely separate.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services