Conversation
yiminc
left a comment
yiminc
left a comment
There was a problem hiding this comment.
could you add a unit test to verify this fix?
|
Added a unit test which will fail if we don't have this fix. |
|
|
||
| const ( | ||
| goSqlDriverName = "sqlite" | ||
| goSqlDriverName = "sqlite_temporal" |
There was a problem hiding this comment.
Wondering why this change is required. What's the implication of changing this string?
There was a problem hiding this comment.
golang's sql library keeps a list of registered drivers adressed by the driver name. modernc.org/sqlite registers a driver using the name sqlite. So we have to register this new driver using a different name and use this name when opening connections.
There was a problem hiding this comment.
Got it, as long as this doesn't have other implications, this works for me.
You should also add a TODO to remove this once the sqlite driver fix is adopted.
| // ResetSession does nothing. | ||
| func (c *conn) ResetSession(ctx context.Context) error { | ||
| return nil | ||
| } | ||
|
|
||
| // IsValid always returns true. We only have one connection to the sqlite db. It should always be valid. Otherwise, we would have lost the database. | ||
| func (c *conn) IsValid() bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
I wonder what the implications are for always returning valid and ignoring ResetSession apart from preventing the connection from being closed on canceled context.
There was a problem hiding this comment.
If the connection is not valid for some reason, we must have lost the database already. and if the connection is not valid, there is nothing we can do to create a new connection. Because of this I think it is safe to return valid in all cases. We can let the next database operation fail if connection is not valid.
There was a problem hiding this comment.
Put this in a comment in the code please.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Approved with comments. Thanks!
|
|
||
| const ( | ||
| goSqlDriverName = "sqlite" | ||
| goSqlDriverName = "sqlite_temporal" |
There was a problem hiding this comment.
Got it, as long as this doesn't have other implications, this works for me.
You should also add a TODO to remove this once the sqlite driver fix is adopted.
| // ResetSession does nothing. | ||
| func (c *conn) ResetSession(ctx context.Context) error { | ||
| return nil | ||
| } | ||
|
|
||
| // IsValid always returns true. We only have one connection to the sqlite db. It should always be valid. Otherwise, we would have lost the database. | ||
| func (c *conn) IsValid() bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Put this in a comment in the code please.
|
Thank you! Added comments. |
## What changed? Use the latest release of modernc.org/sqlite library which has our fix for the connection cancellation issue: https://gitlab.com/cznic/sqlite/-/merge_requests/74. We had a temporary fix merged in #6815. Reverting that as well. ## Why? Sqlite library did not implement functions ResetSession() and IsValid() in the database connection. sql library was discarding a database connection when a context was canceled during a transaction. The new version of modernc.org/sqlite has these two methods implemented. ## How did you test it? Existing unit test to verify the connection is not lost. ## Potential risks None ## Documentation ## Is hotfix candidate? No
This reverts commit 1f3ebb5.
3 participants
What changed?
Use an sqlite connection that has ResetSession() and IsValid() implemented.
Why?
If these methods are not implemented, sql library will close the connection when the context is cancelled during a transaction. We will lose the database if this happens.
IsValid() will always return true in this case. We should always have a valid database connection, otherwise we would have already lost the database. Similarly ResetSession() does nothing.
How did you test it?
Tried to reproduce the issue by cancelling a transaction context.
Potential risks
None
Documentation
Is hotfix candidate?
No