Best way to handle database transactions? (commit, or rollback on error)

[Ben Hoyt]

Hi folks,

We found some subtle bugs in our db transaction code for handling commits/rollbacks. Here's the pattern we were using (not real, but shows the issue):

func DoTwoThings() error {

    tx, err := db.Begin()

    if err != nil {

        return err

    }

    // commit or rollback the transaction before we return

    defer tx.Close(&err)

    err := InsertFoo(tx)

    if err != nil {

        return err

    }

    if _, err := UpdateBar(tx); err != nil {

        return err

    }

    return nil

}

The problem is there's a subtle but potentially quite bad bug with this usage pattern -- if the InsertFoo succeeds but UpdateBar fails, the first "err" variable will be nil, so the deferred tx.Close() will COMMIT the transaction rather than ROLLBACK, and the database will be in an inconsistent state.

The code above is a bit contrived, and you can easily fix it by moving the "_, err := UpdateBar()" outside of the if so it's referring to the same "err" variable, but it's very easy to miss and get it wrong. So we decided it was a bad pattern and started thinking about the best way to fix.

One idea is a RollbackUnlessCommitted() function which you can defer, and then you call Commit() once manually (stolen from gocraft/dbr):

func DoTwoThings() error {

    tx, err := db.Begin()

    if err != nil {

        return err

    }

    defer tx.RollbackUnlessCommitted()

    err := InsertFoo(tx)

    if err != nil {

        return err

    }

    if _, err := UpdateBar(tx); err != nil {

        return err

    }

    tx.Commit()

    return nil

}

Another idea is to create a "Transact" function which takes an anonymous function and does all the transaction handling:

func (db *DatabaseImpl) Transact(txFunc func() error) (err error) {

    tx, err := db.Begin()

    if err != nil {

        return

    }

    defer func() {

        if p := recover(); p != nil {

            tx.Rollback()

            panic(p) // re-throw panic after Rollback

        } else if err != nil {

            tx.Rollback() // err is non-nil; don't change it

        } else {

            err = tx.Commit() // err is nil; if Commit returns error update err

        }

    }()

    err = txFunc(tx)

    return err

}

And then the DoTwoThings function becomes:

func DoTwoThings() error {

    return db.Transact(func() error) {

        err := InsertFoo(tx)

        if err != nil {

            return err

        }

        if _, err := UpdateBar(tx); err != nil {

            return err

        }

    })

}

I think the second is probably safer and nicer, but it's slightly awkward in that it requires an extra level of indentation. Still, awkward is better than buggy.

Does anyone else have a better pattern for this kind of thing, or feedback on the above?

-Ben

[Robert Engels]

How can you write this

err := InsertFoo(tx)

Don’t you get no new variables defined error here?

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ben Hoyt]

Ah, quite right. That's what comes of trying to modify the code snippet from our actual code on the fly. It was more like:

t, err := InsertFoo(tx)

-Ben

[Robert Engels]

Then there should only be a single err variable, and the address should not change. Not sure why it isn’t working.... are you sure you are not in a code block that is causing a shadowing of err?

[Leonel Quinteros]

Hi Ben, 

I'm pretty sure that the err variable is getting shadowed on your Update construct. 

Instead of doing: 

if _, err := UpdateBar(tx); err != nil {

    return err

}

You should do something like: 

_, err = UpdateBar(tx)

if err != nil {

    return err

}

Just like you do with the insert and avoiding the := operand

Let me know if that works. 

Best! 

Leonel

[Ben Hoyt]

Robert, here is code (actual working code this time) similar to what we have: https://play.golang.org/p/jUPqgnk6Ttk

Leonel, yep, I understand that, which is why we have this problem. The thing is, this pattern makes it very easy to forget to always re-use the same error variable, especially in code that's a bit more complex with a bunch of nested ifs, etc. So we're trying to come up with a pattern that's hard or impossible to misuse, instead of easy to misuse.

-Ben

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/ge49ywYnjio/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.

[Robert Engels]

Go vet will report this as a problem. 

You received this message because you are subscribed to the Google Groups "golang-nuts" group.

To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

[Kevin Malachowski]

The best way to access the return value of a function is to name the return variable:

func DoTwoThings(db *Database) (err error) {

tx, err := db.Begin()

if err != nil {

return err

}

defer tx.Close(&err)

...

}

Then any accidental shadowing doesn't matter because the "defer" is referring to the actual return variable; it also means that you can support returning a new error directly (i.e. `return nil, fmt.Errorf(...)`) rather than needing to set the outer 'err' variable and then return it.

[ritt...@gmail.com]

In this situation, we normally use named return variables. That way we can just check if we are returning with an error in the defer.

func DoTwoThings() (retErr error) {

    tx, err := db.Begin()

    if err != nil {

        return err

    }

    defer func() {

        if retErr != nil {

            tx.Rollback()

        }

    }()

    ...

}

Note that this doesn't account for panics.

[Ben Hoyt]

Ah yes, that's a nice way -- thanks.

Still has the issue that it's fairly easy to forget ... oh well.

-Ben

[Tamás Gulácsi]

defer tx.Rollback()
...
tx.Commit() // at the end

[Manlio Perillo]

On Monday, December 3, 2018 at 10:53:30 PM UTC+1, Ben Hoyt wrote:

Hi folks,

We found some subtle bugs in our db transaction code for handling commits/rollbacks. Here's the pattern we were using (not real, but shows the issue):

func DoTwoThings() error {

    tx, err := db.Begin()

    if err != nil {

        return err

    }

    // commit or rollback the transaction before we return

    defer tx.Close(&err)

    err := InsertFoo(tx)

    if err != nil {

        return err

    }

    if _, err := UpdateBar(tx); err != nil {

        return err

    }

    return nil

}

The problem is there's a subtle but potentially quite bad bug with this usage pattern -- if the InsertFoo succeeds but UpdateBar fails, the first "err" variable will be nil, so the deferred tx.Close() will COMMIT the transaction rather than ROLLBACK, and the database will be in an inconsistent state.

The code above is a bit contrived, and you can easily fix it by moving the "_, err := UpdateBar()" outside of the if so it's referring to the same "err" variable, but it's very easy to miss and get it wrong. So we decided it was a bad pattern and started thinking about the best way to fix.

 

> [...]

 

Another idea is to create a "Transact" function which takes an anonymous function and does all the transaction handling:

func (db *DatabaseImpl) Transact(txFunc func() error) (err error) {

    tx, err := db.Begin()

    if err != nil {

        return

    }

    defer func() {

        if p := recover(); p != nil {

            tx.Rollback()

            panic(p) // re-throw panic after Rollback

        } else if err != nil {

            tx.Rollback() // err is non-nil; don't change it

        } else {

            err = tx.Commit() // err is nil; if Commit returns error update err

        }

    }()

    err = txFunc(tx)

    return err

}

I'm using a similar function but with a different implementation:

https://play.golang.org/p/4t4ls8JH8Ss

Currently does not handle panics.

I'm still experimenting with the database/sql package.

One problem is what happens if the function needs to commit some code.

Should you use nested transaction?  Or should you use reference counting to only commit the last transaction?

> [...]

Manlio

[Leonel Quinteros]

Hi Ben, 

Sorry, I've misread that part. 

Now that I got some sleep and reviewed your code a little bit more, it looks like you got trapped by your own design decisions. 

First of all, variable shadowing is something every Go developer should be able to handle gracefully, but I understand your concern about how easy it's to miss that bug. Doesn't the linter alert you about that shadowing process? Just curious about that. 

