-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
sql: make aggregate builtins share the same memory account
Release justification: fixes for high-priority or high-severity bugs in existing functionality (we could hit memory limit due to accounting long before the available RAM is actually used up). We recently fixed a couple of leaks in memory accounting by aggregate builtins. It was done in the same way that other similar aggregates were doing - by instantiating a separate memory account for each aggregate struct. However, when a single aggregate, say `anyNotNull`, wants to store a single datum, say `DInt` of size 8 bytes, when growing its own memory account will actually make a reservation of `mon.DefaultPoolAllocation = 10240` bytes although we will only be using 8 bytes. This can result in "memory-starvation" for OLAP-y queries because we're likely to hit `max-sql-memory` limit long before we're getting close to it because of such "overestimation" in the accounting. This commit fixes this problem by making all aggregates that aggregate a single datum (these are all aggregate builtins that perform memory accounting except for `arrayAgg` which works with multiple datums) to share the same memory account (when non-nil) which is plumbed via `tree.EvalContext` (this is unfortunate but, as always, seems like necessary evil). That account is instantiated by `rowexec.aggregator` and `rowexec.windower` processors. Also it is acceptable from the concurrency's point of view because the processors run in a single goroutine, so we will never have concurrent calls to grow/shrink this shared memory account. If for some reason the field for shared memory account is nil in the eval context, then we will fallback to old behavior of instantiating a memory account for each aggregate builtin struct. A helper struct was created (that is now embedded by all aggregate builtins in question) that unifies the memory accounting. Release note: None (a follow-up to a recent PR).
- Loading branch information
1 parent
0e9f07a
commit fdbb7f7
Showing
5 changed files
with
200 additions
and
96 deletions.
There are no files selected for viewing
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 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 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
Oops, something went wrong.