Skip to content

Commit

Permalink
[#7070] ysql: Import 'Fix ancient memory leak in contrib/auto_explain.'
Browse files Browse the repository at this point in the history
Summary:
Upstream commit was 5c0f7cc5442108e113d4fb88c952329b467e2c6a

Commit message was:
```
    The ExecutorEnd hook is invoked in a context that could be quite
    long-lived, not the executor's own per-query context as I think
    we were sort of assuming.  Thus, any cruft generated while producing
    the EXPLAIN output could accumulate over multiple queries.  This can
    result in spectacular leakage if log_nested_statements is on, and
    even without that I'm surprised nobody complained before.

    To fix, just switch into the executor's context so that anything we
    allocate will be released when standard_ExecutorEnd frees the executor
    state.  We might as well nuke the code's retail pfree of the explain
    output string, too; that's laughably inadequate to the need.

    Japin Li, per report from Jeff Janes.  This bug is old, so
    back-patch to all supported branches.

    Discussion: https://postgr.es/m/CAMkU=1wCVtbeRn0s9gt12KwQ7PLXovbpM8eg25SYocKW3BT4hg@mail.gmail.com
```

Test Plan: Build Yugabyte DB and run test suite via Jenkins

Reviewers: jason

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D10529
  • Loading branch information
tedyu committed Feb 3, 2021
1 parent d79eeb1 commit 17de532
Showing 1 changed file with 9 additions and 2 deletions.
11 changes: 9 additions & 2 deletions src/postgres/contrib/auto_explain/auto_explain.c
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,15 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
{
if (queryDesc->totaltime && auto_explain_enabled() && current_query_sampled)
{
MemoryContext oldcxt;
double msec;

/*
* Make sure we operate in the per-query context, so any cruft will be
* discarded later during ExecutorEnd.
*/
oldcxt = MemoryContextSwitchTo(queryDesc->estate->es_query_cxt);

/*
* Make sure stats accumulation is done. (Note: it's okay if several
* levels of hook all do this.)
Expand Down Expand Up @@ -360,9 +367,9 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
(errmsg("duration: %.3f ms plan:\n%s",
msec, es->str->data),
errhidestmt(true)));

pfree(es->str->data);
}

MemoryContextSwitchTo(oldcxt);
}

if (prev_ExecutorEnd)
Expand Down

0 comments on commit 17de532

Please sign in to comment.