database/sql: context cancellation allows statements to execute after rollback [1.14 backport] #39101
Comments
gopherbot
added
the
CherryPickCandidate
|
Approved |
andybons
added
the
CherryPickApproved
gopherbot
removed
the
CherryPickCandidate
|
@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? |
|
@leighmcculloch up to you. If the backport CL applies cleanly or otherwise without too much fuss then go for it. |
|
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. |
|
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. |
|
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. |
|
Has this change been released in 1.14 |
|
Will this make it into the announced release of 1.14.5 on July 14, 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 |
|
@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. |
|
Change https://golang.org/cl/242101 mentions this issue: |
|
Change https://golang.org/cl/242102 mentions this issue: |
|
Hey folks, sorry for the delay. It was quite a thorny cherry pick because: However, due to the attention this has gotten I was able to sit down and manually |
|
@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. |
|
Yes, indeed that’ll work if you get the lone composite CL
https://go-review.googlesource.com/c/go/+/242102
This issue was approved for backporting to Go1.14.5 (has been for quite a
while) but we’ll see whether the release team accepts it or not. If it is
drawn for the release of Go1.14.5, you’ll be in luck to use it ASAP without
manually building Go yourself, or might just have to wait until Go1.14.6 is
released.
…On Sun, Jul 12, 2020 at 5:28 AM Roman Dvoskin ***@***.***> wrote:
@odeke-em <https://github.com/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?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39101 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFL3V47E6LDEJK77KWAYQTR3GT75ANCNFSM4NBYLTSQ>
.
|
|
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. |
|
@andybons I've reviewed the 1.13 backport CL with a +2. Good to go from my end. |
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 <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Daniel Theophanes <[email protected]>
|
Closed by merging 399ce80 to release-branch.go1.14. |
|
Thank you very much for the quick review, @kardianos. |
@leighmcculloch requested issue #34775 to be considered for backport to the next 1.14 minor release.
The text was updated successfully, but these errors were encountered: