Update moz_origins index column order to match desktop

[shawnz]

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.

[mhammond]

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]

my actual 58MB places DB.

not an outlier in terms of size

Actually smaller than our maintenance target of 75MiB

[shawnz]

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?

Here are some of the risks I see with using writable_schema here

• If we do it wrong, we corrupt the db or silently start misinterpreting existing rows with a different column order, etc. But this should be detected by tests

• If the schema changes without us knowing, like for example if for some reason someone adds some application level code outside the migrations that makes schema changes, we'll be operating against a changed target and that will cause corruption/silent misinterpretation of rows. But it's pretty unlikely anyone would ever do that and we could possibly defend against it by asserting the old expected schema first or just doing a search and replace on the constraint part specifically.

• There could be a risk of the behaviour changing in future sqlite versions. Not sure how likely that is

• It prevents us from using SQLITE_DBCONFIG_DEFENSIVE, if we ever wanted to do that, although we can't anyway since 17 uses writable_schema

Overall I think it's probably worth it to use writable_schema here, especially since we already used it in the past, but since this is such a widely used product I figured I should think of some alternatives first

[mak77]

In general we try to avoid writable_schema, exacly because you're removing any guard rail from corrupting your own database. But we still support it (we did not add SQLITE_DBCONFIG_DEFENSIVE) because sometimes it's the only way to do certain things without paying excessive costs. It can be 20x faster.
Btw, with writable_schema is not just matter of rewriting the SQL, but also the autoindex must be rebuilt.

If the choice is multiple seconds of I/O VS ms, considered this is an index change, using writable_schema is probably ok, with the due diligence

[bendk]

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.

I would have thought you could use DROP INDEX/ADD INDEX and DROP TRIGGER/ADD TRIGGER trigger to do this without the temporary table or nulling out the columns. What breaks when you do it that way?

[shawnz]

I would have thought you could use DROP INDEX/ADD INDEX and DROP TRIGGER/ADD TRIGGER trigger to do this without the temporary table or nulling out the columns. What breaks when you do it that way?

DROP INDEX can't remove indexes associated with constraints, and there's no ALTER TABLE DROP CONSTRAINT support in sqlite. e.g.

DROP INDEX sqlite_autoindex_moz_origins_1;
Parse error near line 13: index associated with UNIQUE or PRIMARY KEY constraint cannot be dropped

regarding triggers, they aren't a problem here. the ON DELETE CASCADE is implemented without using triggers

[bendk]

Oh, that makes total sense sorry to sidetrack this. writable_schema seems okay to me and the downsides you list seem acceptable. In particular, there's many ways that users can mess up their database if they manually change it. Maybe this would be the first way that they could cause us to corrupt the database, but there are other ways to make it more-or-less unusable.

Does SQLite reindex the columns after the UPDATE statement for the schema table?

[shawnz]

you have to manually call REINDEX, but thankfully it can be done transactionally together with the schema update.

As far as I can tell, there's basically three things you need to do for correctness purposes after changing the schema in this case:

• Set PRAGMA writable_schema=RESET, to reload the schema in the current connection
• Then, call REINDEX to rebuild the index with the new column order
• Then increment PRAGMA schema_version so other connections also know it's been changed

This can all be done within a transaction together with the UPDATE on sqlite_schema and a rollback correctly brings you back to the previous state

[shawnz]

ok, since it sounds like everyone is interested in going with the writable_schema approach, i pushed that change into this branch

[bendk]

The new code looks great to me. My only concern is that it continues to handle CREATE_SHARED_SCHEMA_SQL like previous upgrades did. Maybe now is a good time to break the pattern.

[bendk]

IIUC, this check is required because previous schema upgrades execute CREATE_SHARED_SCHEMA_SQL which means they might have already ran this upgrade. This seems dangerous to me, since it means wherever that changes we'll need to think through the effects on every upgrade function.

How do others feel about copying the v19 version of that SQL into some other module and call it something like legacy_schemas. The upgrades can use legacy_schemas::CREATE_SHARED_SCHEMA_SQL and then we can change the real CREATE_SHARED_SCHEMA_SQL without needing to worry about that. There's probably a better naming system, but I like freezing the code that the upgrade functions run as much as possible.

[shawnz]

i actually put that check here just to better copy what desktop does, which has that same check because of (i think) the possibility of downgrades and re-applications of migrations that have already been applied?

but now that I think about it, you are right. it also defends against the scenario where the unversioned CREATE_SHARED_SCHEMA_SQL already ran with the new definition in an older migration. and I agree, having older migrations apply newer and potentially changing SQL is risky. i will prepare a change with your proposed design based on the contents of CREATE_SHARED_SCHEMA_SQL at the time of each past migration's migration 19's commit