Repository navigation
Break actor on errors that rollback transactions - #3728
Conversation
|
@kentonv For some reason, I don't have permissions to edit the reviewers list to add you explicitly, would be great if you could review this PR |
shrima-cf
changed the title
3bdfa2d to
bec9a1e
Compare
| code == SQLITE_NOMEM || code == SQLITE_INTERRUPT) { | ||
|
|
||
| sqlite3* db = &KJ_ASSERT_NONNULL(maybeDb, "previous reset() failed"); | ||
| // The transaction was rolledback, re-enabling the auto commit mode, so we should fail |
There was a problem hiding this comment.
Note that we might not have been in a transaction to start with, in which case there's no need to treat this as a critical error.
You can check if we were in a transaction before the error by checking inTransaction || !savepoints.empty().
We should probably then do inTransaction = false; savepoints.clear(); here since we need these to track what sqlite thinks the state is... although this feels precarious. I kind of wonder whether SqliteDatabase itself should refuse to execute any more statements after a critical error... not sure.
There was a problem hiding this comment.
I had the check for inTransaction || !savepoints.empty() , but got rid of it as it felt checking if the db is in auto_commit mode was sufficient.
I was also not able to get into the code path of if(inTransaction || !savepoints.empty()) through my test - any idea why this could be the case with the test I have?
There was a problem hiding this comment.
inTransaction || !savepoints.empty() determines if we were in a transaction before the error.
sqlite3_get_autocommit() tells us if we are in a transaction now.
This is how we tell if the critical error caused a rollback -- we have to compare these two things.
If we were not in a transaction before, then we don't need to consider the error critical, because it only rolled back the immediate statement that caused it, which is what we expect from any error. The critical problem is if past statements were rolled back since then the database state may no longer be consistent with the expectations of the application.
There was a problem hiding this comment.
I fixed the check here - 9b726ce
But two tests are failing:
KJ_TEST("SQLite critical error handling for SQLITE_BUSY")
KJ_TEST("SQLite critical error handling for SQLITE_NOMEM")
Both are failing because the commit is not auto-rolledback when run inside a transaction. Appreciate help to debug this
b8461b8 to
2a8ca60
Compare
|
I am going to merge this into my personal branch in cf repo (shrima/STOR-3841) and will open a new PR from there. Will keep this PR open as well since the comments need to be present. I had to do this update since something changed in the the main cf repo's build generation logic, that I am not able to reproduce here. There is no way to sync those two repos without discarding these commits either. |
|
Pull request moved - #3832 |
We should break output gates and reset actors in the event that we catch the following exceptions because they implicitly roll back any transaction:
SQLITE_FULL: database or disk full
SQLITE_IOERR: disk I/O error
SQLITE_BUSY: database in use by another process
SQLITE_NOMEM: out of memory
https://www.sqlite.org/lang_transaction.html#response_to_errors_within_a_transaction