From 05cc307112f46cbccfc277e1ff161e7b928ebdbd Mon Sep 17 00:00:00 2001
From: Julian Brost <julian.brost@icinga.com>
Date: Tue, 8 Dec 2020 18:10:38 +0100
Subject: [PATCH] Don't retry individual SQL transaction commands

If anything in a transaction fails, the transaction should be retried as
a whole, just retrying some individual command makes no sense and breaks
transaction semantics. For example, if a COMMIT fails in MySQL, it
automatically rolls back the transaction and if you issue another
COMMIT, you just commit an empty transaction.

Remove the test case for commit as is basically tested that it showed
the broken behavior described above.
---
 connection/mysql.go      | 26 ++++++--------------------
 connection/mysql_test.go | 30 ------------------------------
 2 files changed, 6 insertions(+), 50 deletions(-)

diff --git a/connection/mysql.go b/connection/mysql.go
index c91a1973..80fe3a30 100644
--- a/connection/mysql.go
+++ b/connection/mysql.go
@@ -300,12 +300,6 @@ func (dbw *DBWrapper) SqlCommit(tx DbTransaction, quiet bool) error {
 			}).Debug("COMMIT transaction")
 		}
 
-		if err != nil {
-			if dbw.isConnectionError(err) {
-				continue
-			}
-		}
-
 		return err
 	}
 }
@@ -335,12 +329,6 @@ func (dbw *DBWrapper) SqlRollback(tx DbTransaction, quiet bool) error {
 			err = tx.Rollback()
 		}
 
-		if err != nil {
-			if dbw.isConnectionError(err) {
-				continue
-			}
-		}
-
 		return err
 	}
 }
@@ -382,7 +370,7 @@ func (dbw *DBWrapper) SqlFetchAllTxQuiet(tx DbTransaction, queryObserver prometh
 }
 
 // sqlExecInternal is a wrapper around sql.Exec() for auto-logging.
-func (dbw *DBWrapper) sqlExecInternal(db DbClientOrTransaction, opObserver prometheus.Observer, sql string, quiet bool, args ...interface{}) (sql.Result, error) {
+func (dbw *DBWrapper) sqlExecInternal(db DbClientOrTransaction, opObserver prometheus.Observer, query string, quiet bool, args ...interface{}) (sql.Result, error) {
 	for {
 		if !dbw.IsConnected() {
 			dbw.WaitForConnection()
@@ -394,7 +382,7 @@ func (dbw *DBWrapper) sqlExecInternal(db DbClientOrTransaction, opObserver prome
 			benchmarc = utils.NewBenchmark()
 		}
 
-		res, err := db.Exec(sql, args...)
+		res, err := db.Exec(query, args...)
 		DbOperationsExec.Inc()
 
 		if !quiet {
@@ -408,12 +396,12 @@ func (dbw *DBWrapper) sqlExecInternal(db DbClientOrTransaction, opObserver prome
 				"benchmark":     benchmarc,
 				"affected_rows": prettyPrintedRowsAffected{res},
 				"args":          prettyPrintedArgs{args},
-				"query":         prettyPrintedSql{sql},
+				"query":         prettyPrintedSql{query},
 			}).Debug("Finished Exec")
 		}
 
 		if err != nil {
-			if dbw.isConnectionError(err) {
+			if _, isTx := db.(DbTransaction); !isTx && dbw.isConnectionError(err) {
 				continue
 			}
 		}
@@ -433,10 +421,8 @@ func (dbw *DBWrapper) sqlFetchAllInternal(db DbClientOrTransaction, queryObserve
 		res, err := sqlTryFetchAll(db, queryObserver, query, quiet, args...)
 
 		if err != nil {
-			if _, isDb := db.(*sql.DB); isDb {
-				if dbw.isConnectionError(err) {
-					continue
-				}
+			if _, isTx := db.(DbTransaction); !isTx && dbw.isConnectionError(err) {
+				continue
 			}
 		}
 
diff --git a/connection/mysql_test.go b/connection/mysql_test.go
index 32bc7f7b..da119bf5 100644
--- a/connection/mysql_test.go
+++ b/connection/mysql_test.go
@@ -107,36 +107,6 @@ func TestDBWrapper_CheckConnection(t *testing.T) {
 	assert.Equal(t, uint32(11), atomic.LoadUint32(dbw.ConnectionLostCounterAtomic))
 }
 
-func TestDBWrapper_SqlCommit(t *testing.T) {
-	mockDb := new(DbMock)
-	dbw := NewTestDBW(mockDb)
-	mockTx := new(TransactionMock)
-
-	mockTx.On("Commit").Return(errors.New("whoops")).Once()
-	mockTx.On("Commit").Return(nil).Once()
-	mockDb.On("Ping").Return(errors.New("whoops")).Once()
-
-	var err error
-	done := make(chan bool)
-
-	dbw.CompareAndSetConnected(true)
-	go func() {
-		err = dbw.SqlCommit(mockTx, false)
-		done <- true
-	}()
-
-	time.Sleep(time.Millisecond * 50)
-
-	dbw.CompareAndSetConnected(true)
-	dbw.ConnectionUpCondition.Broadcast()
-
-	<-done
-
-	assert.NoError(t, err)
-	mockTx.AssertExpectations(t)
-	mockDb.AssertExpectations(t)
-}
-
 func TestDBWrapper_SqlBegin(t *testing.T) {
 	mockDb := new(DbMock)
 	dbw := NewTestDBW(mockDb)