[native] Blocklist for backed up tables
ClosedPublic
Actions

Authored by michal on Mar 29 2024, 6:47 AM.

Details

Reviewers

kamil
marcin

Commits

rCOMMeb75d36995ab: [native] Blocklist for backed up tables

Summary

ENG-6918 : Improve backup handling of new tables

Currently tracking if a table is contained in backup happens in two places. Compactions use a SQL query that iterates over tables and runs DELETE FROM on backup file. Logs use a vector of table list to watch for. This diff unifies them and has a single blocklist, which informs both compaction and log creation.

Test Plan

Created a draft, created a compaction, created another draft (this one is only captured in logs)
Re-logged, restored backup, made sure both drafts are there
Added "drafts" table to backedUpTableBlocklist and repeated the test. This time neither of the logs existed after restore

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Mar 29 2024, 6:47 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMar 29 2024, 6:47 AM

michal added inline comments.Mar 29 2024, 6:51 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
41–48 ↗	(On Diff #38539)	There are a few differences: `integrity_store` is blocked, as we don't need it for backup `persist_storage` is blocked, all O(1) data in redux persist that we will need, will be stored in `synced_metadata` Also this doesn't block `communities` table which means that it will now be included in logs gathering (so it was a bug because this table was included in a compaction but not in logs).
1116 ↗	(On Diff #38539)	Wasn't sure where to put this, but at this point we have run the migrations so we should have a static db schema (for the lifetime of `SQLiteQueryExecutor`).

Harbormaster completed remote builds in B27819: Diff 38539.Mar 29 2024, 7:03 AM

michal requested review of this revision.Mar 29 2024, 7:03 AM

michal added a child revision: D11473: [sqlite] Add keyservers_synced_data table.Mar 29 2024, 7:22 AM

ashoat added subscribers: will, ginsu.Mar 29 2024, 12:05 PM

ashoat added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
41–48 ↗	(On Diff #38539)	cc @will @ginsu

will added inline comments.Mar 29 2024, 12:15 PM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
41–48 ↗	(On Diff #38539)	Could you explain what you mean by compaction? Currently working on sqlite stores for AuxUser (will store farcaster ids of mutuals) and ThreadActivity. Do either of these need to go on the blocklist? Possibly thread activity? https://linear.app/comm/issue/ENG-5310/move-threadactivitystore-to-sqlite https://linear.app/comm/issue/ENG-7405/auxiliary-user-store-follow-ups

LGTM for me but please stick to the convention to use SQLiteStatementWrapper.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1090 ↗	(On Diff #38539)	The convention is not to use `sqlite3_stmt` directly but to use `SQLiteStatementWrapper` class that handles freeing memory and error handling for you.

This revision now requires changes to proceed.Tue, Apr 2, 3:03 AM

kamil added inline comments.Tue, Apr 2, 3:07 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1082 ↗	(On Diff #38539)	could you put this below and group it with other functions hidden behind the `ifdef` to avoid adding more macros to this file?
1090 ↗	(On Diff #38539)	agree with that
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h
32 ↗	(On Diff #38539)	isn't this more readable now?

Move function, rename, use SQLiteStatementWrapper

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
41–48 ↗	(On Diff #38539)	Backup consists of two parts: Compaction which is a SQLite dump Backup logs, which we capture when there are new changes to the sqlite db (so we don't have to upload a large compaction everytime and only smaller deltas) I agree, thread activity should be shouldn't be backed up.