fix: skip unnecessary FOR UPDATE locks in append_event when no app/user state delta#4661
Conversation
…er state delta DatabaseSessionService.append_event() unconditionally acquires SELECT ... FOR UPDATE on app_states and user_states even when the event has no delta for those scopes. Since app_states is keyed by app_name alone, all concurrent append_event calls within the same app serialize on this single row lock, even when most events only carry session-scoped state. Pre-analyze the event's state delta before the transaction and only pass use_row_level_locking=True for scopes that actually have pending writes. Fixes google#4655
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes the DatabaseSessionService.append_event method by making database row-level locking conditional. Previously, SELECT ... FOR UPDATE locks were unconditionally applied to app_states and user_states, leading to unnecessary contention, especially for app_states. The change introduces a pre-analysis step to identify relevant state deltas, ensuring locks are only acquired when actual state modifications are pending, thereby improving concurrency and performance. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
adk-bot
added
the
services
![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 introduces a valuable optimization by conditionally acquiring FOR UPDATE locks in append_event only when there are app or user state deltas. This should effectively reduce lock contention and improve performance for events that don't modify shared state. The implementation is clear and the logic for detecting deltas is sound. The addition of comprehensive unit tests covering various delta scenarios is excellent and ensures the new logic is working as expected. I have one suggestion to refactor the new tests to improve maintainability by reducing code duplication.
|
addressed the review feedback in 7c416ae. extracted a /gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an important optimization by avoiding unnecessary FOR UPDATE locks in append_event when there are no state deltas for app or user scopes. The change correctly pre-analyzes the event's state delta and conditionally applies row-level locking. The new logic is well-covered by a comprehensive set of new unit tests. I have one suggestion to improve code clarity and avoid a repeated function call.
|
addressed the second review comment in 9ec1187. now reusing the pre-analyzed /gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable optimization to append_event by avoiding unnecessary FOR UPDATE row-level locks when an event lacks app or user state deltas. The implementation correctly pre-analyzes the event's state delta to conditionally apply locks, which should reduce database contention. The change is supported by a comprehensive new test suite that validates the conditional locking logic across various scenarios. My feedback includes a minor suggestion to refactor the state delta analysis for improved readability.
|
done in adb51fb, moved the bool checks inside the if block. /gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable optimization to DatabaseSessionService.append_event() by conditionally acquiring SELECT ... FOR UPDATE locks only when there are state deltas for the app or user scopes. This effectively reduces unnecessary lock contention. The implementation is clean, pre-analyzing the state delta and reusing the result to avoid redundant computations. The accompanying tests are thorough, using a spy to verify the locking behavior across various scenarios, which is excellent.
|
keeping the same reasoning for /gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-reasoned optimization to append_event by avoiding unnecessary SELECT ... FOR UPDATE locks on app_states and user_states when an event doesn't contain deltas for those scopes. The implementation correctly pre-analyzes the event's state delta and conditionally applies row-level locking. The code is clean, and the logic is sound. The addition of a new test suite with a spy on the locking mechanism is excellent, providing thorough coverage for all the new conditional logic paths. The changes look solid and effectively address the potential for lock contention.
|
@rusherman implemented the fix you suggested in #4655. let me know if you have any feedback. @xuanyang15 @ryanaiagent would appreciate a review when you get a chance. |
2 participants
DatabaseSessionService.append_event()acquiresSELECT ... FOR UPDATEon bothapp_statesanduser_statesunconditionally, even when the event carries no delta for those scopes.Since
app_statesis keyed byapp_namealone, this means all concurrentappend_eventcalls within the same app serialize on that single row lock. Most events only carry session-scoped state, so this lock contention is unnecessary.This was partially addressed by #764 which fixed the unnecessary UPDATE (merge) side, but the SELECT ... FOR UPDATE (row-level lock acquisition) still happens unconditionally.
The fix pre-analyzes the event's state delta before the transaction using
extract_state_delta()(which only reads from the event object, no side effects) and only passesuse_row_level_locking=Truefor scopes that actually have pending writes.Added 5 tests covering all combinations: no delta, session-only delta, app-only delta, user-only delta, and both app+user deltas. All 69 session tests pass.
Fixes #4655