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
transient in memory cache #4922
base: dev
Are you sure you want to change the base?
Conversation
This adds another level of in memory cache, to mitigate issues with unique or infrequent queries pushing frequently used queries out of the in memory cache. If a query has only been seen once, then it is stored inthe transient cache (short term, small number of entries, LRU). If that query is requested again, and is still present in the transient cache, then it is added to the long term, larger in memory cache, to the redis cache as well, and removed from the transient cache. If that query is requested again but not present in the transient cache, then we test the larger in memory cache and redis
@Geal, please consider creating a changeset entry in |
CI performance tests
|
We need to do something in this space, but I feel we can do something better when #4796 lands. I imagine that with the new rust support we could do something like: (query from client) -> new functionality in 4796 -> (normalised form) Then use (normalised form) as the key to the query plan. If the conversion to (normalised form) is expensive we could introduce a smaller cache to hold (query from client) -> (normalised form), but I imagine that shouldn't be required. Note: Some details (extra items in the key such as metadata) are omitted for brevity, but wouldn't this basic approach work? |
That's definitely something we could do. And we already have the right cache for that, in query analysis #4796 (comment) |
Note that right now the normalised form is stripping out some important information like alias names and input objects, but I'm planning to add support for including those things as part of PULSR-695. |
@bonnici for some context, we recently merged #4883 which uses a hashing scheme for the query that keeps the same hash across schema updates, if the update does not affect the query. Unfortunately, we needed to add back the operation name to the query plan cache key (#4921), because the query planner was returning the usage reporting structure as well, and the operation signature contains the operation name, so with the operation name in the cache key, we would have not reported operations properly. |
I've converted this to a draft so we can discuss it further, but it won't be on our review backlog for now. |
@abernix it was in the review queue because I expected some reviews :/ |
Before looking too much into the implementation of this, do we still think this is the right solution for this problem? My comment above still seems applicable. More so, since various changes have landed that make "normalised form" a more realistic prospect. In general, this feels like a "bigger" decision than code review and could use some more substantial design reviewing before proceeding to code review. Perhaps we could discuss this in the next router architectural working group meeting? One thing we definitely want to discuss is whether we should expose configuration of this feature. |
normalization is a much larger issue than this, and not that much related. I agree that it needs further discussion though, I added it to the topics of the meeting |
This adds another level of in memory cache, to mitigate issues with unique or infrequent queries pushing frequently used queries out of the in memory cache.
If a query has only been seen once, then it is stored inthe transient cache (short term, small number of entries, LRU).
If that query is requested again, and is still present in the transient cache, then it is added to the long term, larger in memory cache, to the redis cache as well, and removed from the transient cache. If that query is requested again but not present in the transient cache, then we test the larger in memory cache and redis
Description here
Fixes #issue_number
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