feat: auto purge stale prepared statements from cache and retry query#4199
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4199 +/- ##
=======================================
Coverage 90.86% 90.87%
=======================================
Files 89 89
Lines 14490 14568 +78
Branches 1864 1882 +18
=======================================
+ Hits 13167 13239 +72
- Misses 1323 1329 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds internal invalidation/retry behavior for cached prepared statements when the server reports a stale statement handler, so users don’t need to manually purge internal caches.
Changes:
- Detect
ER_UNKNOWN_STMT_HANDLERduringexecute()and retry after purging the cached prepared statement. - Add an integration test that simulates a stale cached statement and asserts automatic retry + cache refresh.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/base/connection.js |
Implements stale prepared-statement detection, cache purge, and automatic retry for execute(). |
test/integration/connection/test-execute-cached.test.mts |
Adds coverage ensuring stale cached prepared statements are discarded and queries succeed after automatic retry. |
186a4fd to
48d7b45
Compare
|
Added additional coverage for the EventEmitter path. |
|
Ok, this time I think it's ready; apologies for all the noise 😅 |
|
Should be at 100% coverage now, and also cleaned up the "emit 'end' twice" that was happening when the internal prepare statement failed. |
|
I think this PR does not cover all possible cases where a prepared statement gets unexpectedly invalidated. There are essentially two scenarios:
The first case, in my view, should be fixed by removing the cache from the manual mechanism. If someone is manually using prepare(), stmt.execute(), stmt.close(), it should not interact with the cache at all. The caching mechanism should remain exclusively on the execute method side. Regarding the second case: During ZDP, the session is essentially destroyed and |
agree.
interesting. I wonder if there is any way to get notified about statement counter reset / ZDP. If that's not possible, clearing cache on ER_UNKNOWN_STMT_HANDLER seems like a logical thing to do ( still, potentially unexpected behaviour when the query that is actiually executed might not match to what was in |
|
#3726 mentions this... |
|
I think for the ZDP scenario where statement IDs might point to the wrong statement... a Map of statement ID to statement key would allow us, during prepare response, to detect that this has happened and remove the old statement that had the same ID... thoughts? This would need to include all prepared statements, even if they weren't automatically created via execute... though we should also stop caching if not coming from execute in the first place. |
|
I was thinking if changing prepare() / unprepare() caching behavior needs a major version release, but reading #805 (comment) I'm more inclined to release it as a bugfix as its currently already quite broken and likely almost never used. I think we can merge this change and have a follow up PR which removes caching from manual prepare() |
|
Awesome, but the ZDP thing for me (and I think that's what bit me too which prompted me to write this), highlights a bigger problem with mismatched query IDs to queries... admittedly unlikely on a given connection to SELECT * FROM widgets WHERE id = ?, and the server thinks that prepared statement is DELETE FROM widgets WHERE id = ?, but possible? |
|
I initially thought it's possible, but maybe not. After ZDP happens, the very next ER_UNKNOWN_STMT_HANDLER -> reset cache, retry prepare + execute again |
|
I thought it would be execute -> prepare/cache store -> execute it, execute again -> cache fetch succeed -> ZDP -> another out of band prepare makes the same query id for a delete with same number and type of arguments -> second execute performs the wrong query. |
|
But if you are using the pool correctly that's not possible... so maybe not a real issue 🤔 |
|
Unclear on how id conflict would work, I don't think the id is part of the cache key so how would we know without a full cache scan (which sounds like a bad thing to do...) |
|
That leads to my map of query id to cache key. |
|
@Invader444 yes, unless we add statement id -> cache key map and reset connection on the first conflict the following is possible now: |
Co-authored-by: Weslley Araújo <46850407+wellwelwel@users.noreply.github.com>
Co-authored-by: Weslley Araújo <46850407+wellwelwel@users.noreply.github.com>
e7eacef to
debdc76
Compare
Even with proper tuning of maxPreparedStatements, it is possible that the automatic LRU cache contains stale prepared statements that have been released on the server from time to time.
Normally, with cached prepared statements, a user could catch the ER_UNKNOWN_STMT_HANDLER error, purge from their cache, and try again with a newly prepared statement. However, to do that with the internal LRU cache, a user would need to touch internals that are not meant to be public APIs... which could break at any time.
This handles that invalidation and retry logic internally so the user doesn't have to touch private/internal implementation details.
This may help issue #805