Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cznic opened this issue Apr 30, 2014 · 59 comments
Open

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

cznic opened this issue Apr 30, 2014 · 59 comments

Comments

@cznic
Copy link
Contributor

@cznic cznic commented Apr 30, 2014

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

#Go 1.4+
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 30, 2014

Comment 1:

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

@rsc
Copy link
Contributor

@rsc rsc commented Sep 15, 2014

Comment 2:

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

Status changed to Accepted.

@cznic cznic added accepted labels Sep 15, 2014
@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@bradfitz bradfitz removed the release-go1.5 label Dec 16, 2014
@rsc rsc removed accepted labels Apr 14, 2015
@amoghe
Copy link

@amoghe amoghe commented Jul 10, 2015

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 10, 2015

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

@amoghe
Copy link

@amoghe amoghe commented Jul 10, 2015

:'(

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

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 10, 2015

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
Copy link

@amoghe amoghe commented Jul 10, 2015

I'll take a stab at it.

@amoghe
Copy link

@amoghe amoghe commented Jul 14, 2015

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.

@rsc rsc modified the milestones: Go1.6Early, Go1.5 Jul 14, 2015
@amoghe
Copy link

@amoghe amoghe commented Jul 22, 2015

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
Copy link
Contributor

@bradfitz bradfitz commented Jul 22, 2015

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
Copy link
Contributor

@kostya-sh kostya-sh commented Sep 16, 2015

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

@amoghe
Copy link

@amoghe amoghe commented Sep 16, 2015

@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
Copy link
Contributor

@kostya-sh kostya-sh commented Sep 17, 2015

@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
Copy link
Contributor

@rsc rsc commented Nov 4, 2015

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.

@rsc rsc changed the title database/sql: Nested transaction support. proposal: database/sql: nested transaction support Nov 4, 2015
@rsc rsc modified the milestones: Proposal, Go1.6Early Nov 4, 2015
@rsc rsc added the Proposal label Nov 4, 2015
@adg adg added the help wanted label Jul 19, 2016
@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 30, 2017

@rubensayshi I understand this sentiment. For simple queries you are 100% correct, that would be nice.

Here is what's currently stopping me from making a Queryable interface that can perform nested transactions: SQL doesn't work that way.

Let's assume a Queryable interface that allowed nested transactions in the API. Your function takes a func(ctx context.Context, q Queryable) error right? All good. Now let's take a look at some SQL text we might want to run in this queryable.

select * from Account;
begin tran;
insert into Account(ID)
select 42;
commit tran;

This is a perfectly good SQL text if you aren't already in a transaction. But if you ARE in a transaction, it will blow up. However, if you make it a policy to never start transactions within SQL text, this would work just fine.

...

In my own code bases, I do have a Queryable interface, but I use it only for executing queries in a single location with some extra handling around it. When I define SQL text, I have the type information that shows me if I'm in a Tx or not.

All that being said, I'm open to be persuaded otherwise. What I think I would need is the correct wording for the documentation and general advice when and when not to use these functions (nested tran, queryable).

@timbunce
Copy link

@timbunce timbunce commented May 16, 2017

It seems to me that this 3 year old proposal has stumbled because it tried to cover both savepoints and nested transactions. (And nested transactions vs emulated nested transactions, and something like a Queryable interface that would work for DB, Tx, or Conn.)

In my experience, savepoints are valuable when truly needed but are relatively rarely needed. On the other hand, simple emulated nested transactions can be very useful for the reasons @wkhere pointed out.

Generally transactions should be defined at the outermost level of the code. If that code then gets used by some other code, such as when testing or by some another package, then that new outermost code should be able to control the transactions on that database connection.

func original_func() {
    tx, err := db.Begin()
    ....
    tx.Commit()  // tx.Commit (and tx.Rollback) do nothing when called via other_func()
}
func other_func() {
    tx, err := db.Begin()
    original_func()
    tx.Rollback()
}

That's very simple to understand and requires no external API changes.

Good support for savepoints, on the other hand, is tricky and deserves a separate Issue so as not to hinder discussion of support for simple emulated nested transactions here. (Or perhaps close this and create two separate issues?)

p.s. I'm very new to Go so please forgive me if I'm misunderstanding something here, and for not trying to develop a patch myself.

@kardianos
Copy link
Contributor

@kardianos kardianos commented May 16, 2017

@timbunce Savepoint/Rollback support could be easily added; the reason the linked CL for savepoints was abandoned was that I was still unsure if we should support anything, or if we should support something different.

I haven't pushed a specific proposal yet because if people just want savepoints, they can run tx.ExecContext(ctx, "savepoint dancing_gophers;"). It would also be fairly easy to write a wrapper to create a POC for nested Tx using simple exec statements. So there isn't anything intrinsically blocking creating this. Right now I've been focusing on blocking issues; things that require API changes / additions to enable functionality.

Anything that can be proved out in a third party repo first, should be at this point. I have some experiments living at: https://github.com/golang-sql/sqlexp . If I have time I'll create a wrapper obj that implements the nested Tx.

@timbunce
Copy link

@timbunce timbunce commented May 17, 2017

If I have time I'll create a wrapper obj that implements the nested Tx.

That would be wonderful. Thanks @kardianos!

@kardianos
Copy link
Contributor

@kardianos kardianos commented May 18, 2017

First attempt at https://godoc.org/github.com/golang-sql/sqlexp/nest . Feedback?

@amoghe
Copy link

@amoghe amoghe commented May 18, 2017

@kardianos At first blush I'm quite excited by this. Thanks for taking the effort of whipping this up. Exposing Begin on an existing txn is what was originally discussed in 2015, however back then I noted that (repasting here to save folks the effort of scrolling back):

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

While I'm all for this approach (having used it successfully as described by @wkhere in ruby/rails) I'm not sure how we would avoid existing driver pkgs from breaking without introducing a new EnhancedTx or TxPlus or fixing as many drivers as we can before/after the introduction of such a change. Thoughts?

@kardianos
Copy link
Contributor

@kardianos kardianos commented May 18, 2017

@wkhere
Copy link

@wkhere wkhere commented May 19, 2017

@kardianos in short - looks cool!

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jul 22, 2017
@JimNasbyGE
Copy link

@JimNasbyGE JimNasbyGE commented Aug 15, 2017

Something to consider for savepoints; some databases (Oracle and Postgres at least) support RELEASE SAVEPOINT. Making use of that can become beneficial as you get several levels of nesting. If explicit support for savepoints is added, RELEASE should be included. It would be nice if @kardianos's nest package used them as well.

@kardianos kardianos modified the milestones: Go1.10, Unplanned Oct 31, 2017
@robcapo
Copy link

@robcapo robcapo commented Apr 17, 2018

I agree with @timbunce that in general, the savepoint API is not usually needed. I think the most common usecase is to allow composable functions to execute more than one SQL statement and guarantee that they finish with all-or-none. I think emulated nested transactions can solve this problem. Here's a really quickly thrown together example, which uses savepoints for its implementation:

type NestableTx struct {
	sql.Tx

	savePoint int
	next *NestableTx
	resolved bool
}

func (tx *NestableTx) Begin() (*NestableTx, error) {
	tx.next = &NestableTx{
		Tx:        tx.Tx,
		savePoint: tx.savePoint + 1,
	}

	_, err := tx.Exec("BEGIN SAVEPOINT SP" + strconv.Itoa(tx.next.savePoint))

	if err != nil {
		return nil, err
	}

	return tx.next, nil
}

func (tx *NestableTx) Rollback() error {
	tx.resolved = true

	if tx.savePoint > 0 {
		_, err := tx.Exec("ROLLBACK TO SAVEPOINT SP" + strconv.Itoa(tx.savePoint))
		return err
	}

	return tx.Tx.Rollback()
}

func (tx *NestableTx) Commit() error {
	if tx.next != nil && !tx.next.resolved {
		err := tx.next.Commit()
		if err != nil {
			return err
		}
	}

	tx.resolved = true

	if tx.savePoint > 0 {
		_, err := tx.Exec("RELEASE SAVEPOINT SP" + strconv.Itoa(tx.savePoint))

		return err
	}

	return tx.Tx.Commit()
}

I'm sure I'm missing some edge cases here, but my point is that a relatively simple implementation that supports emulated nested transactions can provide large value. Until Go has native support, I will likely just add an implementation like the above to my projects as needed.

@dhui
Copy link

@dhui dhui commented Jan 12, 2019

Here's my stab at the problem: github.com/dhui/satomic

satomic is based on sqlexp and implements "nested" transactions, using savepoints for "nesting".
It's designed for ease of use and will automatically rollback to the previous savepoint/transaction on errors/panics!
The test coverage is pretty good and most popular RDBMSs (postgres, mysql, mssql, and sqlite) are supported.

Keep in mind that the API isn't stable yet, so the interfaces may change.
Any feedback is most welcome!

@jonbodner
Copy link

@jonbodner jonbodner commented Jul 11, 2019

Is there still interest in adding this to Go 1.14 or 1.15? I've just written up a quick library for internal usage at my company, but it would be nicer to have nested transactions as part of the standard library.

I am happy to write up a simple spec and provide an implementation. I think it involves adding one new method to sql.Tx (Begin), updating the Commit and Rollback methods on sql.Tx to handle nested cases, and adding an interface to the driver package.

The rest of the work is really up to the db driver providers. I can see if I can release my internal implementation using savepoints; it's not a lot of code.

@wkhere

This comment was marked as off-topic.

@golang golang deleted a comment from sammy007 Jan 22, 2020
@golang golang deleted a comment from sammy007 Jan 22, 2020
@kardianos
Copy link
Contributor

@kardianos kardianos commented Jan 23, 2020

@jonbodner This proposal has been accepted, and I would welcome an implementation, esp if the code doesn't complicate the existing implementation. This would be API support for nested transactions; the benefit of being able to create a consistent Queryer interface over both DB and Tx is too large to implement the more "raw" savepoint interface.

I've thought about the benefit/cost working through various line of business applications. I agree, it would be good to have this API.

@amoghe
Copy link

@amoghe amoghe commented Jan 23, 2020

@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
Copy link

@jonbodner jonbodner commented Jan 30, 2020

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

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jan 30, 2020

@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
Copy link

@jonbodner jonbodner commented Jan 30, 2020

@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
Copy link

@jonbodner jonbodner commented Jan 30, 2020

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

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jan 30, 2020

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

  1. I would avoid random, and just name a sequential name like from the save point name index.
  2. I would replace the ExecContext calls with a new interface defined on the tx.txi.
  3. 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.
  4. 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
Copy link

@jonbodner jonbodner commented Jan 30, 2020

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
Copy link
Contributor

@kardianos kardianos commented Jan 30, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.