Skip to content
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.

Sign up for GitHub

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

Jump to bottom

When a SQL error occurs, clean up all prepared statement. #2936

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+31 −9
Open

When a SQL error occurs, clean up all prepared statement. #2936

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b246630
When a SQL error occurs, clean up all prepared statement.
px3303 Mar 15, 2023
237ad82
Add logs and metric
px3303 Mar 15, 2023
96a0965
Improve readability
px3303 Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions storage/mysql/log_storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,7 @@ func (t *logTreeTX) getLeavesByHashInternal(ctx context.Context, leafHashes [][]
args = append(args, t.treeID)
rows, err := stx.QueryContext(ctx, args...)
if err != nil {
t.ls.cleanAllStmt()
klog.Warningf("Query() %s hash = %v", desc, err)
return nil, err
}
Expand Down
16 changes: 16 additions & 0 deletions storage/mysql/tree_storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,20 @@ 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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would clearStmtCache be a clearer name for this func?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

m.statementMutex.Lock()
defer m.statementMutex.Unlock()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

for _, ns := range m.statements {
for _, s := range ns {
s.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
}

m.statements = make(map[string]map[int]*sql.Stmt)
}

// getStmt creates and caches sql.Stmt structs based on the passed in statement
// and number of bound arguments.
// TODO(al,martin): consider pulling this all out as a separate unit for reuse
Expand Down Expand Up @@ -200,6 +214,7 @@ func (t *treeTX) getSubtrees(ctx context.Context, treeRevision int64, ids [][]by

rows, err := stx.QueryContext(ctx, args...)
if err != nil {
t.ts.cleanAllStmt()
klog.Warningf("Failed to get merkle subtrees: %s", err)
return nil, err
}
Expand Down Expand Up @@ -296,6 +311,7 @@ func (t *treeTX) storeSubtrees(ctx context.Context, subtrees []*storagepb.Subtre

r, err := stx.ExecContext(ctx, args...)
if err != nil {
t.ts.cleanAllStmt()
klog.Warningf("Failed to set merkle subtrees: %s", err)
return err
}
Expand Down