database/sql: nested transaction or save point support #7898

cznic · 2014-04-30T07:30:39Z

It might be useful to consider supporting nested transactions when a particular
driver/DB combo is able to support that.

#Go 1.4+

ianlancetaylor · 2014-04-30T12:31:07Z

Comment 1:

Labels changed: added repo-main, release-go1.4.

rsc · 2014-09-15T16:19:34Z

Comment 2:

Labels changed: added release-go1.5, removed release-go1.4.

Status changed to Accepted.

amoghe · 2015-07-10T19:54:18Z

Github reports this as targetting 1.5, but the 1.5 beta release has no relevant changes to the database/sql package.

Any updates on whether this will make it or be pushed out once again?

ianlancetaylor · 2015-07-10T21:32:08Z

I think it's getting pushed out again. Sorry.

amoghe · 2015-07-10T22:28:31Z

:'(

Datapoint: This (lack of) caused me much pain when migrating an older project from ruby(rails) to go.

ianlancetaylor · 2015-07-10T22:42:54Z

I completely agree that this is desirable, but somebody has to step up and do the work. Perhaps you could tackle it, for 1.6?

amoghe · 2015-07-10T23:02:02Z

I'll take a stab at it.

amoghe · 2015-07-14T07:59:39Z

In order to support nested transactions, I see two obvious alternatives.

TL;DR - Option 2 is better.

Option 1

Make the driver.Tx interface include an additional function

type Tx interface {
        Commit() error
        Rollback() error
        Begin() (*Tx, error)
}

The Begin() function can return an error indicating that transactions cannot be nested (any further). This would allow the sql.Tx object to expose a Begin(), which directly calls the underlying Begin function on the driver.Tx (which it wraps). E.g.

// - - - - - - - - - - - - - - - - - 
// In package sql
//

func (tx *Tx) Begin() (*Tx, error) {
      return tx.txi.Begin()
}

// - - - - - - - - - - - - - - - - - 
// In the driver implementation package
//

func (t *MyTx) Begin() (*Tx, error) {
      // error sanity checks
      _, err  := t.conn.Exec(...) // execute a SAVEPOINT maybe?
      if err != nil {
            return nil, err
      }
      newTx := t.clone()
      // some bookkeeping for newTx
      return &newTx, nil
}

The immediate drawback of this approach is that the next release will mean existing DB drivers will immediately stop compiling since their implementations of transaction object will no longer satisfy the driver.Tx interface.

However, the benefit is that having the transaction struct (that implements driver.Tx) implement the function that begins the nested transaction feels more natural. It is likely* that the transaction already holds a reference to the driver.Conn that it is tied to, so all the state it needs is already present in the transaction struct itself (* = my reading of 2 driver implementations)

Option 2

Have driver.Conn optionally implement an additional interface (driver.NestedBeginner?) which indicates that it can begin nested transactions

type NestedBeginner interface {
  NestedBegin(origTx *Tx) Tx
}

And expose this on the sql.Tx object as a Begin() function, which calls the NestedBegin() on the conn object, passing it the current transaction. E.g.

// - - - - - - - - - - - - - - - - - 
// In package sql
//

var ErrNestedTxUnsupported = errors.New("sql: Driver does not support nested transactions")

func (tx *Tx) Begin() (*Tx, error) {
      if tx.done {
            return ErrTxDone
      }
      if beginner, ok := tx.dc.ci.(driver.NestedBeginner); ok {
            tx.dc.Lock()
            nestedTx, err := beginner.NestedBegin(tx.txi) 
            if err != nil {
                  tx.dc.Unlock()
                  return nil, err
            }
            tx.dc.Unlock()
            return &Tx{db:  tx.db, dc:  dc, txi: nestedTx}, nil
      }
      return nil, ErrNestedTxUnsupported
}

// - - - - - - - - - - - - - - - - - 
// In the driver implementation package
//

