From f69c07bba5bffc215af738655e3769859e3ab17f Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Fri, 20 Oct 2023 15:45:06 +0200 Subject: [PATCH 1/6] buxfix: statement prepared in TX is closed with TX If a sqlair.Statement is first used inside a transaction, it is prepared in the transaction dedicated connection. When the transaction is closed, the connection is closed as well, meaning the statement is no longer valid. --- package_test.go | 53 ++++++++++++++++++------------------------------- sqlair.go | 2 +- 2 files changed, 20 insertions(+), 35 deletions(-) diff --git a/package_test.go b/package_test.go index e48f055b..46bf31a1 100644 --- a/package_test.go +++ b/package_test.go @@ -1165,40 +1165,6 @@ func (s *PackageSuite) TestPreparedStmtCaching(c *C) { checkCacheEmpty() } -func (s *PackageSuite) TestTransactionWithOneConn(c *C) { - tables, sqldb, err := personAndAddressDB() - c.Assert(err, IsNil) - sqldb.SetMaxOpenConns(1) - ctx := context.Background() - - db := sqlair.NewDB(sqldb) - defer dropTables(c, db, tables...) - - // This test sets the maximum number of connections to the DB to one. The - // database/sql library makes use of a pool of connections to communicate - // with the DB. Certain operations require a dedicated connection to run, - // such as transactions. - // This test ensures that we do not enter a deadlock when doing a behind - // the scenes prepare for a transaction. - selectStmt := sqlair.MustPrepare("SELECT &Person.* FROM person WHERE name = 'Mark'", Person{}) - mark := Person{20, "Mark", 1500} - - tx, err := db.Begin(ctx, nil) - c.Assert(err, IsNil) - - q := tx.Query(ctx, selectStmt) - defer func() { - c.Assert(tx.Commit(), IsNil) - }() - iter := q.Iter() - c.Assert(iter.Next(), Equals, true) - p := Person{} - c.Assert(iter.Get(&p), IsNil) - c.Assert(mark, Equals, p) - c.Assert(iter.Next(), Equals, false) - c.Assert(iter.Close(), IsNil) -} - type JujuLeaseKey struct { Namespace string `db:"type"` ModelUUID string `db:"model_uuid"` @@ -1407,3 +1373,22 @@ func (s *PackageSuite) TestRaceConditionFinalizerTX(c *C) { // Assert that sql.Stmt was not closed early. c.Assert(q.Run(), IsNil) } + +func (s *PackageSuite) TestStatementTXReuse(c *C) { + sqldb, err := setupDB() + c.Assert(err, IsNil) + + db := sqlair.NewDB(sqldb) + + // Create a statement and run it on a transaction. + selectStmt := sqlair.MustPrepare(`SELECT 'hello'`) + tx, err := db.Begin(nil, nil) + c.Assert(err, IsNil) + q := tx.Query(nil, selectStmt) + c.Assert(q.Run(), IsNil) + tx.Commit() + + // Run the same existing statement outside the transaction. + q = db.Query(nil, selectStmt) + c.Assert(q.Run(), IsNil) +} diff --git a/sqlair.go b/sqlair.go index 43c2963d..6766a37b 100644 --- a/sqlair.go +++ b/sqlair.go @@ -558,7 +558,7 @@ func (tx *TX) Query(ctx context.Context, s *Statement, inputArgs ...any) *Query return &Query{ctx: ctx, err: ErrTXDone} } - sqlstmt, err := tx.db.prepareStmt(ctx, tx.sqlconn, s) + sqlstmt, err := tx.db.prepareStmt(ctx, tx.db.sqldb, s) if err != nil { return &Query{ctx: ctx, err: err} } From c98533e0e083fabd3ad6ea5e76d771d7d84bee7a Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Fri, 20 Oct 2023 18:06:50 +0200 Subject: [PATCH 2/6] Remove prepareSubstrate --- sqlair.go | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/sqlair.go b/sqlair.go index 6766a37b..c03cfa4f 100644 --- a/sqlair.go +++ b/sqlair.go @@ -164,7 +164,7 @@ func (db *DB) Query(ctx context.Context, s *Statement, inputArgs ...any) *Query ctx = context.Background() } - sqlstmt, err := db.prepareStmt(ctx, db.sqldb, s) + sqlstmt, err := db.prepareStmt(ctx, s) if err != nil { return &Query{ctx: ctx, err: err} } @@ -177,17 +177,9 @@ func (db *DB) Query(ctx context.Context, s *Statement, inputArgs ...any) *Query return &Query{sqlstmt: sqlstmt, stmt: s, db: db, qe: qe, ctx: ctx, err: nil} } -// prepareSubstrate is an object that queries can be prepared on, e.g. a sql.DB -// or sql.Conn. It is used in prepareStmt. -type prepareSubstrate interface { - PrepareContext(context.Context, string) (*sql.Stmt, error) -} - -// prepareStmt prepares a Statement on a prepareSubstrate. It first checks in -// the cache to see if it has already been prepared on the DB. -// The prepareSubstrate must be assosiated with the same DB that prepareStmt is -// a method of. -func (db *DB) prepareStmt(ctx context.Context, ps prepareSubstrate, s *Statement) (*sql.Stmt, error) { +// prepareStmt prepares a Statement on a DB. It first checks in the cache to +// see if it has already been prepared. +func (db *DB) prepareStmt(ctx context.Context, s *Statement) (*sql.Stmt, error) { var err error cacheMutex.RLock() // The statement ID is only removed from the cache when the finalizer is @@ -195,7 +187,7 @@ func (db *DB) prepareStmt(ctx context.Context, ps prepareSubstrate, s *Statement sqlstmt, ok := stmtDBCache[s.cacheID][db.cacheID] cacheMutex.RUnlock() if !ok { - sqlstmt, err = ps.PrepareContext(ctx, s.pe.SQL()) + sqlstmt, err = db.sqldb.PrepareContext(ctx, s.pe.SQL()) if err != nil { return nil, err } @@ -558,7 +550,7 @@ func (tx *TX) Query(ctx context.Context, s *Statement, inputArgs ...any) *Query return &Query{ctx: ctx, err: ErrTXDone} } - sqlstmt, err := tx.db.prepareStmt(ctx, tx.db.sqldb, s) + sqlstmt, err := tx.db.prepareStmt(ctx, s) if err != nil { return &Query{ctx: ctx, err: err} } From 57c59cc9289aa7cac9e3923eabb8bc1ceb8d4b9c Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Mon, 23 Oct 2023 09:56:19 +0200 Subject: [PATCH 3/6] Remove dedicated sql connection --- sqlair.go | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/sqlair.go b/sqlair.go index c03cfa4f..5f491c70 100644 --- a/sqlair.go +++ b/sqlair.go @@ -468,10 +468,9 @@ func (q *Query) GetAll(sliceArgs ...any) (err error) { } type TX struct { - sqltx *sql.Tx - sqlconn *sql.Conn - db *DB - done int32 + sqltx *sql.Tx + db *DB + done int32 } func (tx *TX) isDone() bool { @@ -490,15 +489,11 @@ func (db *DB) Begin(ctx context.Context, opts *TXOptions) (*TX, error) { if ctx == nil { ctx = context.Background() } - sqlconn, err := db.sqldb.Conn(ctx) + sqltx, err := db.sqldb.BeginTx(ctx, opts.plainTXOptions()) if err != nil { return nil, err } - sqltx, err := sqlconn.BeginTx(ctx, opts.plainTXOptions()) - if err != nil { - return nil, err - } - return &TX{sqltx: sqltx, sqlconn: sqlconn, db: db}, nil + return &TX{sqltx: sqltx, db: db}, nil } // Commit commits the transaction. @@ -507,9 +502,6 @@ func (tx *TX) Commit() error { if err == nil { err = tx.sqltx.Commit() } - if cerr := tx.sqlconn.Close(); err == nil { - err = cerr - } return err } @@ -519,9 +511,6 @@ func (tx *TX) Rollback() error { if err == nil { err = tx.sqltx.Rollback() } - if cerr := tx.sqlconn.Close(); err == nil { - err = cerr - } return err } From 49ac151e59e174268e9cfdeeaa98b7cfba747515 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Wed, 8 Nov 2023 10:22:36 +0100 Subject: [PATCH 4/6] Comment test and add extra assert --- package_test.go | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/package_test.go b/package_test.go index 46bf31a1..9f9944e7 100644 --- a/package_test.go +++ b/package_test.go @@ -1002,6 +1002,27 @@ func (s *PackageSuite) TestTransactions(c *C) { c.Assert(err, IsNil) } +// Test that when preparing a statement inside a transaction it can still be prepared on the db directly and that it is +// not closed along with the transaction. +func (s *PackageSuite) TestStatementTXReuse(c *C) { + sqldb, err := setupDB() + c.Assert(err, IsNil) + + db := sqlair.NewDB(sqldb) + + // Create a statement and run it on a transaction. + selectStmt := sqlair.MustPrepare(`SELECT 'hello'`) + tx, err := db.Begin(nil, nil) + c.Assert(err, IsNil) + q := tx.Query(nil, selectStmt) + c.Assert(q.Run(), IsNil) + c.Assert(tx.Commit(), IsNil) + + // Run the same existing statement outside the transaction. + q = db.Query(nil, selectStmt) + c.Assert(q.Run(), IsNil) +} + func (s *PackageSuite) TestTransactionErrors(c *C) { tables, sqldb, err := personAndAddressDB() c.Assert(err, IsNil) @@ -1373,22 +1394,3 @@ func (s *PackageSuite) TestRaceConditionFinalizerTX(c *C) { // Assert that sql.Stmt was not closed early. c.Assert(q.Run(), IsNil) } - -func (s *PackageSuite) TestStatementTXReuse(c *C) { - sqldb, err := setupDB() - c.Assert(err, IsNil) - - db := sqlair.NewDB(sqldb) - - // Create a statement and run it on a transaction. - selectStmt := sqlair.MustPrepare(`SELECT 'hello'`) - tx, err := db.Begin(nil, nil) - c.Assert(err, IsNil) - q := tx.Query(nil, selectStmt) - c.Assert(q.Run(), IsNil) - tx.Commit() - - // Run the same existing statement outside the transaction. - q = db.Query(nil, selectStmt) - c.Assert(q.Run(), IsNil) -} From 622fc8b38566bb6863c12a3b045d1de237834dea Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Thu, 30 Nov 2023 09:25:19 +0100 Subject: [PATCH 5/6] Add argument --- package_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package_test.go b/package_test.go index e0fa588e..d46ae4f0 100644 --- a/package_test.go +++ b/package_test.go @@ -1023,7 +1023,7 @@ func (s *PackageSuite) TestTransactions(c *C) { // Test that when preparing a statement inside a transaction it can still be prepared on the db directly and that it is // not closed along with the transaction. func (s *PackageSuite) TestStatementTXReuse(c *C) { - sqldb, err := setupDB() + sqldb, err := setupDB(c.TestName()) c.Assert(err, IsNil) db := sqlair.NewDB(sqldb) From 8add63b75bfdcb1a47e5a02c0635fe0cbe260221 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Mon, 4 Dec 2023 16:15:35 +0100 Subject: [PATCH 6/6] Fix test not passing --- package_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/package_test.go b/package_test.go index d46ae4f0..77641855 100644 --- a/package_test.go +++ b/package_test.go @@ -1388,6 +1388,11 @@ func (s *PackageSuite) TestRaceConditionFinalizer(c *C) { } func (s *PackageSuite) TestRaceConditionFinalizerTX(c *C) { var q *sqlair.Query + // Because of how the sql internal library works, we have to commit the transaction for the statement to be closed + // properly and the test to pass. Reason: the sql library keeps track of what connections are used for transactions + // and, when they are, it does not close statements, instead it marks them to be closed only when the connection + // itself is closed. + var tx *sqlair.TX // Drop all the values except the query itself. func() { sqldb, err := setupDB(c.TestName()) @@ -1396,7 +1401,7 @@ func (s *PackageSuite) TestRaceConditionFinalizerTX(c *C) { db := sqlair.NewDB(sqldb) selectStmt := sqlair.MustPrepare(`SELECT 'hello'`) - tx, err := db.Begin(nil, nil) + tx, err = db.Begin(nil, nil) c.Assert(err, IsNil) q = tx.Query(nil, selectStmt) }() @@ -1409,4 +1414,5 @@ func (s *PackageSuite) TestRaceConditionFinalizerTX(c *C) { // Assert that sql.Stmt was not closed early. c.Assert(q.Run(), IsNil) + c.Assert(tx.Commit(), IsNil) }