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

Fix cached plan not getting invalidated #1348

Merged
merged 24 commits into from Nov 7, 2023

Conversation

DavIvek
Copy link
Contributor

@DavIvek DavIvek commented Oct 11, 2023

This PR is about resolving memory spike that happens when we use variable in MERGE statement. The MERGE clause that contains variable for a node will cause a significant memory spike 30x times.

[master < Task] PR

  • Check, and update documentation if necessary
  • Provide the full content or a guide for the final git message

To keep docs changelog up to date, one more thing to do:

  • Write a release note here, including added/changed clauses
  • Tag someone from docs team in the comments

closes #1251

@DavIvek DavIvek marked this pull request as draft October 11, 2023 07:53
@DavIvek DavIvek self-assigned this Oct 11, 2023
@DavIvek
Copy link
Contributor Author

DavIvek commented Oct 11, 2023

If we avoid to use monotonic memory, the difference in memory consumption (when we use variable and when we don't) will decrease but still remain significant.

@gitbuda
Copy link
Member

gitbuda commented Oct 19, 2023

The problem here seems to be the caching of query plans (probably query plan cache). The solution is to implement some cache eviction strategy, LRU or some background process (LRU is the best place to start).

@DavIvek
Copy link
Contributor Author

DavIvek commented Oct 24, 2023

Since we are not invoking a garbage collector for plan cache, for testing purposes, I changed the Interpreter::PrepareIndexQuery part of the function where we remove cached plans from the skip list. Instead of calling remove for every cached plan, now we call the skip lists clear() function to force memory deallocation because remove() doesn't deallocate memory, it only marks skip list entry as deleted (and the garbage collector should do deallocation when it's safe to do it).

With that minor change, I was able to release all the memory used by the plan cache:
Firstly, by executing query: 'CREATE INDEX ON: MyNode1(address)', we will call plan_cache->clear(), following should remove all elements from the skip list.
After that, memory still isn't released, but if we execute the 'FREE MEMORY' query, memory usage should drop drastically.

I'm still confused about what actually happens there when we call the FREE MEMORY query.

@DavIvek
Copy link
Contributor Author

DavIvek commented Oct 26, 2023

TODO:

  • rename query-plan-cache-ttl flag and think of what would be a good default value
  • remove unnecessary parts of code

@DavIvek DavIvek marked this pull request as ready for review October 31, 2023 07:34
@DavIvek DavIvek added the Docs needed Docs needed label Oct 31, 2023
@DavIvek DavIvek marked this pull request as ready for review November 2, 2023 12:37
Copy link
Collaborator

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Two comments left, and that is it

@antoniofilipovic antoniofilipovic changed the title Fix memory spike when using variable in MERGE statment Fix cached plan not getting invalidated Nov 2, 2023
@DavIvek DavIvek marked this pull request as draft November 3, 2023 14:06
@DavIvek DavIvek marked this pull request as ready for review November 3, 2023 14:12
Copy link
Collaborator

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Looks good to me, good job here 🥳

@antoniofilipovic antoniofilipovic added this to the mg-v2.13.0 milestone Nov 6, 2023
@DavIvek DavIvek merged commit ece4b0d into master Nov 7, 2023
6 checks passed
@DavIvek DavIvek deleted the fix-memory-spike-merge-with-variable branch November 7, 2023 12:34
@gitbuda gitbuda modified the milestones: mg-v2.13.0, mg-v2.12.1 Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix simple MERGE with variable causes a big memory spike
3 participants