Skip to content

Update moz_origins index column order to match desktop - #7521

Open
shawnz wants to merge 1 commit into
mozilla:mainfrom
shawnz:shawnz/origins-index-order
Open

Update moz_origins index column order to match desktop#7521
shawnz wants to merge 1 commit into
mozilla:mainfrom
shawnz:shawnz/origins-index-order

Conversation

@shawnz

@shawnz shawnz commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

This PR serves as a follow up to #7484 as well as a port of the fix in desktop for bug 2025999.

This doesn't add any new indexes, like #7484 did. Instead, it just reverses the column order of the existing (prefix, host) unique index on moz_origins with a (host, prefix) index instead. In addition to mitigating the bad query plan from bug 2056115, this change also has the advantage of improved performance due to putting the higher-cardinality column first, and also makes queries which don't filter on prefix eligible for the index.

Similar to the fix on desktop, we create a new table and copy the data over due to sqlite limitations on modifying constraints. However, we can't use defer_foreign_keys like we do on desktop, since the foreign key here is ON DELETE CASCADE -- therefore we have to also temporarily null out the foreign key references and copy them back afterward. This makes the migration much slower and generate much more WAL than the respective migration on desktop.

As an alternative, we could modify sqlite_schema directly: this is more risky, but far more efficient: only ~76ms and ~0MB WAL as opposed to ~2060ms and ~22MB WAL on my actual 58MB places DB. It also has some precedent in this codebase (migration 17 does it), unlike desktop. An example of that approach can be seen here: shawnz@e157a2b

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
    • This change doesn't include any user-facing functional changes and historically, index changes like this haven't gotten changelog entries.
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

This change ports the fix from https://bugzilla.mozilla.org/show_bug.cgi?id=2025999 which replaces the (prefix, host) unique index on moz_origins with a (host, prefix) index instead. This improves the performance of the index by putting the higher-cardinality column first, and also makes queries which don't filter on prefix eligible for the index.

Similar to the fix on desktop, we create a new table and copy the data over due to sqlite limitations on modifying constraints. However, we can't use defer_foreign_keys like we do on desktop, since the foreign key here is ON DELETE CASCADE -- therefore we have to also temporarily null out the foreign key references and copy them back afterward.
@shawnz
shawnz force-pushed the shawnz/origins-index-order branch from ec21654 to 47dd6a4 Compare August 2, 2026 00:55
@mhammond

mhammond commented Aug 3, 2026

Copy link
Copy Markdown
Member

Thanks for this!

As an alternative, we could modify sqlite_schema directly: this is more risky, but far more efficient: only ~76ms and ~0MB WAL as opposed to ~2060ms and ~22MB WAL on my actual 58MB places DB. It also has some precedent in this codebase (migration 17 does it), unlike desktop. An example of that approach can be seen here: shawnz@e157a2b

Thanks for this too, and the performance does scare me a little - 2s for a migration on a phone that's probably fairly recent and probably not an outlier in terms of size sounds bad. The fact Lina already did for the 17 migration does make me think we should consider it. When you say risky, do you just mean that if we do something dumb we might destroy the DB, or is there something more subtle here?

@bendk @mak77 any thoughts?

@mhammond

mhammond commented Aug 3, 2026

Copy link
Copy Markdown
Member

my actual 58MB places DB.

not an outlier in terms of size

Actually smaller than our maintenance target of 75MiB

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.

2 participants