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.

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

rowexec: fix usage of the shared "single datum" acc in windower #89050

Merged
merged 1 commit into from Sep 30, 2022

Conversation

yuzefovich
Copy link
Member

This commit fixes the usage of the shared "single datum agg mem account" by the window builtins. Previously, we were updating the eval context with the memory account after the builtins were constructed. The impact of the bug on its own is pretty minor (namely that we could reserve more memory than necessary - the initial allocation is 10KiB), but I have some other changes brewing which make it more important to be precise about the attribution of the memory allocations.

Release note: None

This commit fixes the usage of the shared "single datum agg mem account"
by the window builtins. Previously, we were updating the eval context
with the memory account after the builtins were constructed. The impact
of the bug on its own is pretty minor (namely that we could reserve more
memory than necessary - the initial allocation is 10KiB), but I have
some other changes brewing which make it more important to be precise
about the attribution of the memory allocations.

Release note: None
@yuzefovich yuzefovich added backport-22.1.x 22.1 is EOL backport-22.2.x Flags PRs that need to be backported to 22.2. labels Sep 30, 2022
@yuzefovich yuzefovich requested review from DrewKimball and a team September 30, 2022 02:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

So, just to make sure I understand, the problem is that not setting the account causes a monitor to be created for each aggregate, which leads to an overestimate because we amortize the cost of accounting by reserving more than the exact amount requested. Is that right?

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@yuzefovich
Copy link
Member Author

Yes, pretty much. The only minor difference is that an account (not a monitor) is created for each aggregate builtin. Then, regardless of the memory usage of that particular builtin, once that separate account is grown, its initial allocation will be at least 10KiB. This was very problematic for the row-by-row hash aggregator which creates a separate builtin object for each bucket (i.e. each group), but for window functions we reuse the same builtin objects for each row. Still, if we have a query with 100 window functions, then we might reserve 1MiB from the monitor and only use a small fraction of that.

There is some more historical context in this PR description as well as this comment.

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 30, 2022

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Sep 30, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 982eb16 to blathers/backport-release-22.1-89050: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.


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

@yuzefovich yuzefovich removed backport-22.1.x 22.1 is EOL backport-22.2.x Flags PRs that need to be backported to 22.2. labels Sep 30, 2022
@yuzefovich yuzefovich deleted the fix-window-builtins branch September 30, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants