708331 - Need additional columns in browser content provider

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Jason Voll [:jvoll]]

We require the following columns to be added for bookmarks in the browser content provider:
String description
String keyword
Array of tags

For sync we also require a deleted flag (for both bookmark and history records).

Comment 1

[Jason Voll [:jvoll]]

For tags:
Tags is an array of strings. It is up to you how you choose to implement this on your end. If you want to make it simple and will not be using this data at any point, you are welcome to accept a json string representing the array and just store that.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I am leary of adding too much stuff that mobile never plans to use. I realize you might want this for roundtripping, but couldn't we just ignore things that don't exist in one schema or the other?

Comment 3

[Jason Voll [:jvoll]]

I believe some of your questions are answered in rnewman's comments on 708149. If you have further questions/concerns feel free to ping rnewman in #mobile.

Comment 4

[Doug Turner (:dougt)]

what mfinkle said.  every byte counts.  justify the need or burn it with fire.

Comment 5

[Richard Newman [:rnewman]]

mfinkle: I would love to just ignore things, but that's not how Sync works. It doesn't schlep deltas around, it does whole records. If Fennec changes something in a record, or has to re-upload it for some other reason (e.g., a node migration), then other clients will take that record as truth.

That means data loss, and of course that's not acceptable.

(If we treat Fennec as Firefox Home -- i.e., not permitted to write to the Sync server -- then we can ignore fields, but I don't think we want to do that.)

To change that behavior would be a significant change for Sync, involving a protocol and perhaps a storage bump, as well as a multi-month cascade to get changes into Firefox and a handful of third-party clients. That's unlikely to happen.

Our choices, then, are:

* Put the data into Fennec's DBs. Most records won't have all three of those fields, if any, so these should be cheap to store as nulls. If you ever decide to try to get closer to the features of the Awesomebar, you'll have the data there to search. But yes, each bookmark row will be larger.

* Instantiate new sqlite databases on the Sync side, and track additions to the data that Fennec keeps. This would be less space efficient (storing GUID and Android ID twice, essentially, as well as needing to include some reference to profile IDs), and consume more RAM for the additional DB handles and Java objects, as well as performing more file IO and introducing an opportunity for error.

I favor the former.

Enough justification?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #5)

> Enough justification?

Yes, but I don't have to like it. Let's try to store the stuff we don't use in a way that causes the least impact on DB size and performance.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Add keyword, description, and tags columns to Bookmarks

Note: there's no need for a schema migration at this point because I haven't enabled local DB yet. There's no users running on top of the schema version 1 right now.

Comment 8

[Lucas Rocha (:lucasr)]

Another note: the tags cols will be of TEXT type for 1.0. Implementing a proper schema for tags would add a fair amount of complexity to the schema for a feature that we're not even using in Fennec right now. Consider the tags column just as something to help us sync properly.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 580146 [details] [diff] [review]
Add keyword, description, and tags columns to Bookmarks

Review of attachment 580146 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, assuming you don't want to take on the extra burden of a separate table.

(A separate table would reduce the minimum space required for a row, at the cost of slightly complicating deletion and of course introducing another table. I'm not concerned about increased complication in querying from Sync's side.)

blassey's call on which side of the tradeoff to choose.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #5)
> mfinkle: I would love to just ignore things, but that's not how Sync works.
> It doesn't schlep deltas around, it does whole records. If Fennec changes
> something in a record, or has to re-upload it for some other reason (e.g., a
> node migration), then other clients will take that record as truth.
> 
> That means data loss, and of course that's not acceptable.
> 
> (If we treat Fennec as Firefox Home -- i.e., not permitted to write to the
> Sync server -- then we can ignore fields, but I don't think we want to do
> that.)
> 
> To change that behavior would be a significant change for Sync, involving a
> protocol and perhaps a storage bump, as well as a multi-month cascade to get
> changes into Firefox and a handful of third-party clients. That's unlikely
> to happen.
> 
> Our choices, then, are:
> 
> * Put the data into Fennec's DBs. Most records won't have all three of those
> fields, if any, so these should be cheap to store as nulls. If you ever
> decide to try to get closer to the features of the Awesomebar, you'll have
> the data there to search. But yes, each bookmark row will be larger.
> 
> * Instantiate new sqlite databases on the Sync side, and track additions to
> the data that Fennec keeps. This would be less space efficient (storing GUID
> and Android ID twice, essentially, as well as needing to include some
> reference to profile IDs), and consume more RAM for the additional DB
> handles and Java objects, as well as performing more file IO and introducing
> an opportunity for error.
> 
> I favor the former.
> 
> Enough justification?

I'd like to actually know what it would take to change things on the sync side. You're basically trading off extra storage on the server side (new protocol, new storage) vs. extra storage in the users' profile and I don't think that's the right trade off.

Also, why doesn't sync work with deltas?

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to Brad Lassey [:blassey] from comment #10)
> I'd like to actually know what it would take to change things on the sync
> side. You're basically trading off extra storage on the server side (new
> protocol, new storage) vs. extra storage in the users' profile and I don't
> think that's the right trade off.

This is not the trade off we're dealing with here. The trade off is between "writing an Android client from scratch" and "redesigning Sync and writing not only an Android client from scratch but also rewriting the Firefox Sync server components, Firefox Sync and Firefox Home clients".

> Also, why doesn't sync work with deltas?

Please ask the team who came up with Weave 3 years ago and everybody who resisted a back-to-the-drawing-board rewrite of Sync before forcing it into the product. I would love to know, too, but this question isn't a very useful one to ask if you want to solve this problem in a reasonable timeframe.

Comment 12

[Richard Newman [:rnewman]]

(In reply to Brad Lassey [:blassey] from comment #10)

> I'd like to actually know what it would take to change things on the sync
> side. You're basically trading off extra storage on the server side (new
> protocol, new storage) vs. extra storage in the users' profile and I don't
> think that's the right trade off.

When I was talking about "the Sync side" I meant in Android Sync. Rather than instantiate three new columns (or one new table with four columns) in Fennec's content provider, we could instantiate a whole new database and a pile of complexity in our process to make sure we don't delete data from our users' computers.

To alter the Sync protocol in any capacity would take on the order of a year to go from inception to full deployment (depending on complexity of the change, and honestly we wouldn't bother for something small), and needs very careful thought. There are a multitude of deployed clients, many of whom we haven't built; migration issues; multiple datacenters to rev; etc. etc. ad infinitum. We have a wishlist for the protocol itself:

  https://wiki.mozilla.org/Services/Sync/Protocol_2.0

but the storage version isn't likely to change much even in that case: the protocol can be revved on top of the same storage version without breaking old clients, but the reverse is not true.

Practically speaking, making a change to the Sync protocol to accommodate a limitation in Fennec is a no-go. It would have to result from some massive intrinsic failings in the mobile stack, such as "can't use HTTP", or "cannot encrypt data with AES", and it definitely wouldn't be on the cards for this quarter.


So, considering the real question of Fennec storing description/keyword/tags, versus Android Sync storing the same in sqlite on Android:

  Fennec:
    * An additional ~12 bytes per record (assuming minimum 4 bytes for a NULL flag or TEXT pointer) per bookmark row (of course more for records that have one of the added fields, but for most users it will be minimal).
    * Alternative approach: 0 bytes per record in the bookmarks table, plus one table for additions. Would require a second clause to DELETE from that table when appropriate.
    * Patch is ready.
    * Schema can easily be changed through migrations in the future. (That doesn't mean "oh, we can fix this later" -- once users start putting their data in Fennec, it has to be right. But we can improve storage if it comes to that.)

  Sync:
    * An additional sqlite database with connections, schema, etc.
    * At least 36 bytes per record with any of those fields (storing profile, guid, ID on top of what Fennec would store).
    * Additional complication to integrate data across two content providers. Real chance of inconsistency here.
    * Knowledge of Fennec profiles!
    * Need to clean up on Fennec profile changes, mass deletions, etc.
    * No ability for Fennec to use this data in the future.
    * Quite possibly deadline slippage.

Basically, any reason why you might not want this change in Fennec would apply tenfold if Sync held the data, and come in to play every time Android decided to schedule a sync. You might save 150 bytes on the user's ten bookmarks, but you'd get swapped out when we requested 250+ kilobytes to open a whole new database connection to get the same data. Not a saving IMO!


> Also, why doesn't sync work with deltas?

Because it was a Labs project that never got the two rewrites it really ought to have had. :)

From a less cynical perspective, Firefox really wasn't -- and still isn't! -- designed to exchange deltas. On desktop Firefox we get very coarse change notifications, with minimal transactional semantics, and some missing events. On mobile we get nothing at all, instead relying on timestamps, and we don't want to spend the memory or processor to try to fake it by duplicating everything.

To implement a proper, rock-solid delta-based sync solution requires it to be baked-in, most likely near the database transaction level. From that perspective it's closer to database replication than anything else. Firefox is simply too old and crufty for that to be the case.

In the past few weeks I've been doing my best to make sure that Fennec's new data stores are closer to what a sync solution needs, because we've been through all this once before. It's much better to get it right up front than to spend a year adding guids and timestamps to every table, trying to remember your passwords.

Comment 13

[Richard Newman [:rnewman]]

tl;dr: trust me. Add the damn columns.

Comment 14

[Toby Elliott [:telliott]]

(In reply to Brad Lassey [:blassey] from comment #10)

> Also, why doesn't sync work with deltas?

The original Sync spec was driven by requirements such as "we need to be able to get the 10 most important bookmarks" and "we need to be able to get all the children of this node" (plus, a whole bunch of key-lookups for crypto keys for specific records, back when Sync was also a sharing engine). Ironically these requirements were to limit the amount of data we sent to those Nokia devices that couldn't parse a large response. Some of those requirements are no longer relevant, and there's been at least one proposed version that uses deltas. That's merely in the wishlist stage, though.

Comment 15

[Lucas Rocha (:lucasr)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/4c56ca404637