2

While I was reading the example of "Prepared" statement in "transaction" in golang SQL/database example. One of the line says, "danger", yet the code example was provided without an alternative.

I wanted to have more clear explanation on below query, as not much information was provided on Wiki page at - http://go-database-sql.org/prepared.html

tx, err := db.Begin()
if err != nil {
    log.Fatal(err)
}
defer tx.Rollback()
stmt, err := tx.Prepare("INSERT INTO foo VALUES (?)")
if err != nil {
    log.Fatal(err)
}
defer stmt.Close() // danger!
for i := 0; i < 10; i++ {
    _, err = stmt.Exec(i)
    if err != nil {
        log.Fatal(err)
    }
}
err = tx.Commit()
if err != nil {
    log.Fatal(err)
}
// stmt.Close() runs here!

If you see in defer stmt.Close() it mentions, it's dangerous and yet not commented out for users to remove it.

Though I see no issue in above code as "defer" is going to run the code at the end but do they mean, above code is wrong and it should be replaced with below code or other better alternatives code.

tx, err := db.Begin()
if err != nil {
    log.Fatal(err)
}
defer tx.Rollback()
stmt, err := tx.Prepare("INSERT INTO foo VALUES (?)")
if err != nil {
    log.Fatal(err)
}
// Commented out below line.
// defer stmt.Close()
for i := 0; i < 10; i++ {
    _, err = stmt.Exec(i)
    if err != nil {
        log.Fatal(err)
    }
}
err = tx.Commit()
if err != nil {
    log.Fatal(err)
}
// Comment removed from below line to close the stmt
stmt.Close()

I see no difference in both of the code above, yet, I need expert advice on above if there is any difference or If I am missing something.

John Cargo
  • 1,839
  • 2
  • 29
  • 59
  • Could be that Close returns an error that was being ignored. Your version has the same issue. You could wrap the close in a func that does something with the error in order to more safely use the defer, or return the error in your second version and check for tx completeness. – saarrrr Sep 28 '17 at 18:57
  • 1
    The comment is saying the defer was dangerous in Go 1.4, but that was fixed a long time ago. The use of log.Fatal is unfortunate too. I'm not sure I'd use this as an example, instead you could look at the sql pkg examples, though those don't cover prepared statements well https://golang.org/pkg/database/sql/#pkg-examples – Kenny Grant Sep 28 '17 at 19:10
  • yes, I am not going to use `fatal` either. I will be doing something like https://stackoverflow.com/a/46476451/2819754 – John Cargo Sep 28 '17 at 19:35

1 Answers1

3

a defer statement is a good way to make sure something runs no matter how you exit the function.

In this particular case, it seems to not matter, since all the error handlers use log.Fatal. If you replace the log.Fatals with return statements, and remove the defers, you now have to cleanup in many places:

tx, err := db.Begin()
if err != nil {
    return nil,err
}
stmt, err := tx.Prepare("INSERT INTO foo VALUES (?)")
if err != nil {
    tx.Rollback()
    return nil,err
}
defer 
for i := 0; i < 10; i++ {
    _, err = stmt.Exec(i)
    if err != nil {
        tx.Rollback()
        return nil,err
    }
}
err = tx.Commit()
if err != nil {
    stmt.Close()
    tx.Rollback()
    return nil,err
}
stmt.Close()
return someValue, nil

If you use defer, it is harder to forget one place you need to clean something up.

captncraig
  • 22,118
  • 17
  • 108
  • 151