Repository navigation
Conversation
|
There's a followup patch that should land with this, I'll update momentarily. |
|
Added the migration from 20 => 21 to clean up any existing orphans that should be deleted, and recalculates |
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
This looks right to me, with the possible exception of the comment I left about SyncStatus::normal. Mark should also take a look though, since he knows this code better than me.
| UPDATE moz_places SET foreign_count = | ||
| IFNULL((SELECT n FROM placeForeignCounts WHERE place_id = moz_places.id), 0) | ||
| WHERE foreign_count IS NOT | ||
| IFNULL((SELECT n FROM placeForeignCounts WHERE place_id = moz_places.id), 0); |
There was a problem hiding this comment.
Why is the WHERE needed? It seems like setting something to it's current value should be okay and just a no-op. Is there a trigger or something that runs when this is updated?
There was a problem hiding this comment.
There's a trigger that would normally run, but it's not defined yet, so that WHERE is just updating only what rows would change based on this recalculation.
There was a problem hiding this comment.
I don't have strong feelings either way, but it seems simpler and maybe faster to just update all rows. I'd expect that the WHERE check is slower than just setting the value regardless.
| DELETE FROM moz_places | ||
| WHERE foreign_count = 0 AND last_visit_date_local = 0 AND last_visit_date_remote = 0;", | ||
| normal = SyncStatus::Normal as u8, |
There was a problem hiding this comment.
Why only SyncStatus::Normal? I think this bug could also happen for SyncStatus::new
There was a problem hiding this comment.
Could you have a new one that's an existing orphan due to its grandparent being deleted?
There was a problem hiding this comment.
This is where we need Mark, but I thought SyncStatus::New just meant that it hasn't been synced yet.
There was a problem hiding this comment.
Oh. If that's what it means, it's probably ok since it should've been deleted?
There was a problem hiding this comment.
Couldn't it have hit the same bug you're fixing now? I didn't think it was sync only, just that we only observed it during sync.
| // places could remain with zero foreign_counts, instead of | ||
| // leaving them to be cleaned up by history, they are explicitly | ||
| // deleted at the end of this migration.) | ||
| db.execute_batch(&format!( |
There was a problem hiding this comment.
Could this be split up into a series of db.execute() calls? That way SQLite will handle interpolating values like normal and now rather than Rust's format! method.
There was a problem hiding this comment.
I thought now() isn't available because the connection doesn't run prepare (where that custom function is defined). But it's still a good question given the number of statements here.
There was a problem hiding this comment.
I'm fine with generating now in Rust, but my main concern is that SQLite should handle interpolation, rather than format!. The code would be something like:
db.execute("
INSERT OR IGNORE INTO moz_bookmarks_deleted(guid, dateRemoved)
SELECT guid, ? FROM moz_bookmarks
WHERE id IN (SELECT id FROM orphanedBookmarks)
AND syncStatus = ?", now, normal)?;
…an up after orphan bookmarks mitigation
For bug 2084607, this adds an explicit
PRAGMA recursive_triggers = ON. Accordingly, also it adds the following:moz_bookmarks_parent_afterdelete_triggerinsql/create_shared_triggers.sqlforeign_countdecrement/increment insrc/db/schema.rssrc/storage/bookmarks.rsThis restores the behavior for cascading deletions that were present with the previous FK, which were assumed to still be in effect.
Two more changes will be needed, addressed separately (handling existing erroneous orphans, and recalculating the
moz_places.foreign_count).Pull Request checklist
[ci full]to the PR title.