When a SQL error occurs, clean up all prepared statement. #2936
Conversation
| func (m *mySQLTreeStorage) cleanAllStmt() { | ||
| m.statementMutex.Lock() | ||
| defer m.statementMutex.Unlock() | ||
|
|
There was a problem hiding this comment.
Some kind of logging for this would be really useful. This should be a rare event in an otherwise healthy system:
klog.Info("Clearing all prepared statements")
Actually, for bonus points I think it's worth incrementing a counter whenever we clear statements, which should appear on monitoring dashboards and allow correlation with potential impacts such as increased latency or errors. Take a look at queuedCounter in this class as an example (though we won't need a logID for this as it's effectively global).
storage/mysql/tree_storage.go
Outdated
|
|
||
| for _, ns := range m.statements { | ||
| for _, s := range ns { | ||
| s.Close() |
There was a problem hiding this comment.
Please check the error returned from Close. Maybe not much more to do other than log the failure, but I don't like letting errors like this simply disappear.
There was a problem hiding this comment.
This LGTM. Would appreciate @AlCutter giving final approval as an original author of this code.
|
/gcbrun |
storage/mysql/log_storage.go
Outdated
| @@ -101,6 +102,7 @@ func createMetrics(mf monitoring.MetricFactory) { | |||
| queuedCounter = mf.NewCounter("mysql_queued_leaves", "Number of leaves queued", logIDLabel) | |||
| queuedDupCounter = mf.NewCounter("mysql_queued_dup_leaves", "Number of duplicate leaves queued", logIDLabel) | |||
| dequeuedCounter = mf.NewCounter("mysql_dequeued_leaves", "Number of leaves dequeued", logIDLabel) | |||
| clearedAllStmtCounter = mf.NewCounter("mysql_cleared_all_prepared_statements", "Number of all prepared statements cleared") | |||
There was a problem hiding this comment.
nit: -> "number of times the prepared-statement cache has been cleared"?
storage/mysql/tree_storage.go
Outdated
| @@ -104,6 +104,25 @@ func expandPlaceholderSQL(sql string, num int, first, rest string) string { | |||
| return strings.Replace(sql, placeholderSQL, parameters, 1) | |||
| } | |||
|
|
|||
| // clearAllStmt clean up all sql.Stmt in cache | |||
| func (m *mySQLTreeStorage) cleanAllStmt() { | |||
There was a problem hiding this comment.
Would clearStmtCache be a clearer name for this func?
|
/gcbrun |
|
@px3303 Would you mind adding a small bit of text to the CHANGELOG to capture this? It's an important change which definitely should be highlighted in the next release's set of release notes. |
|
@px3303 Looks like the metric name needs a tweak, might be the "-": |
There was a problem hiding this comment.
I feel a bit bad that we never got round to this. The way this works here means some errors will surface but it should recover. Alternatively, it could retry on error but I think this code is probably already complex enough and this isn't something that should happen often.
| @@ -101,6 +102,7 @@ func createMetrics(mf monitoring.MetricFactory) { | |||
| queuedCounter = mf.NewCounter("mysql_queued_leaves", "Number of leaves queued", logIDLabel) | |||
| queuedDupCounter = mf.NewCounter("mysql_queued_dup_leaves", "Number of duplicate leaves queued", logIDLabel) | |||
| dequeuedCounter = mf.NewCounter("mysql_dequeued_leaves", "Number of leaves dequeued", logIDLabel) | |||
| clearedStmtCacheCounter = mf.NewCounter("mysql_cleared_prepared-statement_caches", "Number of times the prepared-statement cache has been cleared") | |||
There was a problem hiding this comment.
Might be worth noting in the description that this is an unexpected condition / could indicate a problem.
| INNER JOIN Subtree | ||
| ON Subtree.SubtreeId = x.SubtreeId | ||
| AND Subtree.SubtreeRevision = x.MaxRevision | ||
| INNER JOIN Subtree |
There was a problem hiding this comment.
I assume these are gofmt changes. Could be done separately but up to you.
|
@AlCutter @Martin2112 @mhutchinson I submitted an improved version #2937 , please review it again. |
As described in #420 , when the prepared statement fails unexpectedly in the database, the program cannot be recovered.
This PR will clear all the prepared statements in the cache when the statement execution error occurs (this is because other prepared statements may also make errors).