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

database/sql: context cancellation allows statements to execute after rollback [1.14 backport] #39101

Closed
gopherbot opened this issue May 15, 2020 · 19 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@gopherbot
Copy link

@leighmcculloch requested issue #34775 to be considered for backport to the next 1.14 minor release.

@odeke-em I believe this fix satisfies the requirements for a backport according to the minor version release policy because the issue is a serious issue that is causing databases to get into bad state where no work around is possible and updating to the latest release Go 1.14.3 does not fix the issue.

If patching Go is the suggested work around that suggests there is no work around in an application. Unfortunately building custom Go isn't an option for everyone as our product is open source and so we'd need to ask everyone who uses it to build with a custom Go.

@gopherbot please consider this for backport because this issue breaks production applications resulting in unexpected database state and cannot be fixed with a work around including updating the version of Go to the latest version.

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label May 15, 2020
@gopherbot gopherbot added this to the Go1.14.4 milestone May 15, 2020
@andybons
Copy link
Member

Approved

@andybons andybons added the CherryPickApproved Used during the release process for point releases label May 27, 2020
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label May 27, 2020
@leighmcculloch
Copy link
Contributor

@andybons Thanks for approving the backport 🎉. I only need this fixed in Go 1.14 but given 1.13 is still a supported Go version should I also open a backport issue for this for Go 1.13?

@andybons
Copy link
Member

@leighmcculloch up to you. If the backport CL applies cleanly or otherwise without too much fuss then go for it.

@odeke-em
Copy link
Member

The CLs don't apply cleanly, there are some merge conflicts with Go1.14, but also the 3 CLs were mailed in a relation chain, so that's tricky too. @kardianos could you please help with composing the backport CLs? Thank you.

@kardianos
Copy link
Contributor

The only way to apply this would likely be to pull the full chain with it, or devise a new patch with the same effect. This fix relied on another fix prior to it I believe.

@kardianos
Copy link
Contributor

On a desktop now. If you do a backport, you should cherry pick both https://go-review.googlesource.com/c/go/+/216197 and https://go-review.googlesource.com/c/go/+/216240 . These won't apply cleanly because they were after the change that fixes the session resetter.

@dmitshur dmitshur modified the milestones: Go1.14.4, Go1.14.5 Jun 1, 2020
@fednerjuste
Copy link

Has this change been released in 1.14

@brbaker
Copy link

brbaker commented Jul 10, 2020

Will this make it into the announced release of 1.14.5 on July 14, 2020?

@crbraun
Copy link

crbraun commented Jul 10, 2020

Surprising to me that this critical issue has been around for so long and not fixed. To not be able to rely on SQL transaction ACID rules is very dangerous indeed. Am I to assume that developers are not using auto commit or not building mission critical applications that require SQL transactions to be atomic in nature?

I encourage the community to back port this fix as soon as possible. Thanks for listening.

Chris

@ianlancetaylor
Copy link
Contributor

@brbaker Nobody has done the backport yet. Also 1.14.5 is going to be a security release so this will definitely not be in that release. The question is whether somebody does the backport to get it into the 1.14.6 release.

@gopherbot
Copy link
Author

Change https://golang.org/cl/242101 mentions this issue: [release-branch.go1.14] database/sql: backport 3 Tx rollback related CLs

@gopherbot
Copy link
Author

Change https://golang.org/cl/242102 mentions this issue: [release-branch.go1.14] database/sql: backport 3 Tx rollback related CLs

@odeke-em
Copy link
Member

Hey folks, sorry for the delay. It was quite a thorny cherry pick because:
a) A relation chain in 3 CLs that had merge conflcits
b) I don't have the Gerrit "forge-author" permission set
thus the normal way of making cherry-picks was a pain.

However, due to the attention this has gotten I was able to sit down and manually
make those cherry picks, along with some git magic and I've mailed https://go-review.googlesource.com/c/go/+/242102
for Go1.14.X, so at least now we have a cherry pick up :)

@odeke-em odeke-em self-assigned this Jul 12, 2020
@romandvoskin
Copy link

romandvoskin commented Jul 12, 2020

@odeke-em Thank you! Does this mean that with the cherry pick, one can manually build a custom distribution by applying the cherry pick on the latest 1.14 release? I'm asking because it seems 1.14.5 is deemed a security release and will not include the SQL fixes and we can't wait for an official release to fix this critical bug in our organization.

@odeke-em
Copy link
Member

odeke-em commented Jul 12, 2020 via email

@andybons
Copy link
Member

We are waiting for the 1.13 backport CL to be reviewed before either can land in a point release. We are planning to cut tomorrow, and if it doesn’t make it in, then we can aim for the August 1 point release in two weeks.

@kardianos
Copy link
Contributor

@andybons I've reviewed the 1.13 backport CL with a +2. Good to go from my end.

gopherbot pushed a commit that referenced this issue Jul 16, 2020
Manually backported the subject CLs, because of lack of
Gerrit "forge-author" permissions, but also because the prior
cherry picks didn't apply cleanly, due to a tight relation chain.

The backport comprises of:
* CL 174122
* CL 216197
* CL 223963
* CL 216240
* CL 216241

Note:
Due to the restrictions that we cannot retroactively
introduce API changes to Go1.14.6 that weren't in Go1.14, the Conn.Validator
interface (from CL 174122, CL 223963) isn't exposed, and drivers will just be
inspected, for if they have an IsValid() bool method implemented.

For a description of the content of each CL:

* CL 174122:
database/sql: process all Session Resets synchronously

Adds a new interface, driver.ConnectionValidator, to allow
drivers to signal they should not be used again,
separatly from the session resetter interface.
This is done now that the session reset is done
after the connection is put into the connection pool.

Previous behavior attempted to run Session Resets
in a background worker. This implementation had two
problems: untested performance gains for additional
complexity, and failures when the pool size
exceeded the connection reset channel buffer size.

* CL 216197:
database/sql: check conn expiry when returning to pool, not when handing it out

With the original connection reuse strategy, it was possible that
when a new connection was requested, the pool would wait for an
an existing connection to return for re-use in a full connection
pool, and then it would check if the returned connection was expired.
If the returned connection expired while awaiting re-use, it would
return an error to the location requestiong the new connection.
The existing call sites requesting a new connection was often the last
attempt at returning a connection for a query. This would then
result in a failed query.

This change ensures that we perform the expiry check right
before a connection is inserted back in to the connection pool
for while requesting a new connection. If requesting a new connection
it will no longer fail due to the connection expiring.

* CL 216240:
database/sql: prevent Tx statement from committing after rollback

It was possible for a Tx that was aborted for rollback
asynchronously to execute a query after the rollback had completed
on the database, which often would auto commit the query outside
of the transaction.

By W-locking the tx.closemu prior to issuing the rollback
connection it ensures any Tx query either fails or finishes
on the Tx, and never after the Tx has rolled back.

* CL 216241:
database/sql: on Tx rollback, retain connection if driver can reset session

Previously the Tx would drop the connection after rolling back from
a context cancel. Now if the driver can reset the session,
keep the connection.

* CL 223963
database/sql: add test for Conn.Validator interface

This addresses comments made by Russ after
https://golang.org/cl/174122 was merged. It addes a test
for the connection validator and renames the interface to just
"Validator".

Updates #31480
Updates #32530
Updates #32942
Updates #34775
Fixes #39101

Change-Id: I043d2d724a367588689fd7d6f3cecb39abeb042c
Reviewed-on: https://go-review.googlesource.com/c/go/+/242102
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Theophanes <kardianos@gmail.com>
@gopherbot
Copy link
Author

Closed by merging 399ce80 to release-branch.go1.14.

@andybons
Copy link
Member

Thank you very much for the quick review, @kardianos.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests