<dietrich> ok, so bug 372501 changes the db schema in a non-trivial way <dietrich> and there are more changes coming down the pipe <sspitzerMsgMe> about the places.sqlite file, and migration, I don't think we should worry about migrating previous alphas. is there a version internal that we can bump? <sspitzerMsgMe> to cause it to be blown away? so I propose we add the version and increment it each time, starting with when bug #372501 lands, and again, as we make changes (as dietrich indicates that we will). during the alpha stages, I think this sort of data loss (of bookmarks and history) is ok. or am I being too cavalier? I just don't think writing migration code is worth it right now. once we hit a public beta, different story. note, I think this will cause us to re-import history bookmarks from firefox 2 (if they exist), which is OK. this fix should go in nsNavHistory::InitDB()

(In reply to comment #0) > during the alpha stages, I think this sort of data loss (of bookmarks and > history) is ok. or am I being too cavalier? I just don't think writing > migration code is worth it right now. > > once we hit a public beta, different story. i think that this is great for now, not too cavalier. however, i think we'll have to evaluate the alphas as we go to see whether it's necessary to do this (cost for migration, # of users/dls, etc).

(In reply to comment #0) > or am I being too cavalier? Count the number of people who've posted in http://forums.mozillazine.org/viewforum.php?f=23 in the last couple months. Multiply by five. That's how many ALL CAPS bugspams with multiple exclamation points I'd expect. If you're doing dataloss for nightlies. Releases? I know I'd stop watching the places and bookmarks and history default qa accounts, until the noise died down, say a couple of months.

If you delete nightly testers' bookmarks, you can expect no more nightly testers. For places, we were always very careful to write migration code. Not that it was fun, but there's really no other choice. When places was turned off, there were posts to devmo, etc. and several weeks' notice that you needed to download a new build and export your bookmarks to HTML to keep them. This was just to avoid losing bookmarks added or modified since places was turned on.

Thanks for the input, point taken. I'm aiming to have all (expected) schema changes completed before bookmarks-on-places gets flipped on the trunk, so we should only clobber the db until that point. After that point, we can use this versioning system to more cleanly detect and handle migration when schema changes occur.

in nsNavHistory::InitDB(), my plan is to use "PRAGMA user_version" (and "PRAGMA user_version=X") to get / set the version. from http://www.sqlite.org/pragma.html#version Pragmas to query/modify version values * PRAGMA [database.]schema_version; PRAGMA [database.]schema_version = integer ; PRAGMA [database.]user_version; PRAGMA [database.]user_version = integer ; The pragmas schema_version and user_version are used to set or get the value of the schema-version and user-version, respectively. Both the schema-version and the user-version are 32-bit signed integers stored in the database header. The schema-version is usually only manipulated internally by SQLite. It is incremented by SQLite whenever the database schema is modified (by creating or dropping a table or index). The schema version is used by SQLite each time a query is executed to ensure that the internal cache of the schema used when compiling the SQL query matches the schema of the database against which the compiled query is actually executed. Subverting this mechanism by using "PRAGMA schema_version" to modify the schema-version is potentially dangerous and may lead to program crashes or database corruption. Use with caution! The user-version is not used internally by SQLite. It may be used by applications for any purpose.

but...: 1) we still need the actual migration code before we enable the code that sets the user_version 2) what about the scenario of someone who toggle between versions with different schemas?

adding a dependency on bug 374876 b/c we don't want to do any kind of migration w/o first backing up the users bookmarks.

Adds migration code from 0 -> 1. This initial migration is from history-only Places to history'n'bookmarks Places, recreates the bookmarks tables, forcing re-import of bookmarks.html.

hey dietrich, two quick questions / comments: 1) after you check in, but before we have schema version 2, we will be producing builds with version 1. what about adding code (now) so that we do something when a user switches back from version 2,3,... back to 1? if (PLACES_SCHEMA_VERSION == 1 && schemaVersion == 0) { ... } else { // handle downgrading... } for "// handle downgrading...", perhaps we could at least assert and or dump a message to the console. or should we call MigrateFromV0ToV1() [and maybe rename it to MigrateFromVnToV1] 2) +// This just dumps all bookmarks-related tables, recreates them +// forcing a re-import of bookmarks.html. I see that we call InitTables() to recreate them, but were are we forcing the re-import of bookmarks.html? and, will the code to re-import bookmarks.html gracefully handle the scenario where bookmarks.html doesn't exist?

Comment on attachment 259559 [details] [diff] [review] fix v2 clearing the review request. I'll waiting for dietrich to respond to my comments and questions.

(In reply to comment #10) > hey dietrich, two quick questions / comments: > > 1) after you check in, but before we have schema version 2, we will be > producing builds with version 1. what about adding code (now) so that we do > something when a user switches back from version 2,3,... back to 1? > > if (PLACES_SCHEMA_VERSION == 1 && schemaVersion == 0) { > ... > } > else { > // handle downgrading... > } > > for "// handle downgrading...", perhaps we could at least assert and or dump a > message to the console. or should we call MigrateFromV0ToV1() [and maybe > rename it to MigrateFromVnToV1] > calling MigrateFromV0ToV1() for downgrades has the potential for data loss. so for now, i'm just going to assert. > 2) > > +// This just dumps all bookmarks-related tables, recreates them > +// forcing a re-import of bookmarks.html. > > I see that we call InitTables() to recreate them, but were are we forcing the > re-import of bookmarks.html? The import is triggered from within InitRoots. > and, will the code to re-import bookmarks.html gracefully handle the scenario > where bookmarks.html doesn't exist? > Yes: http://lxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavBookmarks.cpp#426

Thanks for answering my questions. > calling MigrateFromV0ToV1() for downgrades has the potential for data loss. so > for now, i'm just going to assert. what will happen when you run a build with v1 after running a build with v2? That could lead other issues, like bug #374640, right? Downgrade might be lossy (are you thinking of things we don't currently export / import?), but at least data in the places.sqlite file will match matches the code. Or am I confused?

(In reply to comment #13) > Thanks for answering my questions. > > > calling MigrateFromV0ToV1() for downgrades has the potential for data loss. so > > for now, i'm just going to assert. > > what will happen when you run a build with v1 after running a build with v2? > That could lead other issues, like bug #374640, right? > > Downgrade might be lossy (are you thinking of things we don't currently export > / import?), but at least data in the places.sqlite file will match matches the > code. > > Or am I confused? > no, you're correct. sorry, i'm fighting a bad cold, so my logic is being forced through a snot filter. i'll do the brute-force downgrade for now (in attached patch).

I think that losing data (new annotations, etc) because I tried a backlevel version isn't going to be acceptable. I could see a case for prompting along the lines of: "Your bookmarks are from a newer version of Firefox, and cannot be read. You can import your saved bookmarks from a backup, however this may result in the loss of some additional information about your bookmarks." (Wordsmithing sorely needed.) We would default to not doing the lossy import in this case. We should be able to disable writing any bookmarks data if the assert happens or the user chooses to not blow away the newer data. Not sure how centralized the storage calls are here. If we can do that, we could import the bookmarks into memory, but keep them read-only, which is probably about right for downrev usage in the majority case.

(In reply to comment #15) > I think that losing data (new annotations, etc) because I tried a backlevel > version isn't going to be acceptable. I could see a case for prompting along > the lines of: > > "Your bookmarks are from a newer version of Firefox, and cannot be read. You > can import your saved bookmarks from a backup, however this may result in the > loss of some additional information about your bookmarks." (Wordsmithing > sorely needed.) We would default to not doing the lossy import in this case. > > We should be able to disable writing any bookmarks data if the assert happens > or the user chooses to not blow away the newer data. Not sure how centralized > the storage calls are here. If we can do that, we could import the bookmarks > into memory, but keep them read-only, which is probably about right for downrev > usage in the majority case. > Yes, we need to have a solution like that when downgrading between versions when there's known data loss. This patch doesn't touch the annotation tables, so there's no issue there. The loss at this point is the import/export delta in bug 375108.

Comment on attachment 259758 [details] [diff] [review] fix v4 r=sspitzer one question: + nsresult // static why the "// static"?

Checking in nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.113; previous revision: 1.112 done Checking in nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHistory.h new revision: 1.72; previous revision: 1.71 done (In reply to comment #17) > (From update of attachment 259758 [details] [diff] [review]) > r=sspitzer > > one question: > > + nsresult // static > > why the "// static"? > leftover comment from an earlier version. i removed it before checking in. (In reply to comment #12) > > 2) > > > > +// This just dumps all bookmarks-related tables, recreates them > > +// forcing a re-import of bookmarks.html. > > > > I see that we call InitTables() to recreate them, but were are we forcing the > > re-import of bookmarks.html? > > The import is triggered from within InitRoots. This is not triggered when moving between places-no-bookmarks and places-bookmarks, which I'll fix in bug 376008 (wherein import will happen outside of service initialization).

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv