refactor: make extension mutable in query context#8088
Conversation
sunng87
requested review from
a team,
discord9,
evenyag and
waynexia
as code owners
github-actions
Bot
added
size/S
docs-not-required
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors QueryContext to consolidate mutable fields into a RwLock-protected structure and optimizes string handling across the codebase. Review feedback points out that the manual Clone implementation for QueryContext now performs a deep copy, breaking the shared mutable state for snapshot tracking which could cause consistency issues. Additionally, it is suggested to update set_extension to take &self to facilitate mutation via shared references and to optimize the extension method to avoid unnecessary String allocations.
| mutable_session_data: Arc<RwLock<MutableInner>>, | ||
| #[builder(default)] | ||
| mutable_query_context_data: Arc<RwLock<QueryContextMutableFields>>, | ||
| mutable_query_context_data: RwLock<QueryContextMutableFields>, |
There was a problem hiding this comment.
The change from Arc<RwLock> to RwLock combined with the manual Clone implementation (lines 95-97) breaks the sharing of snapshot_seqs and sst_min_sequences across QueryContext clones. In the original implementation, these fields were wrapped in an Arc, ensuring that all clones of a context shared the same mutable state for snapshot tracking. The current implementation performs a deep copy, making these fields independent. This could lead to consistency issues if different parts of a query execution expect to share or collectively update snapshot information through cloned contexts.
There was a problem hiding this comment.
Confirmed deep copy will work for existing cases.
|
what kind of usage is needed from this change? can once cell suit it? |
|
This is for storing some context information through the execution path. |
|
@sunng87 Need to fix the conflict. |
|
This is no longer required after a few refactor. Will close this and reopen when needed again. |
3 participants
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
In order to store some information with
QueryContext, I will need to makeextensionmutable. This patch moves all mutable items intoQueryContextMutableFields.It also changes it's clone to deep copy to match original behaviour.
PR Checklist
Please convert it to a draft if some of the following conditions are not met.