Skip to content

perf(mobile): TAM-7126: scope the sync foreign key check to tables that received rows - #11094

Closed
jaskfla wants to merge 6 commits into
tam-7126-2-drop-dependency-sortfrom
tam-7126-3-scope-fk-check
Closed

jaskfla wants to merge 6 commits into
tam-7126-2-drop-dependency-sortfrom
tam-7126-3-scope-fk-check

Conversation

@jaskfla

@jaskfla jaskfla commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Changes

Part 3 of 4 (TAM-7126), stacked on #11093.

The argument-less PRAGMA foreign_key_check rescans every row of every table against every foreign key. The existing comment argued it was cheaper than one pragma per model, which is true of round trips but not of work done: foreign_key_check("table") scans only that table. The save functions now return the set of tables that actually received rows, and the check is scoped to those (usually a handful rather than all ~53). Error output is unchanged.

Stack: #11092#11093this#11095.

Auto-Deploy

  • Deploy
Options
  • Artillery load test
  • Seed from closest snapshot
  • Generate fake data
  • More data (20Gi)
  • No facility servers (central-only)
  • No sync (facility tasks scaled to zero)
  • Skip mobile build
  • Always build mobile
  • Stay up for 8 hours
  • Stay up for 24 hours
  • Stay up (no TTL)
  • Build images only (don't deploy)
  • Build all images (amd64 + Windows; default is arm64 only)
  • Pause this deploy

Tests

  • Run E2E tests
  • Run DAST scan

Review Hero

  • Run Review Hero
  • Auto-fix review suggestions Wait for Review Hero to finish, resolve any comments you disagree with or want to fix manually, then check this to auto-fix the rest.
  • Auto-fix CI failures Check this to auto-fix lint errors, test failures, and other CI issues.
  • Auto-merge upstream Check this to merge the base branch into this PR, with AI conflict resolution if needed.
  • Save suppressions Check this to capture 👎 reactions on Review Hero comments as suppression rules in .github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.

Remember to...

  • ...write or update tests
  • ...add UI screenshots and testing notes to the Linear issue
  • ...add any manual upgrade steps to the Linear issue
  • ...update the config reference, settings reference, or any relevant runbook(s)
  • ...call out additions or changes to config files for the deployment team to take note of

Comment thread packages/mobile/App/services/sync/utils/checkForeignKeys.ts
@review-hero

review-hero Bot commented Sep 14, 2026

Copy link
Copy Markdown

🦸 Review Hero Summary (round 1)
7 agents reviewed this PR | 2 failed | 0 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters, 2 below threshold

Below consensus threshold (2 unique issues not confirmed by majority)
Location Agent Severity Comment
packages/mobile/App/services/sync/utils/checkForeignKeys.ts:24 Design & Architecture suggestion checkForeignKeys used to be a self-contained safety net: it scanned tables from the full incoming-sync model list, independent of how the save step behaved. Now its correctness depends entirely on ...
packages/mobile/App/services/sync/utils/checkForeignKeys.ts:29 Bugs & Correctness suggestion Scoping the check to tables that received rows this pull narrows detection to violations where the referencing (child) table is itself touched. PRAGMA foreign_key_check(table) validates the nam...
Local fix prompt (copy to your coding agent)
Fix these issues identified on the pull request. One commit per issue fixed.

-------

`packages/mobile/App/services/sync/utils/checkForeignKeys.ts:33`: checkForeignKeys now issues one sequential `PRAGMA foreign_key_check("table")` per touched table instead of a single argument-less PRAGMA. This is a net win for incremental syncs (few tables touched, previously always triggered a full-DB scan), but pullInitialSync (MobileSyncManager.ts:444) touches nearly every sync-enabled model — commonly 30+ tables. In that path this turns 1 round trip into 30+ sequential awaited queries with no reduction in rows actually scanned (since virtually every table is 'touched' anyway), adding per-call overhead to the heaviest, most performance-sensitive sync (first-time device provisioning). Consider falling back to the single argument-less PRAGMA when the touched-table count is close to the full model count, and only using the per-table loop when the touched set is a small subset.

@jaskfla
jaskfla force-pushed the tam-7126-3-scope-fk-check branch 2 times, most recently from fc382f7 to cc5d505 Compare September 14, 2026 03:47
@jaskfla
jaskfla force-pushed the tam-7126-3-scope-fk-check branch from cc5d505 to d6bcdbd Compare September 14, 2026 03:49
* that table, and the argument-less form rescans the whole database, so the cost is proportional
* to the tables checked rather than to the number of round trips.
*/
export const checkForeignKeys = async (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Performance] suggestion

This loop reintroduces the exact cost pattern the removed comment warned about ("far cheaper than issuing one PRAGMA per model (20-30+ awaits)"): checkForeignKeys now issues one PRAGMA foreign_key_check(table) round-trip per touched table instead of one for the whole database. The scoping only pays off when few tables are touched (typical incremental sync). But for the initial sync — the largest and most performance-sensitive pull path, in MobileSyncManager.pullInitialSync — touchedTables ends up covering nearly every synced model anyway, since the initial pull populates most tables. In that case this change replaces 1 round-trip with 20-30+ sequential awaited queries through the SQLite bridge, with no reduction in total rows scanned, just added per-call overhead. Consider keeping the single whole-DB PRAGMA + client-side filter for the initial-sync path and reserving the per-table loop for incremental syncs where the touched-table set is genuinely small.

@review-hero

review-hero Bot commented Sep 14, 2026

Copy link
Copy Markdown

🦸 Review Hero Summary (round 2)
8 agents reviewed this PR | 4 failed | 0 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters

Local fix prompt (copy to your coding agent)
Fix these issues identified on the pull request. One commit per issue fixed.

-------

`packages/mobile/App/services/sync/utils/checkForeignKeys.ts:24`: This loop reintroduces the exact cost pattern the removed comment warned about ("far cheaper than issuing one PRAGMA per model (20-30+ awaits)"): checkForeignKeys now issues one PRAGMA foreign_key_check(table) round-trip per touched table instead of one for the whole database. The scoping only pays off when few tables are touched (typical incremental sync). But for the initial sync — the largest and most performance-sensitive pull path, in MobileSyncManager.pullInitialSync — touchedTables ends up covering nearly every synced model anyway, since the initial pull populates most tables. In that case this change replaces 1 round-trip with 20-30+ sequential awaited queries through the SQLite bridge, with no reduction in total rows scanned, just added per-call overhead. Consider keeping the single whole-DB PRAGMA + client-side filter for the initial-sync path and reserving the per-table loop for incremental syncs where the touched-table set is genuinely small.

…nothing to pull

A zero-record incremental pull still created the snapshot table, opened the
(write-locking) save transaction and ran the dependency sort and foreign key
check inside it, every five minutes. Now it only advances the pull cursor.

The pull count is a Postgres bigint that arrives serialised as a string, so it
is coerced to a number where it enters the app; otherwise this gate (and the
existing one in pullRecordsInBatches) never matches.
@jaskfla
jaskfla force-pushed the tam-7126-3-scope-fk-check branch from d6bcdbd to e396315 Compare September 14, 2026 23:39
Foreign keys are deferred for the whole save transaction, so the order models
are saved in is irrelevant. The sort cost one PRAGMA foreign_key_list round
trip per model (~53) inside the transaction on every sync.
…at received rows

The argument-less PRAGMA foreign_key_check rescans every row of every table
against every foreign key, on every sync. The per-table form only scans that
table, so pass the tables that actually had rows saved (usually a handful,
not all ~53 pull-direction tables). Error output is unchanged.
@jaskfla
jaskfla force-pushed the tam-7126-3-scope-fk-check branch from e396315 to d15ae7e Compare September 14, 2026 23:44
@jaskfla jaskfla closed this Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant