Page MenuHomePhabricator
Differential D4826

[native] Trace sqlite queries
ClosedPublic
Actions

Authored by • karol on Aug 12 2022, 2:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Unknown Object (File)
Fri, Nov 8, 8:54 PM
Unknown Object (File)
Tue, Nov 5, 2:06 AM
Unknown Object (File)
Oct 8 2024, 8:02 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
tomek

Details

Reviewers
tomek
ashoat
Commits
rCOMM12d76a161039: [native] Trace sqlite queries
Summary

Linear task: https://linear.app/comm/issue/ENG-1603/track-every-sqlite-query-on-native

We want to keep track of every SQLite query that modifies the database so we can send it as a log to the backup service. That will allow us to recreate the database using backups.

I used sqlite3_trace_v2 for this (ref: https://www.sqlite.org/c3ref/trace_v2.html)

I temporarily added a log so the queries are printed out. I suppose we do not want to do that but I just wanted to add it to the initial state of this diff (also for testing purposes). I will replace it with something like // TODO: send logs to backup here

Test Plan

Launch the mobile app, enter a chat and start typing, you're going to see the logs starting with sql query detected: ...

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• karol created this revision.Aug 12 2022, 2:59 AM
Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptAug 12 2022, 2:59 AM
• karol edited the summary of this revision. (Show Details)Aug 12 2022, 3:08 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: tomek, ashoat.
Harbormaster completed remote builds in B11341: Diff 15601.Aug 12 2022, 3:08 AM
• karol requested review of this revision.Aug 12 2022, 3:08 AM
• karol edited the summary of this revision. (Show Details)Aug 12 2022, 3:15 AM
• karol added inline comments.Aug 12 2022, 3:18 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
286 ↗(On Diff #15601)

Please, do not mind this line, this is only for testing. It is going to be replaced with

// TODO: send logs to backup here
tomek requested changes to this revision.Aug 12 2022, 5:16 AM
tomek added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
278 ↗(On Diff #15601)

What is the performance impact of this function?

281–282 ↗(On Diff #15601)

Could we use meaningful names? What p and s stand for?

360 ↗(On Diff #15601)

This name makes no sense - what callback is opened by this call. I guess the intention was to provide a callback that should be called when we open a new db. This name makes sense when setting on_open, but not here. So maybe just set_up? Or something else, but something which makes sense where it's called.

583–584 ↗(On Diff #15601)

What is the reason of this change?

This revision now requires changes to proceed.Aug 12 2022, 5:16 AM
• karol added inline comments.Aug 12 2022, 5:32 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
278 ↗(On Diff #15601)

I couldn't find anything about this :/ If I find anything, I'll post it.

281–282 ↗(On Diff #15601)

sure

For the record, p stands for "prepared statement", source: https://www.sqlite.org/c3ref/c_trace.html section SQLITE_TRACE_PROFILE

360 ↗(On Diff #15601)

I really do not mind naming. open_callback doesn't mean that the callback is being open. It means that this is the callback for the "open" operation. So when we call SQLite open, this should be triggered. For me, it makes sense. set_up? I'd say it's less descriptive, it doesn't say what we set up, and it does not indicate when this function should be triggered.

Maybe something like on_open? Maybe on_sqlite_open, on_database_open maybe even just on_open? What do you think? Basically, on_xxx seems like a good name for a callback.

583–584 ↗(On Diff #15601)

Yes, sorry this is a leftover and shouldn't be here.

595–597 ↗(On Diff #15601)

Shouldn't be here as well.

• karol updated this revision to Diff 15602.Aug 12 2022, 5:34 AM

address feedback

Harbormaster completed remote builds in B11342: Diff 15602.Aug 12 2022, 5:44 AM
tomek accepted this revision.Aug 12 2022, 5:46 AM
tomek added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
360 ↗(On Diff #15601)

Yeah, agree. Every on_xxx name you proposed sounds really good to me. I would probably go with on_database_open, but it's up to you.

This revision is now accepted and ready to land.Aug 12 2022, 5:46 AM
tomek added inline comments.Aug 12 2022, 5:47 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
278 ↗(On Diff #15601)

Ok. We can also do some performance testing.

• karol updated this revision to Diff 15603.Aug 12 2022, 6:17 AM

rename to on_database_open

Harbormaster completed remote builds in B11343: Diff 15603.Aug 12 2022, 6:28 AM
• karol added a comment.Aug 12 2022, 7:50 AM

I ran performance tests for this. Here's the code: https://github.com/karol-bisztyga/cpp-playground/tree/sqlite-performance-tests

Here are results:

  • with callback
inserts time: 8265
selects time: 6520
updates time: 10266
total time: 25055

inserts time: 8109
selects time: 6589
updates time: 10202
total time: 24904

inserts time: 7922
selects time: 6710
updates time: 10207
total time: 24848
  • without callback
inserts time: 7967
selects time: 6566
updates time: 10177
total time: 24713

inserts time: 7850
selects time: 6524
updates time: 10113
total time: 24492

inserts time: 8193
selects time: 6560
updates time: 10097
total time: 24855

I don't know, it seems that it's just a little tiny bit faster without it but not so much that I'd be worried about it. What do you think?

• karol updated this revision to Diff 15637.Aug 16 2022, 1:43 AM

remove redundant log

Harbormaster completed remote builds in B11372: Diff 15637.Aug 16 2022, 1:53 AM
Closed by commit rCOMM12d76a161039: [native] Trace sqlite queries (authored by • karol). · Explain WhyAug 16 2022, 2:03 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM12d76a161039: [native] Trace sqlite queries.

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
DatabaseManagers/
33 lines

Diff 15638

View Options

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp

Loading...