History schema evolution#5470
Conversation
| // handle that, but it would mean changing a lot of legacy code that assumes that visits are | ||
| // immutable and can be compared based on the timestamp + transition fields (or the other code that | ||
| // compares only the timestamps). If we want to address this case, let's do it as part of a | ||
| // general rework of the places sync merge code. |
There was a problem hiding this comment.
What do others think about this one? The current code treats visits are currently considered immutable, which means if we add unknown fields, they can't change either.
Should we try to rework the code so it doesn't make that assumption? I kind of want to rework the merging code anyways and this could be a good excuse.
Another question is if we want to allow new fields to be part of the "key". Visits don't have a GUID, we treat two visits the same if they have the same timestamp + transition type (except sometimes it's just timestamp -- one more reason that we may want to rework this code). Would we ever want to allow new fields to be added to this key? Right now I don't think it's worth the complexity, but I wonder what others think.
There was a problem hiding this comment.
One thing I'm trying to keep in mind of roundtripping this data, is really to prevent data loss until the clients that can "catch up" and actually understand the data in those fields.
Which means -- unknown_fields should eventually disappear as a client becomes "recent", having unknown_fields stick around even if every client can understand the schema -- seems bad (IMO). I do like the idea of reworking so that we avoid this pitfall but depends on our timelines.
To your second question, I agree given the added complexity and we don't have a spec we'd be trying to design the would-be new "keys" around -- might be better to de-prioritize that.
There was a problem hiding this comment.
Which means -- unknown_fields should eventually disappear as a client becomes "recent", having unknown_fields stick around even if every client can understand the schema -- seems bad (IMO). I do like the idea of reworking so that we avoid this pitfall but depends on our timelines.
I'm not understanding this in relation to this question. I think we will eventually null out unknown_fields once we upgrade no matter what. My question is if we should support changes to unknown_fields before that happens.
However, it's a really good point in general and it's making me rethink the pruning idea.
There was a problem hiding this comment.
Oh, this is tricky! I could see us wanting to add new fields to individual visits in the future, but I think it's definitely worth doing that refactor separately.
The fact that we keep a rolling set of just the last 20 visits around, and don't really support per-visit operations, makes me think that any fields we add won't be very durable—visits written by older devices won't have those new fields, and, since a device might have visits that occurred in between ones that are already on the server, every device's view of the history will be slightly different. They'll have records that you visited a page at least once, but the timestamps might be off.
Another question is if we want to allow new fields to be part of the "key". Visits don't have a GUID...
Oh gosh, yes, the fact that visits don't have an "identity" apart from their timestamps complicates other things, too—including deletions, which you found in #5472. I'm also not sure what kinds of realistic "key" fields (per-device, per-visit annotations?) we can add, though, and supporting that might be too invasive in the current Sync model.
There was a problem hiding this comment.
I think a visit is considered immutable because it's recording a historical event - so changing it in the future really doesn't make sense. I can't see how a different device would somehow say "this old visit I previously recorded now should have different attributes".
Similarly, I think the key question is roughly the same - it's a record of something happening at an instant - I don't see how other fields being part of the key makes sense - you'd never want to record 2 different visits of the same type at the same timestamp with different attributes - there can only really have been one visit at that instant.
If that all makes sense, maybe just update the comment to reflect that?
0f1e4c5 to
185c730
Compare
skhamis
left a comment
skhamis
left a comment
There was a problem hiding this comment.
Seems like a great start! It seems close, but the pruning of unknown_fields separate from visits gives me a little pause. Feels we might need to understand the implications of that a bit more
| "visits": [], | ||
| // Simulate 2 new features that nobody is asking for: | ||
| // - themes for specific URLs | ||
| // - prank the user when they try to open a URL |
There was a problem hiding this comment.
🤣 this gave me a good chuckle
| // handle that, but it would mean changing a lot of legacy code that assumes that visits are | ||
| // immutable and can be compared based on the timestamp + transition fields (or the other code that | ||
| // compares only the timestamps). If we want to address this case, let's do it as part of a | ||
| // general rework of the places sync merge code. |
There was a problem hiding this comment.
One thing I'm trying to keep in mind of roundtripping this data, is really to prevent data loss until the clients that can "catch up" and actually understand the data in those fields.
Which means -- unknown_fields should eventually disappear as a client becomes "recent", having unknown_fields stick around even if every client can understand the schema -- seems bad (IMO). I do like the idea of reworking so that we avoid this pitfall but depends on our timelines.
To your second question, I agree given the added complexity and we don't have a spec we'd be trying to design the would-be new "keys" around -- might be better to de-prioritize that.
| Ok(()) | ||
| } | ||
|
|
||
| fn prune_unknown_fields(conn: &PlacesDb, now: Timestamp) -> Result<()> { |
There was a problem hiding this comment.
This is a nice solution! Would be nice to have some comments of why this exists and why we need to prune (I'm assuming it's related to the immutable visits discussion).
That being said, I'm not entirely sure how I feel of separating the maintenance of pruning unknown_fields -- wouldn't they get removed with the visits their associated with (in prune_older_visits) or is this used for something else? Comments here might also lessen the confusion of this.
There was a problem hiding this comment.
Sorry, I meant to add a comment here and totally forgot about it. But now that I'm writing it down, I'm having second thoughts. Maybe we shouldn't special case the unknown_fields and just continue to use the existing pruning code.
The reasoning behind the prune_unknown_fields is that we only sync the most recent 20 visits for each URL, so storing unknown_fields for visits older than that seems like a waste of space. However, that's not really true:
- As you point out, the client will upgrade and at that point it will want to migrate the
unknown_fieldsdata and start using it. - If you delete recent visits, then that will push older URLs into the most recent 20. (This is related to History syncing doesn't delete visits #5472).
Part of the motivation here is there's a lot of visits in the DB and unknown_fields is space inefficient. You want to add an enum column that could be stored in as an integer, you end up storing an entire JSON string. Maybe we can address this another way though:
- Lets say that we are on nightly version v120 and want to add a new feature that requires a new field
- Before starting work on that feature, we add a migration to store the new field in a column and round-trip the data.
- The migration should be pretty trivial, so we uplift it into v119 and an ESR patch ahead of the v120 release.
- That way users who are doing a reasonable job keeping up-to-date don't have to deal with the space penalty.
781013a to
5beccc2
Compare
|
Based on the above thoughts and some conversations, I reworked this one to only add support for unknown fields and not implement pruning. My feeling is that if we wanted to evolve the history visits table, we should use the workflow-based approach I mentioned above. But we can always revisit this one and decide to re-implement the pruning. @skhamis how does this one look now? |
0b4d951 to
5beccc2
Compare
skhamis
left a comment
skhamis
left a comment
There was a problem hiding this comment.
Thanks for making those changes. I think it looks good! I think we'll be able to better prune the unknown_fields when the time comes to "consume" said fields.
👍 from me! But I'd wait for Linas review before merging!
| // handle that, but it would mean changing a lot of legacy code that assumes that visits are | ||
| // immutable and can be compared based on the timestamp + transition fields (or the other code that | ||
| // compares only the timestamps). If we want to address this case, let's do it as part of a | ||
| // general rework of the places sync merge code. |
There was a problem hiding this comment.
I think a visit is considered immutable because it's recording a historical event - so changing it in the future really doesn't make sense. I can't see how a different device would somehow say "this old visit I previously recorded now should have different attributes".
Similarly, I think the key question is roughly the same - it's a record of something happening at an instant - I don't see how other fields being part of the key makes sense - you'd never want to record 2 different visits of the same type at the same timestamp with different attributes - there can only really have been one visit at that instant.
If that all makes sense, maybe just update the comment to reflect that?
linabutler
left a comment
linabutler
left a comment
There was a problem hiding this comment.
This looks great to me, too; thanks @bendk, and apologies for the delay!
My only question is how deduping will interact with this, but more about capturing the current semantics in a test, and not trying to define new semantics for it.
I also think your approach of adding unknown fields to just the page record, instead of individual visits, is great. If we do decide to look closer at visits, I think it's worth having that discussion in the context of other changes, like supporting deletions for them, instead of trying to solve all that here.
Thank you again! 🎊
| "visits": [], | ||
| // Simulate 2 new features that nobody is asking for: | ||
| // - themes for specific URLs | ||
| // - prank the user when they try to open a URL |
| // handle that, but it would mean changing a lot of legacy code that assumes that visits are | ||
| // immutable and can be compared based on the timestamp + transition fields (or the other code that | ||
| // compares only the timestamps). If we want to address this case, let's do it as part of a | ||
| // general rework of the places sync merge code. |
There was a problem hiding this comment.
Oh, this is tricky! I could see us wanting to add new fields to individual visits in the future, but I think it's definitely worth doing that refactor separately.
The fact that we keep a rolling set of just the last 20 visits around, and don't really support per-visit operations, makes me think that any fields we add won't be very durable—visits written by older devices won't have those new fields, and, since a device might have visits that occurred in between ones that are already on the server, every device's view of the history will be slightly different. They'll have records that you visited a page at least once, but the timestamps might be off.
Another question is if we want to allow new fields to be part of the "key". Visits don't have a GUID...
Oh gosh, yes, the fact that visits don't have an "identity" apart from their timestamps complicates other things, too—including deletions, which you found in #5472. I'm also not sure what kinds of realistic "key" fields (per-device, per-visit annotations?) we can add, though, and supporting that might be too invasive in the current Sync model.
5beccc2 to
0b3e255
Compare
This was only used in tests, always returned a Some() value, and was always unwrapped. I think this was a artifact of how I write the tests and I just never remembered to remove the option.
I'm happy to say that `test_all_upgrades()` caught the error before this migration was in place.
0b3e255 to
00362b9
Compare
4 participants
Pull Request checklist
[ci full]to the PR title.Branch builds: add
[firefox-android: branch-name]to the PR title.