sql: remove EvalContext.ActiveMemAcc #34935
Merged
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 |
jordanlewis
force-pushed
the
rm-activememacc
branch
2 times, most recently
from
e248e2a to
3bff9d1
Compare
jordanlewis
force-pushed
the
rm-activememacc
branch
from
3bff9d1 to
05cecf8
Compare
|
LGTM |
jordanlewis
force-pushed
the
rm-activememacc
branch
2 times, most recently
from
239aa4c to
8019ace
Compare
jordanlewis
force-pushed
the
rm-activememacc
branch
from
8019ace to
5b3785a
Compare
knz
approved these changes
Feb 21, 2019
There was a problem hiding this comment.
LGTM with a nit on the message presented to the user, see suggestion below
Reviewed 18 of 18 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @justinj, and @RaduBerinde)
pkg/sql/sem/builtins/builtins.go, line 72 at r1 (raw file):
errChrValueTooLarge = pgerror.NewErrorf(pgerror.CodeInvalidParameterValueError, "input value must be <= %d (maximum Unicode code point)", utf8.MaxRune) errStringTooLarge = pgerror.NewErrorf(pgerror.CodeProgramLimitExceededError, "requested length too large")
maybe NewErrorf(..., "result too large, value size exceeds %s", humanizeutil.IBytes(maxAllocatedStringsize)
this way the user knows more clearly what's going on.
|
I kept the message like that to achieve Postgres compatibility, but I'll take your advice. |
This memory account was used to track allocations performed by builtins that allocated memory, mainly string manipulation builtins. It was pretty hard to use this correctly, as the lifetimes of the account were never particularly clear. There were a couple of hacks in place to try to clear the memory at the right times, but they didn't work correctly, leading to crashes that were hard to diagnose. Rather than try to make this work perfectly, this commit removes the memory accounting for these builtins and replaces it with a hard cap on the size of string allocations - set arbitrarily to 64 MB at this time. Release note: None
jordanlewis
force-pushed
the
rm-activememacc
branch
from
5b3785a to
2b00f15
Compare
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Feb 21, 2019
34935: sql: remove EvalContext.ActiveMemAcc r=jordanlewis a=jordanlewis This memory account was used to track allocations performed by builtins that allocated memory, mainly string manipulation builtins. It was pretty hard to use this correctly, as the lifetimes of the account were never particularly clear. There were a couple of hacks in place to try to clear the memory at the right times, but they didn't work correctly, leading to crashes that were hard to diagnose. Rather than try to make this work perfectly, this commit removes the memory accounting for these builtins and replaces it with a hard cap on the size of string allocations - set arbitrarily to 64 MB at this time. Release note: None 35074: opt: logical props and stats for merge join r=RaduBerinde a=RaduBerinde Wire up the code for logical props and stats to work with merge joins. This is useful with exprgen. Release note: None 35075: opt: add parallel test for create stats r=RaduBerinde a=RaduBerinde Adding a test where three nodes run CREATE STATS multiple times (in parallel) on the same table. Fixes #34781. Release note: None Co-authored-by: Jordan Lewis <[email protected]> Co-authored-by: Radu Berinde <[email protected]>
Build succeeded |
This was referenced Feb 21, 2019
This was referenced Mar 4, 2019
thoszhang
pushed a commit
to thoszhang/cockroach
that referenced
this pull request
Mar 12, 2019
Add regression tests for a bug where backfilling a computed column or a column with a default value using a builtin function would cause a `memory budget exceeded` error (cockroachdb#34901). This was fixed by cockroachdb#34935. Release note: None
thoszhang
pushed a commit
to thoszhang/cockroach
that referenced
this pull request
Mar 12, 2019
Add regression tests for a bug where backfilling a computed column or a column with a default value using a builtin function would cause a `memory budget exceeded` error (cockroachdb#34901). This was fixed by cockroachdb#34935. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Mar 12, 2019
35565: sqlsmith: various improvements r=mjibson a=mjibson 35662: sql: add regression tests for memory accounting bug r=lucy-zhang a=lucy-zhang Add regression tests for a bug where backfilling a computed column or a column with a default value using a builtin function would cause a `memory budget exceeded` error (#34901). This was fixed by #34935. Release note: None Co-authored-by: Matt Jibson <[email protected]> Co-authored-by: Lucy Zhang <[email protected]>
This was referenced Mar 13, 2019
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.
This memory account was used to track allocations performed by builtins
that allocated memory, mainly string manipulation builtins. It was
pretty hard to use this correctly, as the lifetimes of the account were
never particularly clear.
There were a couple of hacks in place to try to clear the memory at the
right times, but they didn't work correctly, leading to crashes that
were hard to diagnose.
Rather than try to make this work perfectly, this commit removes the
memory accounting for these builtins and replaces it with a hard cap on
the size of string allocations - set arbitrarily to 64 MB at this time.
Release note: None