// This function makes the driver connection object satisfy the NestedBeginner interface
func (c *MyConn) NestedBegin(origTxn driver.Tx) (driver.Tx, error) {
      return origTxn.NestedBegin() 
}

func (t *MyTxn) NestedBegin() {
      // implementation (similar to proposal 1) goes here
}

The benefit of this approach is that nothing changes for existing driver implementations (they are deemed to not support nested transactions until the pkg maintainers make the Conn satisfy the new interface). The sql.Tx.Begin() returns an error if the underlying driver.Conn doesn't implement the function.

However, this means the driver.Conn has to implement the NestedBeginner interface. This, in turn, means that in order for a nested transaction to begin, the struct that implements the driver.Conn acts as a proxy to the actual function that likely needs to be invoked on the existing transaction, to start a new transaction. This could end up feeling slightly clunkier than the first option, although that is not as important as maintaining the interface contract for backwards compatibility.

Hence I believe that option 2 is the more desirable one. I'd appreciate thoughts/feedback on this.

Note

The semantics of the nested transaction (and how Commits/Rollback might cause interactions between the inner/outer transactions) are to be implemented by the underlying driver. The sql package simply propagates them.

Its not clear to me if these discussions are to be had on the golang-dev mailing list or on the bug itself (the instructions on golang.org didn't call it out explicitly). It seems that both places would be able to persist design discussions for posterity but I'm happy to post this on the mailing list if needed.

amoghe · 2015-07-22T01:00:07Z

ping.

Its not clear to me if these discussions are to be had on the golang-dev mailing list or on the bug itself (the instructions on golang.org didn't call it out explicitly)

Happy to post this wherever necessary so that this makes it into 1.6 (as tagged)

bradfitz · 2015-07-22T02:04:19Z

If there's a patch ready, send it via the normal channels and use the normal "Update #7898" or "Fixes #7898" in the commit message and the bot will ping this thread about it.

kostya-sh · 2015-09-16T17:07:10Z

@amoghe, what db with nested transactions support have you used? I beleive savepoints are more widely supported by db engines than nested transactions.

amoghe · 2015-09-16T18:06:47Z

@kostya-sh You are correct. This (proposed) interface would allow db drivers the ability to offer nested transactions in whatever way is most suitable for their particular database. It is likely that most of them will use savepoints to do this.

The alternative (IIUC, you are proposing?) is to offer savepoints in the stdlib api itself and leave the handling of nesting them to the user.

kostya-sh · 2015-09-17T00:12:23Z

@amoghe, you can use savepoints without any changes to database/sql. E.g. with postgresql:

tx, err := db.Begin()
tx.Exec("insert into t1 values ('go1')")
tx.Exec("savepoint s1")
tx.Exec("insert into t1 values ('go2')")
tx.Exec("rollback to savepoint s1")
tx.Commit()

rsc · 2015-11-04T19:11:08Z

We (or at least I) don't understand the semantics of what is being proposed, and there seems to be no agreement about what they should be. There is also no code, nor anyone signed up to write the code. Therefore this will not be in Go 1.6.

Moving this to the proposal process, since it seems like that more clearly reflect the current status. The next step would be to write a design doc outlining the approach to take. See golang.org/s/proposal for details. Thanks.

amoghe · 2020-01-23T23:44:16Z

@kardianos - is the accepted proposal the same one in https://godoc.org/github.com/golang-sql/sqlexp/nest ?

I'm a little confused, I can't tell where along the way the conversation put forth a proposal and got accepted by the go maintainers? Did it happen on golang-nuts?

jonbodner · 2020-01-30T00:18:47Z

@kardianos I can put in some work on this. Should I work from tip in the database/sql package?

kardianos · 2020-01-30T01:47:14Z

@jonbodner Yes, you will want to work from tip.

I'm a little worried about complicating the Tx struct and embedded stmt handling too much. Also running a transaction on the non-active Tx should return an error.

jonbodner · 2020-01-30T04:41:47Z

@kardianos I have a very simple first pass. Maybe this is too simple?

Index: src/database/sql/sql.go
<+>UTF-8
===================================================================
--- src/database/sql/sql.go	(revision 07957b794c7b99034f41976bbab560ff4615bbc4)
+++ src/database/sql/sql.go	(date 1580359014508)
@@ -28,6 +28,8 @@
 	"sync"
 	"sync/atomic"
 	"time"
+
+	"math/rand"
 )
 
 var (
@@ -1974,6 +1976,10 @@
 	// any held driverConn back to the pool.
 	releaseConn func(error)
 
+	// savePointNames are the names assigned to the savepoints used for nested
+	// transactions. It is set to nil when there are no nested transactions.
+	savePointNames []string
+
 	// done transitions from 0 to 1 exactly once, on Commit
 	// or Rollback. once done, all operations fail with
 	// ErrTxDone.
@@ -1994,6 +2000,25 @@
 	ctx context.Context
 }
 
+func makeName() string {
+	out := make([]byte,10)
+	for i := range out {
+		out[i] = 65 + byte(rand.Intn(26))
+	}
+	return string(out)
+}
+
+// Begin starts a nested transaction using a savepoint with a random name.
+func (tx *Tx) Begin() (*Tx, error) {
+	savePointName := makeName()
+	_, err := tx.ExecContext(tx.ctx, "SAVEPOINT "+ savePointName)
+	if err != nil {
+		return tx, err
+	}
+	tx.savePointNames = append(tx.savePointNames, savePointName)
+	return tx, nil
+}
+
 // awaitDone blocks until the context in Tx is canceled and rolls back
 // the transaction if it's not already done.
 func (tx *Tx) awaitDone() {
@@ -2074,8 +2099,15 @@
 	}
 }
 
-// Commit commits the transaction.
+// Commit commits the transaction. If there is a nested transaction active, the 
+// nested transaction savepoint is released.
 func (tx *Tx) Commit() error {
+	if len(tx.savePointNames) != 0 {
+		savePointName := tx.savePointNames[len(tx.savePointNames)-1]
+		tx.savePointNames = tx.savePointNames[:len(tx.savePointNames)-1]
+		_, err := tx.ExecContext(tx.ctx, "RELEASE SAVEPOINT " + savePointName)
+		return err
+	}
 	// Check context first to avoid transaction leak.
 	// If put it behind tx.done CompareAndSwap statement, we can't ensure
 	// the consistency between tx.done and the real COMMIT operation.
@@ -2121,8 +2153,15 @@
 	return err
 }
 
-// Rollback aborts the transaction.
+// Rollback aborts the transaction.  If there is a nested transaction active, the 
+// nested transaction savepoint is rolled back.
 func (tx *Tx) Rollback() error {
+	if len(tx.savePointNames) != 0 {
+		savePointName := tx.savePointNames[len(tx.savePointNames)-1]
+		tx.savePointNames = tx.savePointNames[:len(tx.savePointNames)-1]
+		_, err := tx.ExecContext(tx.ctx, "ROLLBACK TO " + savePointName)
+		return err
+	}
 	return tx.rollback(false)
 }

jonbodner · 2020-01-30T04:43:02Z

If there's a better forum for discussing this, please let me know.

kardianos · 2020-01-30T05:20:54Z

Reviewing here initially is fine. You can also submit the change for review: https://golang.org/doc/contribute.html#sending_a_change_gerrit

I would avoid random, and just name a sequential name like from the save point name index.
I would replace the ExecContext calls with a new interface defined on the tx.txi.
It would be good to use BeginTx too, but perhaps (for now, require TxOptions to be nil), but the first pass could just stick with Begin as that would be simpler.
I think it would be important for parent Tx to not be able to run a query (except rollback and commit) unless the child Tx has finished with rollback or commit.

