Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

querycache: unhook EvalContext from CachedData memo #57059

Closed
ajwerner opened this issue Nov 24, 2020 · 3 comments · Fixed by #57149
Closed

querycache: unhook EvalContext from CachedData memo #57059

ajwerner opened this issue Nov 24, 2020 · 3 comments · Fixed by #57149
Assignees
@RaduBerinde

Comments

@ajwerner
Copy link
Contributor

Describe the problem

When a query plan which is not fully optimized is cached, it retains a pointer to the *tree.EvalContext via the logicalPropBuilder. This retained reference can manifest as a relatively severe memory leak when those cached values end up retaining different SystemConfigs which can end up being rather large.

Environment:

This issue affects at least v20.1.x and v20.2.x. Ideally a fix would be available for backport.
@blathers-crl
Copy link

blathers-crl bot commented Nov 24, 2020

Hi @ajwerner, please add a C-ategory label to your issue. Check out the label system docs.

While you're here, please consider adding an A- label to help keep our repository tidy.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@ajwerner ajwerner mentioned this issue Nov 24, 2020
Closed
@RaduBerinde
Copy link
Member

Thanks for finding this. Yes it is wrong as a principle to hang on to the original EvalCtx. Definitely a bad idea to use the old one when we are reoptimizing - there might be a correctness issue here too, I'll see if I can find one.

@RaduBerinde
Copy link
Member

Thinking about it more, I don't think there is a correctness issue - that EvalCtx would never be used.

@RaduBerinde RaduBerinde mentioned this issue Nov 25, 2020
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Nov 30, 2020
@RaduBerinde
opt: don't hold on to evalCtx from detached Memo
7a41483 
This change adds more "cleanup" code when detaching a Memo (a detached
memo is stored in the query cache and is reused later in a "read-only"
fashion). In particular, we clear the EvalContext in
logicalPropsBuilder which can lead to inadvertently holding on to a
lot of memory.

Fixes cockroachdb#57059.

Release note: None
@RaduBerinde RaduBerinde mentioned this issue Nov 30, 2020
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Nov 30, 2020
@RaduBerinde
opt: don't hold on to evalCtx from detached Memo
b5b1eb4 
This change adds more "cleanup" code when detaching a Memo (a detached
memo is stored in the query cache and is reused later in a "read-only"
fashion). In particular, we clear the EvalContext in
logicalPropsBuilder which can lead to inadvertently holding on to a
lot of memory.

Fixes cockroachdb#57059.

Release note: None
@RaduBerinde RaduBerinde mentioned this issue Nov 30, 2020
Merged
craig bot pushed a commit that referenced this issue Nov 30, 2020
@RaduBerinde @mgartner @knz
Merge #57149 #57153 #57242 #57246
d5222dc 
57149: opt: don't hold on to evalCtx from detached Memo r=RaduBerinde a=RaduBerinde

This change adds more "cleanup" code when detaching a Memo (a detached
memo is stored in the query cache and is reused later in a "read-only"
fashion). In particular, we clear the EvalContext in
logicalPropsBuilder which can lead to inadvertently holding on to a
lot of memory.

Fixes #57059.

Release note: None

57153: optbuilder: fix ambiguous column references for FK cascades r=RaduBerinde a=mgartner

This commit fixes an issue in optbuilder that caused "ambiguous column
reference" errors. This error would be produced during cascading updates
if a child table's reference column name was equal to the parent column
name concatenated with `_new`, and the child table had a check
constraint, computed column, or partial index predicate that referenced
the column.

For example, the following `UPDATE` statement would produce an error.
The expected behavior is a successful `UPDATE`. Notice that `p_new` of
the child table references `p` of the parent table.

    CREATE TABLE parent (p INT PRIMARY KEY)

    CREATE TABLE child (
      c INT PRIMARY KEY,
      p_new INT REFERENCES parent(p) ON UPDATE CASCADE,
      CHECK (p_new > 0)
    )

    UPDATE parent SET p = p * 10 WHERE p > 1

This issue was the result of incorrect scoping while building foreign
key cascading update expressions. A column with the same name and column
ID was added to the update expression's input scope. Because the
`mutationBuilder.disambiguateColumns` function is unable to disambiguate
columns with the same name and column ID, building any expression that
referenced the duplicated column would result in an error.

This commit fixes the issue by no longer duplicating columns in the
update expression's input scope. `mutationBuilder.addUpdateCols` now
detects the special case when the update expression is a `*scopeColumn`
and avoids duplicating it in the generated projection scope.

Fixes #57148

Release note (bug fix): A bug has been fix that caused an "ambiguous
column reference" error during foreign key cascading updates. This error
was incorrectly produced when the child table's reference column name
was equal to the concatenation of the parent's reference column name and
"_new", and when the child table had a CHECK constraint, computed
column, or partial index predicate expression that referenced the
column. This bug was introduce in version 20.2.

57242: kvserver: avoid serving an unsafe string to log.Fatalf r=irfansharif a=knz

A linter change in #57134 made me discover this bug.

Release note: None

57246: rowenc: de-flake TestEncodeContainingArrayInvertedIndexSpans r=rytaft a=mgartner

`TestEncodeContainingArrayInvertedIndexSpans` was failing sporadically
because of randomized test cases that were incorrectly determining the
expected value for the `unique` return value from
`EncodeContainingInvertedIndexSpans`. The test was using the
`reflect.DeepEqual` function to check for `Datum` equality, which does
not return true in all cases where `Datum.Compare` returns `0`.

Fixes #57237

Release note: None

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@craig craig bot closed this as completed in a94f1d9 Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RaduBerinde RaduBerinde

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

opt: don't hold on to evalCtx from detached Memo RaduBerinde/cockroach
2 participants
@ajwerner @RaduBerinde