From 43ea661c2bab1fdb2414d342deb3908dbe6ad050 Mon Sep 17 00:00:00 2001 From: Michael Demmer Date: Tue, 26 May 2020 08:00:58 -0700 Subject: [PATCH 1/2] roll back transactions should not bump query counts When executing implicit rollbacks upon mysql connection close, use an explicit CloseSession function in vtgate rather than executing a "rollback" statement, so as to not artificially inflate the query counts. Signed-off-by: Michael Demmer --- go/vt/vtgate/executor.go | 15 ++++++++------- go/vt/vtgate/plugin_mysql_server.go | 4 ++-- go/vt/vtgate/vtgate.go | 7 +++++++ 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/go/vt/vtgate/executor.go b/go/vt/vtgate/executor.go index d0e2acf5986..d5806bf7b2e 100644 --- a/go/vt/vtgate/executor.go +++ b/go/vt/vtgate/executor.go @@ -168,12 +168,7 @@ func (e *Executor) Execute(ctx context.Context, method string, safeSession *Safe warnings.Add("ResultsExceeded", 1) } - // The mysql plugin runs an implicit rollback whenever a connection closes. - // To avoid spamming the log with no-op rollback records, ignore it if - // it was a no-op record (i.e. didn't issue any queries) - if !(logStats.StmtType == "ROLLBACK" && logStats.ShardQueries == 0) { - logStats.Send() - } + logStats.Send() return result, err } @@ -308,11 +303,17 @@ func (e *Executor) handleRollback(ctx context.Context, safeSession *SafeSession, logStats.PlanTime = execStart.Sub(logStats.StartTime) logStats.ShardQueries = uint32(len(safeSession.ShardSessions)) e.updateQueryCounts("Rollback", "", "", int64(logStats.ShardQueries)) - err := e.txConn.Rollback(ctx, safeSession) + err := e.CloseSession(ctx, safeSession) logStats.CommitTime = time.Since(execStart) return &sqltypes.Result{}, err } +// CloseSession closes the current transaction, if any. It is called both for explicit "rollback" +// statements and implicitly when the mysql server closes the connection. +func (e *Executor) CloseSession(ctx context.Context, safeSession *SafeSession) error { + return e.txConn.Rollback(ctx, safeSession) +} + func (e *Executor) handleSet(ctx context.Context, safeSession *SafeSession, sql string, logStats *LogStats) (*sqltypes.Result, error) { stmt, err := sqlparser.Parse(sql) if err != nil { diff --git a/go/vt/vtgate/plugin_mysql_server.go b/go/vt/vtgate/plugin_mysql_server.go index dc5e614b42e..af852d42566 100644 --- a/go/vt/vtgate/plugin_mysql_server.go +++ b/go/vt/vtgate/plugin_mysql_server.go @@ -108,7 +108,7 @@ func (vh *vtgateHandler) ComResetConnection(c *mysql.Conn) { if session.InTransaction { defer atomic.AddInt32(&busyConnections, -1) } - _, _, err := vh.vtg.Execute(ctx, session, "rollback", make(map[string]*querypb.BindVariable)) + err := vh.vtg.CloseSession(ctx, session) if err != nil { log.Errorf("Error happened in transaction rollback: %v", err) } @@ -134,7 +134,7 @@ func (vh *vtgateHandler) ConnectionClosed(c *mysql.Conn) { if session.InTransaction { defer atomic.AddInt32(&busyConnections, -1) } - _, _, _ = vh.vtg.Execute(ctx, session, "rollback", make(map[string]*querypb.BindVariable)) + _ = vh.vtg.CloseSession(ctx, session) } // Regexp to extract parent span id over the sql query diff --git a/go/vt/vtgate/vtgate.go b/go/vt/vtgate/vtgate.go index 0a92c6dbf95..091b99487e6 100644 --- a/go/vt/vtgate/vtgate.go +++ b/go/vt/vtgate/vtgate.go @@ -346,6 +346,13 @@ handleError: return nil } +// CloseSession closes the session, rolling back any implicit transactions. This has the +// same effect as if a "rollback" statement was executed, but does not affect the query +// statistics. +func (vtg *VTGate) CloseSession(ctx context.Context, session *vtgatepb.Session) error { + return vtg.executor.CloseSession(ctx, NewSafeSession(session)) +} + // ResolveTransaction resolves the specified 2PC transaction. func (vtg *VTGate) ResolveTransaction(ctx context.Context, dtid string) error { return formatError(vtg.txConn.Resolve(ctx, dtid)) From 7d12d770a1d03858ea46a0f9644c2444043c5653 Mon Sep 17 00:00:00 2001 From: Michael Demmer Date: Tue, 26 May 2020 09:05:54 -0700 Subject: [PATCH 2/2] update tests to call CloseSession Signed-off-by: Michael Demmer --- go/vt/vtgate/executor_test.go | 4 ++-- go/vt/vtgate/plan_executor_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go/vt/vtgate/executor_test.go b/go/vt/vtgate/executor_test.go index 60d0dc2de3b..08175e48702 100644 --- a/go/vt/vtgate/executor_test.go +++ b/go/vt/vtgate/executor_test.go @@ -160,8 +160,8 @@ func TestExecutorTransactionsNoAutoCommit(t *testing.T) { t.Errorf("logstats: expected non-zero CommitTime") } - // rollback doesn't emit a logstats record when it doesn't do anything - _, err = executor.Execute(context.Background(), "TestExecute", session, "rollback", nil) + // CloseSession doesn't log anything + err = executor.CloseSession(context.Background(), session) if err != nil { t.Fatal(err) } diff --git a/go/vt/vtgate/plan_executor_test.go b/go/vt/vtgate/plan_executor_test.go index 4714729e253..30aadb644aa 100644 --- a/go/vt/vtgate/plan_executor_test.go +++ b/go/vt/vtgate/plan_executor_test.go @@ -156,8 +156,8 @@ func TestPlanExecutorTransactionsNoAutoCommit(t *testing.T) { t.Errorf("logstats: expected non-zero CommitTime") } - // rollback doesn't emit a logstats record when it doesn't do anything - _, err = executor.Execute(context.Background(), "TestExecute", session, "rollback", nil) + // CloseSession doesn't log anything + err = executor.CloseSession(context.Background(), session) if err != nil { t.Fatal(err) }