Page MenuHomePhabricator
Differential D5291

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

Authored by kamil on Oct 4 2022, 2:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 3, 12:36 AM
Unknown Object (File)
Sun, Nov 3, 12:36 AM
Unknown Object (File)
Sun, Nov 3, 12:36 AM
Unknown Object (File)
Sun, Nov 3, 12:35 AM
Unknown Object (File)
Sun, Nov 3, 12:30 AM
Unknown Object (File)
Sep 26 2024, 10:16 PM
Unknown Object (File)
Sep 26 2024, 10:16 PM
Unknown Object (File)
Sep 26 2024, 10:16 PM
View All Files
Subscribers
• abosh
ashoat
atul
• max

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
  1. This reproduce plan
  2. Mess with PRAGMA statement to force it to throw an error and check if was correctly handled.

Diff Detail

Repository
rCOMM Comm
Branch
add-not-exists
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
kamil updated this revision to Diff 17324.Oct 4 2022, 8:49 AM

update ROLLBACK

ashoat added inline comments.Oct 4 2022, 8:58 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
641 ↗(On Diff #17324)

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 ↗(On Diff #17324)

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 ↗(On Diff #17324)

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 ↗(On Diff #17324)

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 ↗(On Diff #17324)

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

kamil updated this revision to Diff 17374.Oct 6 2022, 12:32 AM

update code structure

Harbormaster completed remote builds in B12644: Diff 17374.Oct 6 2022, 12:42 AM
tomek accepted this revision.Oct 6 2022, 1:37 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
ashoat added a comment.Oct 6 2022, 6:06 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

kamil abandoned this revision.Nov 15 2022, 5:57 AM

This was re-implemented in D5377

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
DatabaseManagers/
19 lines

Diff 17374

View Options

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp

Loading...