Paths

Table of Contentst

[native/sqlite] check for error while updating db version
AbandonedPublic
Actions

Authored by kamil on Oct 4 2022, 2:30 AM.

Details

Reviewers

tomek
• jon
atul
marcin

Summary

More context in this linear task with bug description.

This diff checks if updating version was performed successfully.

Note: I am a little bit worried about how we handle errors on migration fails, right now if an error occurs code logs error info, stops the migration process, and keeps working. I am afraid it's a little bit unsafe, app still might crash later on because there is something missing in DB and maybe unexpected behaviors like those discovered by Max may happen. As an alternative we can throw eg. std::runtime_error and reload app which I think will start failed migration again but I am not sure why a decision like this was made so I will appreciate any thoughts on this.

Test Plan

This reproduce plan
Mess with PRAGMA statement to force it to throw an error and check if was correctly handled.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Oct 4 2022, 2:30 AM

kamil held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 4 2022, 2:31 AM

Herald added subscribers: • abosh, atul, ashoat. · View Herald Transcript

Harbormaster completed remote builds in B12595: Diff 17317.Oct 4 2022, 2:40 AM

kamil edited the summary of this revision. (Show Details)Oct 4 2022, 2:58 AM

kamil edited the test plan for this revision. (Show Details)

kamil added reviewers: atul, marcin.

kamil added a subscriber: • max.

kamil published this revision for review.Oct 4 2022, 3:01 AM

tomek requested changes to this revision.Oct 4 2022, 4:43 AM

tomek added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
640–642 ↗	(On Diff #17317)	Shouldn't we rollback also here?
652 ↗	(On Diff #17317)	What if `shouldBeInTransaction` is false? What does calling `ROLLBACK` without `BEGIN TRANSACTION` do?

This revision now requires changes to proceed.Oct 4 2022, 4:43 AM

kamil added inline comments.Oct 4 2022, 5:05 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
640–642 ↗	(On Diff #17317)	Well, that's the question I also want to hear the answer to. As I wrote in summary we'll probably need better error handling in this case but I am not fully sure what it should look like. Another issue with this is, shouldn't we add `END TRANSACTION;`here?
652 ↗	(On Diff #17317)	I think it only fails with generating an error and because we don't check for errors there no harm is caused by this but I will add a check for this and call `ROLLBACK` only inside transaction.

kamil mentioned this in D5292: [native/sqlite] set `journal_mode` on fresh install.Oct 4 2022, 8:38 AM

update ROLLBACK

ashoat added inline comments.Oct 4 2022, 8:58 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
641	What's the relationship between `ROLLBACK` and `END TRANSACTION`? Are they meant to be used together, or separately? Does every `BEGIN TRANSACTION` need to end with a `ROLLBACK` or `END TRANSACTION`?

Harbormaster completed remote builds in B12601: Diff 17324.Oct 4 2022, 9:00 AM

tomek requested changes to this revision.Oct 5 2022, 1:51 AM

tomek added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
641		Answering these questions is crucial to fix this logic correctly. @kamil can you check that?
640–642 ↗	(On Diff #17317)	I think that any error during migrations should result in rollback and stopping the app - continuing to work with possibly corrupted db can cause more issues.

This revision now requires changes to proceed.Oct 5 2022, 1:51 AM

marcin accepted this revision.Oct 5 2022, 3:25 AM

marcin added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
641	I know it is not my differential but I did a quick search and found that: https://www.sqlite.org/lang_transaction.html `END TRANSACTION` is an alias for `COMMIT`. Therefore we should not add `END TRANSACTION` if we add `ROLLBACK` previously. `COMMIT` and `ROLLBACK` are mutually exclusive.
655	We frequently mention that avoiding indenation is important in our codebase. My idea to reduce indenation here is like this: if(error && shouldBeInTransaction){ sqlite3_exec(...); } if(error){ std::ostringstream errorStream; errorStream <<...<<...; Logger:log(...); sqlite3_free(error); break; }

kamil added inline comments.Oct 5 2022, 6:51 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
641	What's the relationship between ROLLBACK and END TRANSACTION? Are they meant to be used together, or separately? The answer is exactly what @marcin wrote. Does every BEGIN TRANSACTION need to end with a ROLLBACK or END TRANSACTION? It should. After calling another `BEGIN TRANSACTION` without rollbacking or committing previous the command will fail with an error because we don't use nested transactions (eg. `SAVEPOINT`). We don't check for those errors so, in theory, any further operations are within this transaction until `ROLLBACK` or `COMMIT`/`END TRANSACTION`. There is one more thing, on some errors SQLite may automatically rollback transaction. docs

update code structure

Harbormaster completed remote builds in B12644: Diff 17374.Oct 6 2022, 12:42 AM

It is a big improvement but I'm still worried about the behavior after failed migration. What happens with the app when a migration fails and is rolled back?

This revision is now accepted and ready to land.Oct 6 2022, 1:37 AM

I have a similar perspective to my comment in D5292 here... I think we should land this now, but I also think we still have an incomplete understanding, and should follow-up here to understand the code more, and so we can resolve ENG-1910

This was re-implemented in D5377

Revision Contents
Changeset List

Path

Size

native/

cpp/

CommonCpp/

DatabaseManagers/

SQLiteQueryExecutor.cpp

20 lines

Diff 17324

View Options

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp

[native/sqlite] check for error while updating db versionAbandonedPublicActions