Update moz_origins index column order to match desktop - #7521
Conversation
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.
Instead of rebuilding the entire moz_origins table, which requires nulling out and then restoring the entire origin_id column in the large moz_places table, we can just rewrite the schema in sqlite_schema and then trigger a REINDEX. This is not as safe, and has the risk of causing silent data corruption if the schema is modified incorrectly (like for example if it was unknowingly modified by application code). But, it is significantly faster and generates significantly less WAL.
ec21654 to
47dd6a4
Compare
|
Thanks for this!
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? |
Actually smaller than our maintenance target of 75MiB |
Here are some of the risks I see with using writable_schema here
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 |
|
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. 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 |
I would have thought you could use |
DROP INDEX can't remove indexes associated with constraints, and there's no ALTER TABLE DROP CONSTRAINT support in sqlite. e.g. regarding triggers, they aren't a problem here. the ON DELETE CASCADE is implemented without using triggers |
|
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 |
|
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:
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 |
|
ok, since it sounds like everyone is interested in going with the writable_schema approach, i pushed that change into this branch |
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
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.
| AND sql LIKE '%UNIQUE (host, prefix)%'", | ||
| [], | ||
| )?; | ||
| if !already_inverted { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
done in 83aad88. I named it sql/legacy/create_shared_schema_v20.sql because 20 was the value of VERSION prior to my changes.
I think, with this change, we should now consider adding a test which calls assert_schema_matches_new_database, like several other modules have already. But there's a problem with that: it falsely panics here due to several whitespace issues, comments, as well as the presence of table stats due to us running ANALYZE in the migrations. So if we wanted to do that, we'd have to update it to handle those things in its normalization function. Let me know if you want me to look into that for this change
There was a problem hiding this comment.
That function tries to normalize things, but I could easy see this breaking that code. If you wanted to fix that, that's great with me. If you didn't, then I'm also okay with landing this as-is and I can try to fix up that code since I wrote it.
There was a problem hiding this comment.
Other than that, this PR is looking very good to me. I think you could remove the already_inverted check, but I'm okay leaving that in if you think the check is worth it.
There was a problem hiding this comment.
I should check with @mak77 -- what's the rationale exactly for including the already-executed guards in desktop? Is that something we need to worry about here or not? I added it on desktop on the basis of this comment, where he mentions that migrations can run multiple times in some cases. But perhaps that's an issue unique to desktop? https://phabricator.services.mozilla.com/D313507#inline-1685896
There was a problem hiding this comment.
My understanding of the app-services code is that migrations should only run once. Maybe this is a Desktop-only issue or maybe it's because Desktop is deployed to more machines and I just haven't seen the error case that they have.
There was a problem hiding this comment.
it's an explicit choice on desktop to downgrade the schema on downgrade, and reapply migrations, so if the downgrade adds more data that must be migrated, a migration can handle it. Users on desktop are more likely to downgrade on issues. Probably doesn't affect application-services, but it depends on how migration was designed.
There was a problem hiding this comment.
i see. i looked into it and i understand a bit better now, you can see here that upon a downgrade of the software, the schema_version gets decremented on desktop: https://github.com/mozilla-firefox/firefox/blob/04b29f9c2d2dbf5639c3f45ea812bb4c21dc81c6/toolkit/components/places/Database.cpp#L1212-L1215 thus causing new migrations to run a second time on re-upgrade. whereas on mobile, that's just an error:
application-services/components/support/sql/src/open_database.rs
Lines 163 to 164 in 2de6bfb
i removed the check in e396d66, and also tried to improve the normalization in 7fc6879 so that we can call assert_schema_matches_new_database for the places module in f4ce749.
The pull request has been modified, dismissing previous reviews.
Several places DB migrations invoke the shared schema SQL to create/refresh indexes. In order to avoid older migrations being impacted by schema changes, add a frozen copy of the places schema at version 20 and update past migrations to invoke that instead. In future migrations, we can either avoid this pattern or add new frozen copies of the schema at the time of those migrations, following the same pattern.
2b7f558 to
83aad88
Compare
This is no longer necessary now that we've fixed the CREATE_SHARED_SCHEMA_V20_SQL in place, since there's no chance the table could be created with the already correct index order in a previous migration. And unlike on desktop, we never reapply migrations after a downgrade-then-upgrade. Thus, there's no scenario where this will run twice.
In the normalize function, switch to a state machine approach which is able to drop comments and also remove more kinds of syntactically meaningless whitespace -- i.e., not just repeated whitespace but also whitespace next to punctuation. Additionally, ignore sqlite_stat* tables when comparing schemas, since they are only created in some circumstances, like if ANALYZE was run. This can lead to problems when comparing a migrated schema to a freshly built one. This change is in preparation for supporting the use of assert_schema_matches_new_database in the places module.
Since we are now changing the behaviour of the old places DB migrations by storing and applying previous versions of the shared schema, it is prudent to add some tests which verify the migrated database matches the freshly built one. There is already a assert_schema_matches_new_database function which can check this, but it previously wasn't possible to use it in the places module since its normalization system wasn't powerful enough to account for all the comments and whitespace deviations in the places shared schema. That has now been improved, so add a call to that function in the places module.
5 participants
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_schemadirectly: 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@e157a2bPull Request checklist
[ci full]to the PR title.