golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.28k stars 17.7k forks source link

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

Open cznic opened 10 years ago

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

#Go 1.4+
ianlancetaylor commented 10 years ago

Comment 1:

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

rsc commented 10 years ago

Comment 2:

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

Status changed to Accepted.

amoghe commented 9 years ago

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 commented 9 years ago

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

amoghe commented 9 years ago

:'(

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

ianlancetaylor commented 9 years ago

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 commented 9 years ago

I'll take a stab at it.

amoghe commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

amoghe commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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.

adg commented 8 years ago

There seems to be agreement that something should be done here, but we need someone to push it forward with a formal proposal.

kardianos commented 8 years ago

@bradfitz @amoghe @cznic I propose we add to database/sql

func (*Tx) RollbackTo(name string) error
func (*Tx) SavePoint(name string) error

And add to database/sql/driver

type TxSavePoint interface {
  RollbackTo(ctx context.Context, name string) error
  SavePoint(ctx context.Context, name string) error
}

As @kostya-sh noted, nested transactions have interesting semantics and are generally not encouraged at this time. RDMBS ask you to use save points to support such needs.

Depends on #15123

bradfitz commented 8 years ago

@kardianos, you want to see this happen for Go 1.8? Which 2+ popular databases support this? Could you implement support for at least one popular database for Go 1.8 if we do this?

kardianos commented 8 years ago

MySql, Postgresql, MS SQL Server, Oracle, and sqlite all support this.

Once the context gets generally added, I'll work on this.

amoghe commented 8 years ago

@kardianos I can take a stab at the sqlite3 implementation.

However, I'm not sure thats the interface that'll work best for sqlite3. They recommend emulating nested transactions using savepoints. From their documentation:

SAVEPOINTs are a method of creating transactions, similar to BEGIN and COMMIT, except that the SAVEPOINT and RELEASE commands are named and may be nested

So exposing "SavePoint" on the Tx struct would mean that the user has already begun the txn using a non-savepoint mechanism. This is why my initial proposal was to modify the Txn interface itself. This is not to say that this interface won't work for sqlite3 , but I can imagine there exists some SQL database which needs you to use either transactions (BEGIN...COMMIT) or savepoints exclusively (without intermingling the two).

Thoughts?

kardianos commented 8 years ago

@amoghe wait to implement quite yet. Here is what I would suggest: One of the parameters to start a transaction (coming in DB.Being{Context | Level} can take a name. When the name is provided you start a savepoint (named tx). If you attempt a savepoint through a normal tx, then it will give you an error. Thoughts?

gopherbot commented 8 years ago

CL https://golang.org/cl/30165 mentions this issue.

cznic commented 8 years ago

CL 30165 adds support for save points. I don't see how this enables nested transaction support, which is what this issue is about.

Additionally, even though a particular database supports nested transactions, but not savepoints, like for example QL, the new API in driver.go seems to be not compatible with such databases capability. I'd be glad to be wrong, perhaps explain the mechanism to me, TIA.

I'm sorry to comment this late on the issue, but the subscription to notifications for issues filled before moving to Github were lost in the transition (known issue).

kardianos commented 8 years ago

@cznic Fair points. This CL does NOT support nested transactions. The databases I'm aware of (please correct me if I'm wrong) recommend not using true tested transactions, but switch to using savepoint / rollback to savepoint method.

There are a number of changes to the database/sql package, some are CL / being written, others committed. Until 1.8 comes I want as much feedback from other database gophers as possible.

kardianos commented 8 years ago

@cznic More specifically, I think you can use nested transactions on top of savepoint, rollback.

What if you maintained a savepoint to nest level mapping in your driver and then translated the name into the appropriate tx to rollback. After a savepoint, you would need to (under the hood) start using the new Tx in the driver. I think it is doable. Thoughts?

cznic commented 8 years ago

What if you maintained a savepoint to nest level mapping in your driver and then translated the name into the appropriate tx to rollback.

@kardianos That might be the way. Rollback to a savepoint which is not the innermost one would effectively erase all savepoints "bellow" it, but otherwise SGTM, IINM :+1:

amoghe commented 8 years ago

Apologies if I sound like I'm repeating myself. I'm trying to figure out of this discussion is tracking what the user facing API offered by the stdlib should be, or the underlying implementation, or something else completely? (like the viability of even supporting this?).

Assuming we're still discussing what uses might want from the sql pkg API ...

What if you maintained a savepoint to nest level mapping in your driver and then translated the name into the appropriate tx to rollback.

What option 1 proposes (see beginning of thread) are the API level changes we might need to support nested transactions. Its up to the driver to either emulate nested txns using savepoints, or maybe use true nested txns if supported by the database. I'm assuming the user facing API for this should allow for this kind of usage:

tx1 := db.Begin()
// do some stuff
tx2 := tx1.Begin()
// more stuff
tx2.Commit()
tx1.Commit()

This API would allow for the txn object (or maybe the driver) to track any names it may have generated for the savepoints and the appropriate rollback logic.

Thoughts?

kardianos commented 8 years ago

@amoghe Did you see the proposed CL? https://golang.org/cl/30165

amoghe commented 8 years ago

@kardianos I'm sorry I completely missed that. Thank you for bringing it to my attention.

After reading the CL, it seems like the API presented by the database/sql pkg would need me to begin a transaction before I can use savepoints. This means that when the txn is created, a BEGIN is issued (because at that point the driver must begin a transaction on the underlying db), and then subsequently multiple SAVEPOINTS may be issued. A couple of reservations about this...

Thoughts?

amoghe commented 8 years ago

PS - I'm leaving these comments here because they're about the general approach to extending API offered by database/sql, and not the specific implementation proposed in the CL.

If you think that the discussion should be continued on the CL, LMK and I'll repost them there.

kardianos commented 8 years ago

@amoghe What databases and database protocols are you familiar with? All the systems I'm aware of require a savepoint after a tran has started. With exception of sqlite, but that's sqlite.

amoghe commented 8 years ago

I'm only familiar with SQLite3 and MySQL. The difference in behaviors of just these two databases suggests (to me) that there might exist other behaviors that don't follow the transaction-followed-by-savepoint pattern.

kardianos commented 8 years ago

@amoghe I'm aware that sqlite is slightly different and there will be a work around. Every other rdmbs I'm aware of has savepoint rollback to after tran begin.

wkhere commented 8 years ago

2cents,

I just found this discussion when googling for any references on nested transactions in Go.

I believe a valid and obvious case for nested transactions is testing. When you test the code which already uses transactions (because operations being tested need transaction semantics), you want to wrap such code in a bigger transaction which will rollback after running test(s). You also want the whole machinery to be transparent - ie. to not introduce any additional concepts like savepoints just because for some cases you want your transactions to be part of a bigger one.

I was using this approach over years in testing with Postgres (8.x, 9.x) and application-level drivers (SQLAlchemy, recently Ecto, IIRC it was also possible with ActiveRecord as long ago as in its 2.x times). Looks like the effort of implementing nested transactions with savepoints was done in these drivers.

So the approach @amoghe proposes is used "in the wild" and I believe there's a value in it.

kardianos commented 8 years ago

@herenowcoder I've never used or heard of your use case. That certainly doesn't mean it is invalid; it sounds interesting.

Every time I've looked into this I've found that databases don't support nested transactions, they support savepoints. MySql and Postgresql only support savepoints. MS SQL Server kinda supports nested transactions, but not really (the outer transaction rules the inner one, the inner one doesn't even count), but again it does fully support save points.

My takeaway is that savepoints are what databases are doing, not nested transactions.

wkhere commented 8 years ago

@kardianos

I also believe pointing to a distinction between sql driver and sql "framework" (a word lacking a proper definition btw) is not so much applicable here. In Golang ecosystem ORMs don't seem so popular and there must be a reason for this. I already have cases where I would want nested transaction without ORM "framework" thing.

bradfitz commented 8 years ago

I think we're going to kick this until Go 1.9. I'd rather not rush this in.

There are tons of database changes in Go 1.8 to keep at least some people happy.

rubensayshi commented 7 years ago

I think it would be really nice if https://github.com/golang/go/issues/14468 was done before this, this would allow downstream code to take a Queryable (or DBI or w/e name you'd give it) and not have to care if it's a DB, Tx or NestedTx when running queries.

kardianos commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

That would be wonderful. Thanks @kardianos!

kardianos commented 7 years ago

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

amoghe commented 7 years ago

@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 commented 7 years ago

Optional opt in interfaces. Lots sql/driver.

On Wed, May 17, 2017, 18:15 Akshay Moghe notifications@github.com wrote:

@kardianos https://github.com/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 https://github.com/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?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/7898#issuecomment-302273409, or mute the thread https://github.com/notifications/unsubscribe-auth/AAuFsUfZMFnWI3KSvuS8tCC_a6Zs9HqDks5r65uagaJpZM4FWWOn .

wkhere commented 7 years ago

@kardianos in short - looks cool!

JimNasbyGE commented 7 years ago

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.

robcapo commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

@sammy007 have you digested the case described in this thread, with testing the code which uses transactions?