googleapis / go-sql-spanner

Google Cloud Spanner driver for Go's database/sql package.
Apache License 2.0
105 stars 25 forks source link

ExecQuery with a DML statement with THEN RETURN in AutoCommit=true mode uses read-only transaction #235

Open olavloite opened 6 months ago

olavloite commented 6 months ago

Calling ExecQuery (or similar) with a DML statement that contains a THEN RETURN clause while the connection is in auto-commit mode, causes the driver to try to execute the DML statement using a read-only transaction.

egonelbre commented 4 months ago

A simple fix could be to check whether the query string contains THEN RETURN, and if it does then execute it as a RW transaction. e.g.

var rxThenReturn = regexp.MustCompile(`(?i)\bTHEN[\s\r\n]+RETURN\b`)

// isProbablyReadWrite returns true when the query should use a ReadWrite transactions.
// It may return true in some corner cases, where it is a read-only query.
func isProbablyReadWrite(query string) (bool, error) {
       query, err := removeCommentsAndTrim(query)
       if err != nil {
               return false, err
       }

       return rxThenReturn.MatchString(query), nil
}

Executing a read-only statement in a read-write transaction shouldn't cause problems AFAIK.

olavloite commented 4 months ago

@egonelbre We'd rather not introduce more regex checking in this driver. I'm currently working on retrofitting some code from our Java ecosystem into this driver that should allow us to check for the existence of a THEN RETURN clause without the use of regexes. (It should also allow us to improve the current implementation for replacing positional parameters for named parameters by removing the step that first removes all comments from the SQL string).