Possible interface defined in database/sql/driver/driver.go:

type ChildTx interface {
    Savepoint(ctx context.Context, name string) error
    CommitTo(ctx context.Context, name string) error
    RollbackTo(ctx context.Context, name string) error
}

Different DBMSs use different syntax (MS SQL Server and Oracle use different syntax then MySQL.

In order to accomplish (4), many of the fields in *Tx would need to be defined on an internal new *txState struct that is shared between all related *Tx. The *Tx would then hold the current savepoint name (if any), possibly a ctx and release function (unsure), and a *txState that holds the stmts and txi.

jonbodner · 2020-01-30T15:02:54Z

On points 3 and 4, I think we can simplify greatly if we don't have a Tx.BeginTx that takes in a context:

While you can spawn multiple parallel transactions from a single sql.DB, I don't think it makes sense to say that you can create multiple parallel savepoints within a transaction. Is there a database where this isn't the case? Autonomous transactions in Oracle can only be declared in specific situations that I think are out of scope for this issue (https://oracle-base.com/articles/misc/autonomous-transactions). Since savepoints aren't independent, does it make sense for them to potentially have separate contexts?
The intent was to re-use the existing Tx instance (Tx.Begin returns back the instance), so there's no way for a parent Tx to run a query outside of the nested transaction. If there's a possibility of a separate context for a nested transaction, it isn't possible to return back the same instance, which means we need to put flags into parent transactions to return an error if the user attempts to run a query while there's a live child context.

I have another implementation that I can submit for review via gerrit, but I wanted to get your feedback first.

kardianos · 2020-01-30T16:06:57Z

Sounds good. I'm fine leaving BeginTx out, at least of this initial implementation.
I agree that you don't want "allow" parallel transactions (most if not all that would be impossible).

If this is implemented, I'm convinced we will need to turn *Tx into some type of wrapper for an inner *txState. Without this, we will be unable to prevent parent queries to be run on "child" transactions.

If this sounds good, we can move over further review to gerrit. But design overview and questions are great to have here first.

… River Here, add a new minimal driver called `riverdriver/riversql` that supports Go's built-in `database/sql` package, but only for purposes of migrations. The idea here is to fully complete #57 by providing a way of making `rivermigrate` interoperable with Go migration frameworks that support Go-based migrations like Goose, which provides hooks for `*sql.Tx` [1] rather than pgx. `riverdriver/riversql` is not a full driver and is only meant to be used with `rivermigrate`. We document this clearly in a number of places. To make a multi-driver world possible with River, we have to start the work of building a platform that does more than `riverpgxv5`'s "cheat" workaround. This works by having each driver implement specific database operations like `MigrationGetAll`, which target their wrapped database package of choice. This is accomplished by having each driver bundle in its own sqlc that targets its package. So `riverpgxv5` has an `sqlc.yaml` that targets `pgx/v5`, while `riversql` has one that targets `database/sql`. There's some `sqlc.yaml` duplication involved here, but luckily both drivers can share a `river_migration.sql` file that contains all queries involved, so you only need to change one place. `river_migration.sql` also migrates entirely out of the main `./internal/dbsqlc`. The idea here is that eventually `./internal/dbsqlc` will disappear completely, usurped entirely by driver-specific versions. As this is done, all references to `pgx` will disappear from the top-level package. There are some complications here to figure out like `LISTEN`/`NOTIFY` though, and I'm not clear whether `database/sql` could ever become a fully functional driver as it might be missing some needed functionality (e.g. subtransactions are still not supported after talking about them for ten f*ing years [2]. However, even if it's not, the system would let us support other fully functional packages or future major versions of pgx (or even past ones like `pgx/v4` if there's demand). `river/riverdriver` becomes a package as it now has types in it that need to be referenced by driver implementations, and this would otherwise not be possible without introducing a circular dependency. Notably, this development branch has to use some `go.mod` `replace` directives to demonstrate that it works correctly. If we go this direction, we'll need to break it into chunks to release it without them: 1. Break out changes to `river/riverdriver`. Tag and release it. 2. Break out changes to `riverdriver/river*` drivers. Have them target the release in (1), comment out `replace`s, then tag and release them. 3. Target the remaining River changes to the releases in (1) and (2), comment out `replace`s, then tag and release the top-level driver. Unfortunately future deep incisions to drivers will require similar gymnastics, but I don't think there's a way around it (we already have this process except it's currently two steps instead of three). The hope is that these will change relatively rarely, so it won't be too painful. [1] https://github.com/pressly/goose#go-migrations [2] golang/go#7898

… River Here, add a new minimal driver called `riverdriver/riversql` that supports Go's built-in `database/sql` package, but only for purposes of migrations. The idea here is to fully complete #57 by providing a way of making `rivermigrate` interoperable with Go migration frameworks that support Go-based migrations like Goose, which provides hooks for `*sql.Tx` [1] rather than pgx. `riverdriver/riversql` is not a full driver and is only meant to be used with `rivermigrate`. We document this clearly in a number of places. To make a multi-driver world possible with River, we have to start the work of building a platform that does more than `riverpgxv5`'s "cheat" workaround. This works by having each driver implement specific database operations like `MigrationGetAll`, which target their wrapped database package of choice. This is accomplished by having each driver bundle in its own sqlc that targets its package. So `riverpgxv5` has an `sqlc.yaml` that targets `pgx/v5`, while `riversql` has one that targets `database/sql`. There's some `sqlc.yaml` duplication involved here, but luckily both drivers can share a `river_migration.sql` file that contains all queries involved, so you only need to change one place. `river_migration.sql` also migrates entirely out of the main `./internal/dbsqlc`. The idea here is that eventually `./internal/dbsqlc` will disappear completely, usurped entirely by driver-specific versions. As this is done, all references to `pgx` will disappear from the top-level package. There are some complications here to figure out like `LISTEN`/`NOTIFY` though, and I'm not clear whether `database/sql` could ever become a fully functional driver as it might be missing some needed functionality (e.g. subtransactions are still not supported after talking about them for ten f*ing years [2]. However, even if it's not, the system would let us support other fully functional packages or future major versions of pgx (or even past ones like `pgx/v4` if there's demand). `river/riverdriver` becomes a package as it now has types in it that need to be referenced by driver implementations, and this would otherwise not be possible without introducing a circular dependency. [1] https://github.com/pressly/goose#go-migrations [2] golang/go#7898

… River (#98) Here, add a new minimal driver called `riverdriver/riversql` that supports Go's built-in `database/sql` package, but only for purposes of migrations. The idea here is to fully complete #57 by providing a way of making `rivermigrate` interoperable with Go migration frameworks that support Go-based migrations like Goose, which provides hooks for `*sql.Tx` [1] rather than pgx. `riverdriver/riversql` is not a full driver and is only meant to be used with `rivermigrate`. We document this clearly in a number of places. To make a multi-driver world possible with River, we have to start the work of building a platform that does more than `riverpgxv5`'s "cheat" workaround. This works by having each driver implement specific database operations like `MigrationGetAll`, which target their wrapped database package of choice. This is accomplished by having each driver bundle in its own sqlc that targets its package. So `riverpgxv5` has an `sqlc.yaml` that targets `pgx/v5`, while `riversql` has one that targets `database/sql`. There's some `sqlc.yaml` duplication involved here, but luckily both drivers can share a `river_migration.sql` file that contains all queries involved, so you only need to change one place. `river_migration.sql` also migrates entirely out of the main `./internal/dbsqlc`. The idea here is that eventually `./internal/dbsqlc` will disappear completely, usurped entirely by driver-specific versions. As this is done, all references to `pgx` will disappear from the top-level package. There are some complications here to figure out like `LISTEN`/`NOTIFY` though, and I'm not clear whether `database/sql` could ever become a fully functional driver as it might be missing some needed functionality (e.g. subtransactions are still not supported after talking about them for ten f*ing years [2]. However, even if it's not, the system would let us support other fully functional packages or future major versions of pgx (or even past ones like `pgx/v4` if there's demand). `river/riverdriver` becomes a package as it now has types in it that need to be referenced by driver implementations, and this would otherwise not be possible without introducing a circular dependency. [1] https://github.com/pressly/goose#go-migrations [2] golang/go#7898

cznic added accepted labels Sep 15, 2014

bradfitz modified the milestone: Go1.5 Dec 16, 2014

bradfitz removed the release-go1.5 label Dec 16, 2014

rsc removed accepted labels Apr 14, 2015

rsc modified the milestones: Go1.6Early, Go1.5 Jul 14, 2015

rsc changed the title ~~database/sql: Nested transaction support.~~ proposal: database/sql: nested transaction support Nov 4, 2015

rsc modified the milestones: Proposal, Go1.6Early Nov 4, 2015

rsc added the Proposal label Nov 4, 2015

adg added the help wanted label Jul 19, 2016

jszwedko mentioned this issue Apr 15, 2020

Proof-of-concept of passing an interface rather than *sql.DB to database drivers golang-migrate/migrate#374

Draft

rsc unassigned bradfitz and kardianos Jun 23, 2022

ganigeorgiev mentioned this issue Dec 18, 2022

authWithOAuth2 is not working in v0.10.0 pocketbase/pocketbase#1291

Closed

brandur mentioned this issue Dec 3, 2023

Support for database/sql in migrations + framework for multi-driver River riverqueue/river#98

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

database/sql: nested transaction or save point support #7898

database/sql: nested transaction or save point support #7898

cznic commented Apr 30, 2014

ianlancetaylor commented Apr 30, 2014

rsc commented Sep 15, 2014

amoghe commented Jul 10, 2015

ianlancetaylor commented Jul 10, 2015

amoghe commented Jul 10, 2015

ianlancetaylor commented Jul 10, 2015

amoghe commented Jul 10, 2015

amoghe commented Jul 14, 2015

amoghe commented Jul 22, 2015

bradfitz commented Jul 22, 2015

kostya-sh commented Sep 16, 2015

amoghe commented Sep 16, 2015

kostya-sh commented Sep 17, 2015

rsc commented Nov 4, 2015

amoghe commented Jan 23, 2020

jonbodner commented Jan 30, 2020

kardianos commented Jan 30, 2020

jonbodner commented Jan 30, 2020

jonbodner commented Jan 30, 2020

kardianos commented Jan 30, 2020

jonbodner commented Jan 30, 2020

kardianos commented Jan 30, 2020

database/sql: nested transaction or save point support #7898

database/sql: nested transaction or save point support #7898

Comments

cznic commented Apr 30, 2014

ianlancetaylor commented Apr 30, 2014

rsc commented Sep 15, 2014

amoghe commented Jul 10, 2015

ianlancetaylor commented Jul 10, 2015

amoghe commented Jul 10, 2015

ianlancetaylor commented Jul 10, 2015

amoghe commented Jul 10, 2015

amoghe commented Jul 14, 2015

Option 1

Option 2

Note

amoghe commented Jul 22, 2015

bradfitz commented Jul 22, 2015

kostya-sh commented Sep 16, 2015

amoghe commented Sep 16, 2015

kostya-sh commented Sep 17, 2015

rsc commented Nov 4, 2015

amoghe commented Jan 23, 2020

jonbodner commented Jan 30, 2020

kardianos commented Jan 30, 2020

jonbodner commented Jan 30, 2020

jonbodner commented Jan 30, 2020

kardianos commented Jan 30, 2020

jonbodner commented Jan 30, 2020

kardianos commented Jan 30, 2020