Page MenuHomePhabricator

[CommCoreModule] Remove `last[NavigatedTo/Pruned]` from `MessageStoreThread` and `message_store_threads` table
ClosedPublic

Authored by atul on Oct 17 2023, 1:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 31, 12:48 AM
Unknown Object (File)
Tue, Dec 31, 12:48 AM
Unknown Object (File)
Tue, Dec 31, 12:48 AM
Unknown Object (File)
Tue, Dec 31, 12:43 AM
Unknown Object (File)
Tue, Dec 24, 2:14 AM
Unknown Object (File)
Dec 6 2024, 12:22 PM
Unknown Object (File)
Nov 30 2024, 2:52 AM
Unknown Object (File)
Nov 29 2024, 11:24 PM
Subscribers

Details

Summary
IMPORTANT: While I tested this migration a few times and read through things as carefully as I could, making changes to C++ codebase and SQLite store can be risky. Adding @ashoat and @kamil as blocking to take a look... would appreciate careful review to ensure everything is correct.

In this diff we:

  1. Remove lastPruned/lastNavigatedTo from MessageStoreThread struct and remove usages on C++ side.
  2. Introduce SQLite migration that drops lastPruned and lastNavigatedTo columns.

As of D9475, we've started ignoring what's persisted here in SQLite entirely... so this is effectively just dead code that we're cleaning up.


Depends on

D9475

Test Plan
  1. C++ codebase continues to build/run without issue.
  2. recreate_message_store_threads_table migration succeeded and new message_store_threads table is as expected:

c45df9.png (402×1 px, 81 KB)

Before:

7f7378.png (1×1 px, 276 KB)

After:

86e4f2.png (1×1 px, 262 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 17 2023, 1:59 PM
Harbormaster failed remote builds in B23311: Diff 32106!
atul requested review of this revision.Oct 17 2023, 1:59 PM
atul added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
399 ↗(On Diff #32106)

IMO The migrations at the top of SQLiteQueryExecutor are super long and take up a lot of the file. Would be good to move these to a separate migrations file.. or even a migrations folder where each migration gets its own file.

atul edited the summary of this revision. (Show Details)
atul edited the test plan for this revision. (Show Details)

include updated comm_query_executor.wasm

thanks @kamil for adding Emscripten job to CI that caught this

I don't really know c++ so I won't be much use here, but feel free to re-add me if you think otherwise at some point

This looks right to me

This revision is now accepted and ready to land.Oct 19 2023, 2:38 AM