Skip to content
This repository has been archived by the owner on Nov 22, 2018. It is now read-only.

Add support for autocommit (was: "create unique error") #46

Open
lunny opened this issue Apr 16, 2014 · 11 comments
Open

Add support for autocommit (was: "create unique error") #46

lunny opened this issue Apr 16, 2014 · 11 comments
Assignees

Comments

@lunny
Copy link

lunny commented Apr 16, 2014

CREATE UNIQUE INDEX UQE_userinfo_username ON userinfo (username);

attempt to update the DB outside of a transaction

Is a single sql still need to add a transaction?

@cznic cznic closed this as completed in 1bd8bec Apr 16, 2014
@cznic
Copy link
Owner

cznic commented Apr 16, 2014

Yes. It's now documented.

@cznic cznic added the question label Apr 16, 2014
@lunny
Copy link
Author

lunny commented Apr 16, 2014

To a database/sql inteface, for write operations, I must to use tx := db.Begin() every time?

@cznic
Copy link
Owner

cznic commented Apr 16, 2014

Yes as of now. Is it different with respect to other database/sql drivers?

@lunny
Copy link
Author

lunny commented Apr 16, 2014

Yes. It's different to other database/sql drivers. And make my work difficult. Do you have plan that a single write operation could do with no need transaction declaration but in fact is atomic?

@cznic
Copy link
Owner

cznic commented Apr 16, 2014

I wonder how other DB drivers manage the classical transaction example:

db.Exec("UPDATE AccountA SET Balance = Balance - $1;", amount)
db.Exec("UPDATE AccountB SET Balance = Balance + $1;", amount)

If the two statements would each be run in an automatic transaction then funny things can happen - money can unexpectedly appear or disappear. Both of the statements are "single write operation", yet the semantics is that they both must fail or succeed, IOW there must be a transaction wrapping both of them at once. Two automatically provided transactions are separate and do not solve the above discussed problem.

Automatic transactions wrapping single write operations seem to me like not the way any data should be updated. Or am I missing something? Please let me know. Thanks.

@lunny
Copy link
Author

lunny commented Apr 16, 2014

I don't know what's your difficulty. For your example. I have to do

tx1 := db.Begin()
tx1.Exec("UPDATE AccountA SET Balance = Balance - $1;", amount)
tx1.Commit()

tx2 := db.Begin()
tx2.Exec("UPDATE AccountB SET Balance = Balance + $1;", amount)
tx2.Commit()

for other database's interface. I can just write code:

db.Exec("UPDATE AccountA SET Balance = Balance - $1;", amount)
db.Exec("UPDATE AccountB SET Balance = Balance + $1;", amount)

the usage is not friendly.

@cznic
Copy link
Owner

cznic commented Apr 16, 2014

That is wrong. This is not a safe way of transferring money from one account to the other account. The proper way is:

tx1 := db.Begin()
tx1.Exec("UPDATE AccountA SET Balance = Balance - $1;", amount)
tx1.Exec("UPDATE AccountB SET Balance = Balance + $1;", amount)
tx1.Commit()

The transfer is guaranteed to be correct (everything performed or nothing performed) only when both of the statements are wrapped in a single transaction. Providing automatic transactions for every single statement would effectively be the same as what you showed above: The transaction which must be a single one is incorrectly torn into two separated transactions. If for any reason the program doesn't finish both of the two separated transactions then the accounts can have incorrect balances.

If other drivers really insert a separate automatic transaction for every DB updating statement they perform then those drivers allow an incorrect usage pattern. Such pattern violates the "C" in ACID - Consistency.

This is not about the driver being friendly or me not being willing to make the use easier. This is about correctness. Correct data handling in the general case requires an transaction wrapping the whole process of changing one consistent DB state to another, new consistent DB state. In the general case that may require executing more than a single statement (as seen in the accounts example). Thus creating automatic transactions around every single statement is not correct for general use.

If there is a flaw in the presented logic, please let me know. Thanks.

PS: BTW, also, when there are many updates which together are a single transaction, those updates perform slower (or much slower) if performed in N transactions instead of a single transaction. Every transaction has a 2PC protocol cost (WAL + DB update).

@unknwon
Copy link

unknwon commented Apr 17, 2014

Developer should take responsibility for safety in simple query, I'm not a database guy but I think developer should know when to use transaction and when not.

@cznic
Copy link
Owner

cznic commented Apr 17, 2014

@unknwon
Agreed, and that is the current situation. No magic/implicit transactions - the developer should know and create a proper transaction context explicitly ("take the responsibility"). Especially when the database/sql model doesn't support nested transactions, making an implicit one would actually forbid the developer to use the proper (outer) transaction where necessary.

OTOH, wrt DRY:

func myLittleHelper(exec string, arg ...interface{}) (res sql.Result, err error) {
        tx, err := db.Begin()
        if err != nil {
                retrurn
        }

        if res, err = tx.Exec(exec, arg...); err != nil {
                return
        }

        err = tx.Commit()
        return
}

@cznic
Copy link
Owner

cznic commented Apr 17, 2014

After a discussion on G+: #Accepted.

@cznic cznic reopened this Apr 17, 2014
@cznic cznic self-assigned this Apr 17, 2014
@lunny
Copy link
Author

lunny commented Apr 17, 2014

I just changed a little. It's enough.

diff --git a/driver.go b/driver.go
index e58b5cf..9a1a221 100644
--- a/driver.go
+++ b/driver.go
@@ -298,7 +298,35 @@ func (c *driverConn) Exec(query string, args []driver.Value) (driver.Result, err
        return nil, err
    }

-   return driverExec(c.db, c.ctx, list, args)
+   //return driverExec(c.db, c.ctx, list, args)
+   return c.driverExec(c.db, list, args)
+}
+
+func (c *driverConn) driverExec(db *driverDB, list List, args []driver.Value) (driver.Result, error) {
+   internalCtx := (c.ctx == nil)
+
+   if internalCtx {
+       fmt.Println("internal begin")
+       _, err := c.Begin()
+       if err != nil {
+           return nil, err
+       }
+   }
+
+   result, err1 := driverExec(db, c.ctx, list, args)
+
+   if internalCtx {
+       if err1 != nil {
+           fmt.Println("internal rollback")
+           c.Rollback()
+       } else {
+           fmt.Println("internal commit")
+           c.Commit()
+       }
+       c.ctx = nil
+   }
+
+   return result, err1
 }

 func driverExec(db *driverDB, ctx *TCtx, list List, args []driver.Value) (driver.Result, error) {
@@ -513,7 +541,8 @@ func (s *driverStmt) NumInput() int {
 // Exec executes a query that doesn't return rows, such as an INSERT or UPDATE.
 func (s *driverStmt) Exec(args []driver.Value) (driver.Result, error) {
    c := s.conn
-   return driverExec(c.db, c.ctx, s.stmt, args)
+   //return driverExec(c.db, c.ctx, s.stmt, args)
+   return c.driverExec(c.db, s.stmt, args)
 }

 // Exec executes a query that may return rows, such as a SELECT.

@cznic cznic changed the title create unique error Add support for autocommit (was: "create unique error") May 7, 2014
@cznic cznic removed the question label May 7, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants