[native/sqlite] add missing indexes to `sqlite_orm` structure
ClosedPublic
Actions

Authored by kamil on Sep 15 2022, 7:41 AM.

Details

Reviewers

tomek
atul
marcin
ashoat
• jon

Commits

rCOMMd44d14f89a99: [native/sqlite] add missing indexes to `sqlite_orm` structure

Summary

Add missing indexes to sqlite_orm storage which are created for messages table and media table.

Make_index() usage example: https://github.com/fnc12/sqlite_orm/blob/master/examples/index.cpp#L18

Test Plan

Use decoded SQLite (e.g. comment encryption code and re-install app)
Instead of running migrations run SQLiteQueryExecutor::getStorage().sync_schema(); which will generate database.
Connect do database using sqlite3 and call PRAGMA index_list(messages); and PRAGMA index_list(media);;
Check if indexes match previous one, created by migrations.

Diff Detail

Repository

rCOMM Comm

Branch

db-migrations

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Sep 15 2022, 7:41 AM

kamil held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptSep 15 2022, 7:41 AM

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

Harbormaster failed remote builds in B12172: Diff 16700!Sep 15 2022, 7:41 AM

Harbormaster completed remote builds in B12172: Diff 16700.Sep 15 2022, 7:51 AM

kamil edited the summary of this revision. (Show Details)Sep 15 2022, 8:42 AM

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

kamil added reviewers: atul, • karol.

kamil edited the summary of this revision. (Show Details)

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

kamil published this revision for review.Sep 15 2022, 8:46 AM

atul added a reviewer: marcin.Sep 15 2022, 8:47 AM

Not familiar enough to give a good review

kamil added a parent revision: D5148: [native/sqlite] add missing unique and primary_key constraints to `sqlite_orm` structure.Sep 16 2022, 12:30 AM

kamil added a child revision: D5158: [native/sqlite] infer db schema from `sqlite_orm`.Sep 16 2022, 1:53 AM

Same question as in D5148

In D5150#151035, @ashoat wrote:

Same question as in D5148

It the same as in constraint case, I responded here: https://phab.comm.dev/D5148#152210

ashoat accepted this revision.Sep 20 2022, 6:10 AM

ashoat removed a reviewer: • karol. ashoat added 2 blocking reviewer(s): atul, marcin.

Looks good to me

Based on your test plan it looks like we can include these make_index arguments to make_storage(...) before the make_table ones, but wonder if it'd make more sense to sequence them after?

In D5150#152525, @atul wrote:

Looks good to me

Based on your test plan it looks like we can include these make_index arguments to make_storage(...) before the make_table ones, but wonder if it'd make more sense to sequence them after?

Yeah, I agree it will make more sense but I decided to sequence like this place due to two reasons:

This comment - not sure it is relevant to our case but I thought it'll be safer.
I've searched through library examples and everywhere make_index is before make_table calls.

@marcin please give it a review and add me as a blocking reviewer when accepted

In D5150#152541, @kamil wrote:

In D5150#152525, @atul wrote:

Looks good to me

Based on your test plan it looks like we can include these make_index arguments to make_storage(...) before the make_table ones, but wonder if it'd make more sense to sequence them after?

Yeah, I agree it will make more sense but I decided to sequence like this place due to two reasons:

This comment - not sure it is relevant to our case but I thought it'll be safer.

Could you make a test to build and run the app with indexes being created after tables and share the results here? Regardless of the result I would put a short comment above those lines that explain the order here since it might be confusing for future developers.

This revision is now accepted and ready to land.Sep 26 2022, 6:42 AM

add comment

In D5150#153828, @marcin wrote:

Could you make a test to build and run the app with indexes being created after tables and share the results here?

There it is:

ibc++abi: terminating with uncaught exception of type std::__1::system_error: no such table: main.media: SQL logic error
dyld4 config: DYLD_ROOT_PATH=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot DYLD_LIBRARY_PATH=/Users/kamil/Library/Developer/Xcode/DerivedData/Comm-ezpickwrqqgmkohgomffmhsjkkgj/Build/Products/Debug-iphonesimulator:/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/usr/lib/system/introspection DYLD_INSERT_LIBRARIES=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/usr/lib/libBacktraceRecording.dylib:/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/usr/lib/libMainThreadChecker.dylib:/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/Developer/Library/PrivateFrameworks/DTDDISupport.framework/libViewDebuggerSupport.dylib DYLD_FRAMEWORK_PATH=/Users/kamil/Library/Developer/Xcode/DerivedData/Comm-ezpickwrqqgmkohgomffmhsjkkgj/Build/Products/Debug-iphonesimulator
terminating with uncaught exception of type std::__1::system_error: no such table: main.media: SQL logic error
CoreSimulator 802.6.1 - Device: iPhone 13 mini (B47851C4-C23A-4939-B376-23E63FC33D3C) - Runtime: iOS 15.5 (19F70) - DeviceType: iPhone 13 mini

It fails because it tries to create an index on non-existing table.