Page MenuHomePhabricator

Implement SQLiteQueryExecutor method to create main compaction
ClosedPublic

Authored by marcin on Jan 2 2024, 4:29 AM.
Tags
None
Referenced Files
F3393381: D10503.diff
Sat, Nov 30, 1:34 PM
Unknown Object (File)
Fri, Nov 29, 1:31 PM
Unknown Object (File)
Wed, Nov 13, 7:08 AM
Unknown Object (File)
Wed, Nov 13, 7:08 AM
Unknown Object (File)
Wed, Nov 13, 7:07 AM
Unknown Object (File)
Wed, Nov 13, 7:07 AM
Unknown Object (File)
Wed, Nov 13, 7:07 AM
Unknown Object (File)
Wed, Nov 13, 7:07 AM
Subscribers

Details

Summary

This differential introduces SQLiteQueryExecutor method to create main compaction backup. It uses sqlite backup API via sqlite orm in order to be able to detect if a
developer makes mistake and tries to call backup creation inside of a transaction. Additionally this differential introduces changes that allow us to create second sqlite orm
storage object. It is necessary to be able to create backup that is encrypted with encryption key.

Test Plan
  1. Add a call to main compaction creation in SQLiteQueryExecutor::updateDraft and remove a call to replace draft.
  2. Update draft.
  3. Download application container from XCode or examine device file expolorer in Android studio.
  4. Ensure that backup file was created and it is encrypted.
  5. Repeat steps above with encryption disabled. Ensure database content matches backup content.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Rebase to update commit message

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1439 ↗(On Diff #35129)

In fact sqlite orm allows to backup a database to a file apth without creating additional storage object. But in such a case we are unable to set encryption key on the database connection that the orm will use to write to backup path. Introducing getEncryptedStorageAtPath was necessary to be able to use orm and encrypt backup file.

1444 ↗(On Diff #35129)

The call to sqlite3_backup_step attempts to acquire a transaction write lock on the destination database and keeps it until it returns SQLITE_DONE. One of the two error codes that we check against above will be returned if a programmer starts a transaction, performs some queries and then tries to create backup before committing transaction. In such a case we can't acquire lock and have to fail backup operation (This is the reason I explicitly tell in the test plan to remove a call to replace<Draft> in updateDraft if we want to call createMainCompaction there since it will fail due to active transaction (SQLiteQueryExecutor::updateDraft is always called in a transaction. Look at BaseDataStore.h)). One of those two errors is ` programmer mistake so we have to treat it separately from other errors that might occur here.

marcin requested review of this revision.Jan 2 2024, 6:13 AM

Can you remind me why we need to rekey the backup, as opposed to backing up the whole SQLCipher (encrypted) file alongside the current SQLCipher key, with just the SQLCipher key being encrypted with the backup key?

Can you remind me why we need to rekey the backup, as opposed to backing up the whole SQLCipher (encrypted) file alongside the current SQLCipher key, with just the SQLCipher key being encrypted with the backup key?

I am confused by this question a little bit. In this diff we are not rekeying database. We are performing an almost exact copy of the original SQLCipher encrypted database file. I said almost since the backup is further vacuummed to remove unused space making it as small as possible. When it comes to the encryption of encryption key with backup key it is outside the scope of this diff. @michal will encrypt it in Rust - his Rust code has access to Secure Store. Is there something I don't understand about your question?

Make compactionCreationMethod an abstract method of DatabaseQueryExecutor to match convention.

Can you remind me why we need to rekey the backup, as opposed to backing up the whole SQLCipher (encrypted) file alongside the current SQLCipher key, with just the SQLCipher key being encrypted with the backup key?

I am confused by this question a little bit. In this diff we are not rekeying database. We are performing an almost exact copy of the original SQLCipher encrypted database file. I said almost since the backup is further vacuummed to remove unused space making it as small as possible. When it comes to the encryption of encryption key with backup key it is outside the scope of this diff. @michal will encrypt it in Rust - his Rust code has access to Secure Store. Is there something I don't understand about your question?

Maybe I should've asked this question in D10504. I see there's some rekeying done there on restore (change_encryption_key), but I guess that's different from rekeying on backup. I'll ask there... sorry for the confusion!

kamil added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1426 ↗(On Diff #35137)

I would add also Logger::log but up to you

This revision is now accepted and ready to land.Jan 5 2024, 4:46 AM
  1. Add logging when encountering temporary files from previous backup attempts
  2. Rebase before landing

Rebase after landing parent diff

This revision was landed with ongoing or failed builds.Jan 17 2024, 3:06 AM
This revision was automatically updated to reflect the committed changes.