Use a separate unit to manage the cache of prepared statements #2937

px3303 · 2023-03-17T08:40:59Z

This PR is an improved version of #2936, with the following changes:

1.Use a separate unit (StmtCache) to manage the cache of prepared statements.
2.Wrap sql.Stmt struct to handle monitoring prepared statement execution errors.
3.When a statement execution error occurs, only clear the corresponding prepared statement cache, preventing circular cache clearing problems.
4.Add prepared statement execution error metrics.

Checklist

I have updated the CHANGELOG.
I have updated documentation accordingly (including the feature implementation matrix).

AlCutter · 2023-03-17T11:52:54Z

/gcbrun

Martin2112

How good is the test coverage for this? Not sure if it's worth having dedicated tests but worth considering perhaps.

Martin2112 · 2023-03-17T09:33:56Z

storage/sqlutil/stmt_cache.go

@@ -0,0 +1,246 @@
+package sqlutil


The package name is generic. I'd suggest naming it after the main functionality it contains (the statement cache).

The package name is generic. I'd suggest naming it after the main functionality it contains (the statement cache).
fixed

Martin2112 · 2023-03-17T09:37:43Z

CHANGELOG.md

@@ -7,6 +7,8 @@
    different version can lead to presubmits failing due to unexpected
    diffs.

+* Use a separate unit (StmtCache) to manage the cache of prepared statements, which wraps the sql.Stmt struct to handle and monitor the execution errors of the prepared statement. When an error occurs during statement execution, it closes the statement and clears the cache, as well as increments the error monitoring indicator.


Not sure what 'unit' means?

The word 'separate unit' comes from the current TODO comment:

trillian/storage/mysql/tree_storage.go

Line 109 in bc857c2

// TODO(al,martin): consider pulling this all out as a separate unit for reuse

Martin2112 · 2023-03-17T09:38:23Z

storage/sqlutil/stmt_cache.go

+}
+
+// errHandler handling and monitoring SQL errors
+// The err parameter standby


Not sure what 'standby' means?

Comments have been updated.

Martin2112 · 2023-03-17T09:55:23Z

storage/sqlutil/stmt_cache.go

+//	var name string
+//	err := nameByUseridStmt.QueryRow(id).Scan(&name)
+//
+// QueryRow uses context.Background internally; to specify the context, use


Are there cases where the background context is still used? Might be worth deprecating this?

Martin2112 · 2023-03-17T09:56:16Z

storage/sqlutil/stmt_cache.go

+}
+
+// NewStmtCache creates a StmtCache instance.
+func NewStmtCache(db *sql.DB, mf monitoring.MetricFactory) *StmtCache {


I think this can just be 'New'.

Martin2112 · 2023-03-17T09:58:14Z

storage/sqlutil/stmt_cache.go

+	return strings.Replace(sql, PlaceholderSQL, parameters, 1)
+}
+
+// GetStmt creates and caches sql.Stmt structs and returns their wrapper structs Stmt.


I think 'structs' can be removed as this should be clear from context.

AlCutter

The behaviour of this PR is slightly different to #2936 in that I think here we'll only remove the statement which errored from the cache (the others will remain until they too error out) - is that intentional?

The other thing which I think is worth discussing is the relative size of the two PRs - this one feels like it's going for a "cleaner" way of approaching it, but the trade-off is that it's 10x the LoC.
#2936 is very small in comparison, and very explicit, but could miss a call site (although since the entire cache is flushed on any error that would likely mitigate the effect missed call sites).
Any thoughts, @px3303 @Martin2112 @mhutchinson @google/trillian-team?

AlCutter · 2023-03-21T11:13:51Z

storage/stmtcache/stmtcache_test.go

+// 	_, err := cache.GetStmt(ctx, sql, 1, "?", "")
+// 	if err != nil {
+// 		t.Fatalf("failed to cache.GetStmt: %s", err)
+// 	}


Would calling db.Close() after creating the stmt but before using it let you trigger the cache clear logic?

Sorry, that part was submitted before it was completed. It has now been completed.

AlCutter · 2023-03-21T11:13:58Z

storage/stmtcache/stmtcache.go

+	sc.statementMutex.Lock()
+	defer sc.statementMutex.Unlock()
+
+	if sc.statements[s.statement] != nil && sc.statements[s.statement][s.placeholderNum] == s.stmt {


I think you might be able to write this as:

if s, ok := sc.statements[s.statement][s.placeholderNum]; ok && s == s.stmt { ... }

I think you might be able to write this as:

if s, ok := sc.statements[s.statement][s.placeholderNum]; ok && s == s.stmt { ... }

Thanks, fixed.

AlCutter · 2023-03-21T11:14:57Z

storage/stmtcache/stmtcache.go

+	errStmtCounter monitoring.Counter
+)
+
+// PlaceholderSQL SQL statement placeholder


