go-goracle / goracle

Go database/sql driver for connecting to Oracle Database, using the ODPI-C library
273 stars 43 forks source link

Support for "For Update" queries with autocommit on non-tx connections #163

Closed rvegas closed 5 years ago

rvegas commented 5 years ago

We are facing a weird condition when working with queries that use "FOR UPDATE".

Normally, a query like this would block the row until a commit or a rollback is issued to the db:

select * from data.accounts WHERE ID = 76414 FOR UPDATE;

When using sqlplus or similar tools, their default behaviour is to set AUTOCOMMIT to false, so upon closing the client the transaction opened by "FOR UPDATE" is commited or rolled back depending on the status.

I've found that goracle used to support something similar:
https://github.com/go-goracle/goracle/blob/9d7b6d9aec7223fa4eb9ad49a5f20f87be59eb68/driver.go#L290

But now I cannot find it in the documentation or code.

Due to our design we would need to either have this feature somehow or to obtain the transacion object/id returned from a "FOR UPDATE" query on Oracle.

Is there a possibility to solve this with the current status of the library?

rvegas commented 5 years ago

Or even more, is the Db object supposed to acquire transacion locks? The Go mysql driver does not, look here:
https://stackoverflow.com/questions/39802596/mysql-safe-read-update-write

ghost commented 5 years ago

Use https://golang.org/pkg/database/sql/#DB.Conn you can release the database session with the Close() method

tgulacsi commented 5 years ago

FOR UPDATE is a strange thing: it's a Query, but also locks the DB. database/sql.DB is a connection pool, so every Query and Exec may get a different connection. That's why goracle defaults to autocommit.

Use a Tx (transaction) to gran a connection and COMMIT when you wish.

rvegas commented 5 years ago

I'm using the goracle as a database gateway service, parsing each query to look for "FOR UPDATE" and maybe others will greatly decrease performance, and I don't have to do it with other DB drivers afaik.
Any other workaround like using a Tx are great but they also force me to parse the query to forbid certain words.

Also regarding @wwanderley comment on using the Connect and Close, it really should not matter, if I run a query with "execSqlQuery" and I'm iterating over all rows with .Next() I'm guaranteed a pool DB connection release.

Per the standard I was understanding that only running "beginTx" I'd block a connection, but "FOR UPDATE" queries and any other run under no Tx should never hang on to a connection IMO.

I'll keep looking for the code to see if I can find a potential change necessary to the driver.

tgulacsi commented 5 years ago

I'm sorry @rvegas , it seems that I don't understand your use case. A rows,err := db.Query("SELECT * FROM table FOR UPDATE") does not commit till you hold the rows and it's not exhausted. A db.QueryRow releases (thus autocommits) the statement right before it returns the result.

But you can use this locked rows only in the same connection as where your Query ran and Rows is held. That's why you need a Conn and stick with it for the Query.

Neither I do understand why you need to parse for "FOR UPDATE" - how do you know which statements should be in a transaction, and when to COMMIT/ROLLBACK?

rvegas commented 5 years ago

My concern is that ANY query that is run through db.Query and/or db.QueryRow should release the connection either by calling db.Close or rows.Next() -> to the last row.

As per the comments I was understanding that "FOR UPDATE" is a special case where this is not controlled and thus I'd have to avoid using db.Query and use tx.begin + tx.query + tx.commit instead. But the problem is that I can't by design, since I provide an API for services to connect to a Database, if a external service issues a query with a "FOR UPDATE" query without opening a Tx prior I have no way to offer them a method to release the rows later on.

I understand that it's extremely specific and hard to explain, but in general, I'm getting the sense that these two mechanisms are different connection-wise:

tgulacsi commented 5 years ago

If your only need is to release the locked rows, then rows.Close() should be enough. If you want to allow the client to db.Exec("UPDATE my locked rows"), then it must be in the same Conn as Query was on, and Rows is held.

rvegas commented 5 years ago

I'm working on an example step by step, but rows.Close() is not releasing the locked rows.

tgulacsi commented 5 years ago

Hmmm... smt := Prepare; rows := stmt.Query; rows.Close(); stmt.Close() ?

rvegas commented 5 years ago

I'm not using Prepare and/or Statement I'll try those out. What's not working for me (rows not released in the Database) is:

rows = db.Query; rows.Next(); defer rows.Close()

rvegas commented 5 years ago

Indeed, testing all the examples the rows remain locked in the database after calling close on all possible objects, even this:
smt := Prepare; rows := stmt.Query; rows.Close(); stmt.Close()

This means that goracle has no way to ensure that certain queries outside of Transactions will or will not lock rows.

tgulacsi commented 5 years ago

I've searched thorugh ODPI-C 's documentation (https://oracle.github.io/odpi/doc/), but haven't found anything that does anyting with FOR UPDATE statements.

But then it hit me: only ExecContext has C.DPI_MODE_EXEC_COMMIT_ON_SUCCESS execution mode, QueryContext does not! You can try adding that to QueryContext - if that works, we can find out how to do it - always if not in transaction (as in ExecContext), or with a goracle.Option...

rvegas commented 5 years ago

Oh that's interesting, let me see what I can do with this info, thanks.

rvegas commented 5 years ago

Tested a fix and works like a charm.

I've opened a PR, I hope you agree to support this.

tgulacsi commented 5 years ago

v2.16.2 includes this fix. Thanks for your patience!

rvegas commented 5 years ago

Thanks to you for the insight @tgulacsi