Skip to content

Check total memory limit when allocating user memory#15723

Merged
rschlussel merged 1 commit intoprestodb:masterfrom
rschlussel:user-total
Feb 17, 2021
Merged

Check total memory limit when allocating user memory#15723
rschlussel merged 1 commit intoprestodb:masterfrom
rschlussel:user-total

Conversation

@rschlussel
Copy link
Contributor

This prevents queries from exceeding the total memory limit between the
time that user memory is allocated, and the next system memory
allocation.

Test plan - unit test

== RELEASE NOTES ==

General Changes
* Enforce ``max_total_memory_per_node`` on user memory allocation

Copy link
Contributor

@pettyjamesm pettyjamesm left a comment

Choose a reason for hiding this comment

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

Makes sense as a matter of closing the gaps of the current implementation. I do wish this code wasn’t so fragile in regard to potential deadlocks and lock contention or that it didn’t use a “read then update” pattern on the memory pool to avoid it- but that’s definitely a larger refactor for another day.

@rschlussel
Copy link
Contributor Author

Makes sense as a matter of closing the gaps of the current implementation. I do wish this code wasn’t so fragile in regard to potential deadlocks and lock contention or that it didn’t use a “read then update” pattern on the memory pool to avoid it- but that’s definitely a larger refactor for another day.

Agreed 100%. I don't have bandwidth for it, but memory management could do with a good refactoring.

@rschlussel
Copy link
Contributor Author

ooh looks like all the testMemoryReservationYield tests relied on the old behavior. I'll update those.

This prevents queries from exceeding the total memory limit between the
time that user memory is allocated, and the next system memory
allocation.
@rschlussel
Copy link
Contributor Author

tests pass now. Merging.

@rschlussel rschlussel merged commit d86c679 into prestodb:master Feb 17, 2021
@arhimondr arhimondr mentioned this pull request Mar 12, 2021
13 tasks
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.

4 participants