util/log: ensure that secondary loggers do not leak memory #41231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
This was referenced Oct 1, 2019
petermattis
approved these changes
Oct 1, 2019
There was a problem hiding this comment.
I wonder if this change should make getBuffer and putBuffer globals, instead of methods on loggerT. That would ensure you didn't miss any wrong uses. Those methods could still use the logging global, or you could move freeList{,Mu} out of loggingT as well.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @petermattis)
this is my preferred solution. Is there a reason not to do this? |
Because it's redundant work with #40993. WDYT? |
|
oh well I'll change it anyway it's not so much work |
Prior to this patch, logging via a secondary logger would allocate a buffer, then add it to the buffer free list of the secondary logger. This was causing a memory leak because only the free list from the main logger is used to allocate buffers (even in secondary loggers), so all the now-unused buffers from secondary logs would remain unused and accumulate, locked from Go's GC attention because they are referenced somewhere. Release justification: bug fix Release note (bug fix): A memory leak was fixed that affected secondary logging (SQL audit logs, statement execution, and RocksDB logging).
knz
force-pushed
the
20191001-sec-log-leak
branch
from
2ff8e47
to
ab74a33
Compare
done |
|
bors r=petermattis,nvanbenschoten |
craig bot
pushed a commit
that referenced
this pull request
Oct 1, 2019
41220: sql: move histogram decoding into the stats cache r=RaduBerinde a=RaduBerinde Currently the stats cache exposes the encoded histogram data, and the opt catalog decodes it. The problem is that opt catalog objects are not shared across connections so this is inefficient (especially in memory usage). This change moves the decoding into the stats cache. The opt catalog objects now simply point to the data computed by the cache. There are still inefficiencies to improve on in the future: the opt catalog can hold on to multiple versions of tables, so we will keep many versions of histograms "alive". Informs #41206. Informs #40922. Release justification: fix for new functionality. Release note: None 41231: util/log: ensure that secondary loggers do not leak memory r=petermattis,nvanbenschoten a=knz Fixes #41230. Note: the primary cause of this issue is removed by #40993 but that PR is blocked until 19.2 is out. I'm cherry-picking the subset of those changes sufficient to solve issue #41230, here. Prior to this patch, logging via a secondary logger would allocate a buffer, then add it to the buffer free list of the secondary logger. This was causing a memory leak because only the free list from the main logger is used to allocate buffers (even in secondary loggers), so all the now-unused buffers from secondary logs would remain unused and accumulate, locked from Go's GC attention because they are referenced somewhere. Release justification: bug fix Release note (bug fix): A memory leak was fixed that affected secondary logging (SQL audit logs, statement execution, and RocksDB logging). Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
nvanbenschoten
approved these changes
Oct 1, 2019
There was a problem hiding this comment.
Reviewable status:
Build succeeded |
This was referenced Oct 2, 2019
craig bot
pushed a commit
that referenced
this pull request
Oct 17, 2019
40993: util/log: refactor and bug fixes r=knz a=knz Fixes #40983. Fixes #40973. Fixes #40974. Fixes #41231. I might also want to add code to clean up #40972, #40982 and #40990 provided I receive some guidance from @tbg or someone familiar with the log package. 41654: parser: fix telemetry link for materialized views r=jordanlewis a=jordanlewis This commit fixes the error message that you get if you try to create a materialized view to point at the right GitHub issue. Release note: None Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #41230.
Note: the primary cause of this issue is removed by #40993 but that PR is blocked until 19.2 is out. I'm cherry-picking the subset of those changes sufficient to solve issue #41230, here.
Prior to this patch, logging via a secondary logger would allocate a
buffer, then add it to the buffer free list of the secondary logger.
This was causing a memory leak because only the free list from the
main logger is used to allocate buffers (even in secondary loggers),
so all the now-unused buffers from secondary logs would remain unused
and accumulate, locked from Go's GC attention because they are
referenced somewhere.
Release justification: bug fix
Release note (bug fix): A memory leak was fixed that affected
secondary logging (SQL audit logs, statement execution, and RocksDB
logging).