sem/builtins: account for internal memory of some aggregates #46545
Conversation
|
This change is |
yuzefovich
force-pushed
the
fix-aggs
branch
2 times, most recently
from
8ee4131
to
5bc1ed5
Compare
| a.sawNonNull = false | ||
| a.result.Reset() | ||
| a.acc.Empty(ctx) |
There was a problem hiding this comment.
This actually might be incorrect, will need to double check this tomorrow.
There was a problem hiding this comment.
The max aggregate doesn't reset its account here either
|
Can you share the memory profile that brought you to this conclusion? |
|
So Rohan has been staring at this: I looked over all aggregate functions in the same file and found another leak in |
| if a.val == tree.DNull && datum != tree.DNull { | ||
| a.val = datum | ||
| if err := a.acc.Grow(ctx, int64(datum.Size())); err != nil { |
There was a problem hiding this comment.
I think this should be ResizeTo, not Grow
There was a problem hiding this comment.
ack never mind, we are only going to call this once.
| a.sawNonNull = false | ||
| a.result.Reset() | ||
| a.acc.Empty(ctx) |
There was a problem hiding this comment.
The max aggregate doesn't reset its account here either
Release justification: fixes for high-priority or high-severity bugs in existing functionality. Previously, we were not accounting for the memory of the first non-null datum in `anyNotNull` aggregate. Similarly, we were not accounting for the byte slice in `bytesXor` aggregate. This is now fixed. This commit also fixes an issue with double accounting for the current datum in `min` and `max` aggregates on fixed size types (previously, it was reported in `Size()` method as well as separately on the first non-null datum). Additionally, `minAggregate` and `maxAggregate` have been unexported. Release note (bug fix): CockroachDB previously was incorrectly accounting for some RAM usage when computing aggregate functions which results in an underestimation of it and could cause an out of memory crash. Now this has been fixed.
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @rohany)
pkg/sql/sem/builtins/aggregate_builtins.go, line 521 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
ack never mind, we are only going to call this once.
Yep.
pkg/sql/sem/builtins/aggregate_builtins.go, line 766 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
The max aggregate doesn't reset its account here either
Done.
|
this LGTM, but i'd wait for someone else more knowledgeable in this area to agree |
There was a problem hiding this comment.
Reviewable status:
|
TFTRs! bors r+ |
Build succeeded |
Release justification: fixes for high-priority or high-severity bugs in
existing functionality.
Previously, we were not accounting for the memory of the first non-null
datum in
anyNotNullaggregate. Similarly, we were not accounting forthe byte slice in
bytesXoraggregate. This is now fixed.This commit also fixes an issue with double accounting for the current
datum in
minandmaxaggregates on fixed size types (previously, itwas reported in
Size()method as well as separately on the firstnon-null datum). Additionally,
minAggregateandmaxAggregatehavebeen unexported.
Addresses: #45812.
Release note (bug fix): CockroachDB previously was incorrectly
accounting for some RAM usage when computing aggregate functions which
results in an underestimation of it and could cause an out of memory
crash. Now this has been fixed.