Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
olavloite opened this issue May 13, 2024 · 2 comments
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@olavloite
Copy link
Collaborator

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.

@olavloite olavloite added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels May 13, 2024
@egonelbre
Copy link
Contributor

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
Copy link
Collaborator Author

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

2 participants