Then, the mistake, IMHO, is the use of the defer function and the custom tx.Close method (is it custom made, right? I haven't found a reference for that method in the sql.Tx type docs) that "saves" you from manually handling tx.Commit() vs tx.Rollback() calls, which if you think, it gets pretty well aligned to the error handling philosophy of Go. If you get an error, please go ahead and explicitly handle that error. 

So, your DoTwoThings() function is actually doing 3 things, the 2 things against the DB, but it's also handling the transaction. If you see these 2 DB queries as a unit, you may want to put these in a method that takes a sql.Tx object, executes queries in that context and return an error result that will indicate if all of them were successful or not. Then you Commit or Rollback the transaction in the caller, not in a deeper level. 

So pretty much as your Transact() method, but I'm not a fan of the defer use there, while I get how you managed to also catch panics there. 

Out of your 2 proposals, I like this one better. 

But if you ask me, I'd recommend to avoid the defer use, which is the thing that's actually causing the shadowing problem, and to enforce the manually handling of the transactions directly related to error handling. That's how I've been doing it and it's pretty clear and straightforward when you read that code, the Rollbacks after the errors and the Commits at the end of each function are just there, explicitly telling you what's happening when some DB query fails. 

But again, it's a matter of opinion, design and personal preferences. 

I just hope this helps for you to form an opinion. 

Best!

Leonel Quinteros

Director of Technology 

Global Brigades 

[Ben Hoyt]

Helpful comments and good suggestion, thanks Leonel. -Ben

[Eric Johnson]

I always go with the approach that uses the equivalent of the "Transact()" method.

On top of that, rather than use a generic *sql.Tx parameter to the "txFunc" function, I pass an interface that is specific to the operations of the database layer for my application.

This pattern has three benefits:

• Code that changes the database has standard Go semantics - if something goes wrong, return an error.
• Guaranteed to correctly call either "Commit" or "Rollback" as appropriate
• The specifics of the exact SQL queries is hidden behind the interface. Any database compatibility is now isolated in one place/package in the code, instead of having to look for all the places in your code where transactions might be used.

Eric.

[roger peppe]

On Wed, 5 Dec 2018 at 18:55, 'Eric Johnson' via golang-nuts <golan...@googlegroups.com> wrote:

I always go with the approach that uses the equivalent of the "Transact()" method.

On top of that, rather than use a generic *sql.Tx parameter to the "txFunc" function, I pass an interface that is specific to the operations of the database layer for my application.

That's an interesting idea. For the record, this is the code we use:

    package transaction

    

    import (

        "context"

        "database/sql"

        "fmt"

    

        "gopkg.in/errgo.v1"

    )

    

    // WithTxn executes a function inside a transaction context.

    func WithTxn(db *sql.DB, ctx context.Context, f func(*sql.Tx) error) error {

        txn, err := db.BeginTx(ctx, nil)

        if err != nil {

            return errgo.Mask(err)

        }

        if err := f(txn); err != nil {

            return rollback(txn, err)

        }

        return rollback(txn, txn.Commit())

    }

    

    func rollback(txn *sql.Tx, err error) error {

        if err == nil {

            return nil

        }

        errRollback := txn.Rollback()

        if errRollback != nil {

            return errgo.NoteMask(err, fmt.Sprintf("failed to roll back (error: %v) after", errRollback), errgo.Any)

        }

        return err

    }

[Eric Johnson]

On Thu, Dec 6, 2018 at 12:16 AM roger peppe <rogp...@gmail.com> wrote:

On Wed, 5 Dec 2018 at 18:55, 'Eric Johnson' via golang-nuts <golan...@googlegroups.com> wrote:

I always go with the approach that uses the equivalent of the "Transact()" method.

On top of that, rather than use a generic *sql.Tx parameter to the "txFunc" function, I pass an interface that is specific to the operations of the database layer for my application.

That's an interesting idea. For the record, this is the code we use:

This is one of *the* places in my code where I'd probably immediately take advantage of "generics" in Go. This function to wrap transactions could be written generically, and then not have to be copied from application to application. While I could go with the work-around to leave the function in the *sql.Tx parameter form, the other approach is *so* useful that I copy the function instead.

Eric

[roger peppe]

How would generics help you here? The WithTxn function is already generic (courtesy of closures) and there's no reason it couldn't be put in a publicly importable package somewhere.

[Eric Johnson]

On Thu, Dec 6, 2018 at 4:37 PM roger peppe <rogp...@gmail.com> wrote:

How would generics help you here? The WithTxn function is already generic (courtesy of closures) and there's no reason it couldn't be put in a publicly importable package somewhere.

When I use the equivalent functionality, I tie it to an interface specifically for the database changes / queries allowed. Makes for great separation between the database layer and the business logic. That means, however, every time I have a new database API, there's a new interface, and a new implementation of the function.

Eric.