nit: missing trailing full-stops on a few of the comments.

px3303 · 2023-03-21T13:54:05Z

How good is the test coverage for this? Not sure if it's worth having dedicated tests but worth considering perhaps.

I have added some test cases, but some situations cannot be simulated. For example, the prepared statement disappeared in the database for unknown reasons. Currently, we have encountered this problem in the production environment. A failure in the AWS RDS database caused the prepared statement to fail, and an error was reported: Unknown prepared statement handler (17) gave to mysql_ stmt_ precheck。 When this happens, the program does not automatically resume.

@Martin2112 @AlCutter

px3303 · 2023-03-21T14:34:28Z

The behaviour of this PR is slightly different to #2936 in that I think here we'll only remove the statement which errored from the cache (the others will remain until they too error out) - is that intentional?

Because in high concurrency situations, clearing all caches may cause a loop problem.

The other thing which I think is worth discussing is the relative size of the two PRs - this one feels like it's going for a "cleaner" way of approaching it, but the trade-off is that it's 10x the LoC. #2936 is very small in comparison, and very explicit, but could miss a call site (although since the entire cache is flushed on any error that would likely mitigate the effect missed call sites). Any thoughts, @px3303 @Martin2112 @mhutchinson @google/trillian-team?

This PR wants to handle it more cleanly, and also wants to monitor SQL errors. Currently, these SQL errors only print logs, and we cannot detect them in a timely manner.

Martin2112 · 2023-03-27T10:18:05Z

I wonder if we should be looking at the other option of removing the statement cache? It's not clear to me what the performance impact would be and whether it would be acceptable.

The Golang driver does not have the same option as Java for retaining prepared statements: https://docs.oracle.com/cd/B19306_01/java.102/b14355/stmtcach.htm#i1071993.

px3303 added 3 commits March 17, 2023 14:50

Wraps the sql.Stmt struct for handling and monitoring SQL errors.

4586b3e

update CHANGELOG

5409439

update CHANGELOG

23e4e32

px3303 requested a review from a team as a code owner March 17, 2023 08:40

px3303 requested a review from jiggoha March 17, 2023 08:41

px3303 mentioned this pull request Mar 17, 2023

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

Open

2 tasks

add package comment

19c44fb

Martin2112 reviewed Mar 17, 2023

View reviewed changes

px3303 added 5 commits March 20, 2023 10:36

Rename package name sqlutil to stmtcache.

e23980a

Improve comment readability

d554abb

rename NewStmtCache to New

efaf874

Remove support for background context

8303f0a

add stmtcache tests

0506ff5

AlCutter reviewed Mar 21, 2023

View reviewed changes

px3303 added 3 commits March 21, 2023 21:27

Add test cases in case of execution errors

dc3c8bb

Improve comment readability

0d116cc

Improve code readability

ee5c787

px3303 requested review from Martin2112 and AlCutter and removed request for jiggoha, Martin2112 and AlCutter March 21, 2023 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a separate unit to manage the cache of prepared statements #2937

Use a separate unit to manage the cache of prepared statements #2937

px3303 commented Mar 17, 2023

AlCutter commented Mar 17, 2023

Martin2112 left a comment

Martin2112 Mar 17, 2023

px3303 Mar 21, 2023

Martin2112 Mar 17, 2023

px3303 Mar 20, 2023

Martin2112 Mar 17, 2023

px3303 Mar 21, 2023

Martin2112 Mar 17, 2023

px3303 Mar 21, 2023

Martin2112 Mar 17, 2023

px3303 Mar 21, 2023

Martin2112 Mar 17, 2023

px3303 Mar 21, 2023

AlCutter left a comment

AlCutter Mar 21, 2023

px3303 Mar 21, 2023

AlCutter Mar 21, 2023

px3303 Mar 21, 2023

AlCutter Mar 21, 2023

px3303 Mar 21, 2023

px3303 commented Mar 21, 2023

px3303 commented Mar 21, 2023

Martin2112 commented Mar 27, 2023

Use a separate unit to manage the cache of prepared statements #2937

Are you sure you want to change the base?

Use a separate unit to manage the cache of prepared statements #2937

Conversation

px3303 commented Mar 17, 2023

Checklist

AlCutter commented Mar 17, 2023

Martin2112 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlCutter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

px3303 commented Mar 21, 2023

px3303 commented Mar 21, 2023

Martin2112 commented Mar 27, 2023