Closed Bug 1451537 Opened 6 years ago Closed 6 years ago

Simplify tag query rewriting in the mirror

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(2 files)

See bug 1293445, comment 39. Migration doesn't preserve other params for tag contents queries, so we can remove `rewriteRemoteTagQueries`, rewrite when we insert into `moz_places`, and *maybe* flag rewritten queries for weak reupload. (Or make sure the validator ignores them; users can fix them manually if they notice, and then we'll sync the change).

These queries are already uncommon, so the simpler we can make this, the better. It's also not critical.
Priority: -- → P2
Assignee: nobody → kit
Summary: Remove tag contents query rewriting from the mirror → Simplify tag query rewriting in the mirror
Comment on attachment 8967608 [details]
Bug 1451537 - Simplify tag query rewriting in the bookmarks mirror.

https://reviewboard.mozilla.org/r/236316/#review242160
Attachment #8967608 - Flags: review?(mak77) → review+
Comment on attachment 8967609 [details]
Bug 1451537 - Bump bookmarks mirror schema version and add a migration test.

https://reviewboard.mozilla.org/r/236318/#review242164

you may want to file a follow-up to attach some kind of telemetry to this db. What we discovered with places.sqlite is that you can't really know what happens when you try to replace a corrupt db in the wild.
It would be nice to have at least some basic telemetry like db size and the reached point when handling a corrupt db.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1922
(Diff revision 1)
>   * @param {Sqlite.OpenedConnection} db
> - *        The mirror database connection.
> + *        The Places database connection.
> + * @param {String} path
> + *        The full path to the mirror database file.
>   */
> -async function migrateMirrorSchema(db) {
> +async function attachMirrorDatabase(db, path) {

attachAndInitMirrorDatabase?
Attachment #8967609 - Flags: review?(mak77) → review+
Comment on attachment 8967608 [details]
Bug 1451537 - Simplify tag query rewriting in the bookmarks mirror.

https://reviewboard.mozilla.org/r/236316/#review242864

Sorry, I thought I already reviewed this!
Attachment #8967608 - Flags: review?(markh) → review+
Comment on attachment 8967609 [details]
Bug 1451537 - Bump bookmarks mirror schema version and add a migration test.

https://reviewboard.mozilla.org/r/236318/#review242164

Thanks for reviewing! I added telemetry for recording why we failed to open the database in this patch (`initialize` for non-corruption errors, `remove` if we failed to remove the old file, and `replace` if we failed to initialize the replacement database). DB size telemetry is in bug 1451152.
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fa6af3138d5
Simplify tag query rewriting in the bookmarks mirror. r=mak,markh
https://hg.mozilla.org/integration/autoland/rev/8dbdf2617258
Bump bookmarks mirror schema version and add a migration test. r=mak
https://hg.mozilla.org/mozilla-central/rev/1fa6af3138d5
https://hg.mozilla.org/mozilla-central/rev/8dbdf2617258
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1455183
You need to log in before you can comment on or make changes to this bug.