328603 - Calendar sqlite database issues; renaming of storage.sdb

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

After a bunch of discussion related to bug 325287, it seems that we need to do some thinking about our SQLite calendar strategy.  In particular:

Should calendar code use one or more of it's own databases?  The firefox formhistory and places code currently both have their own .sqlite files.  It seems that noone is currently using the "profile storage" except for calendar.  One advantage to doing this would be that if there was any sort of corruption, one could simply throw away a corrupt calendar without having to worry about any future central profile data that is stored there.  Perhaps the calendar management tables should live in profile storage, and the calStorageProvider calendars should get their own file?

What do we want to do about backups?  It made be advantageous to keep several backups similar to the way we do in the ICS provider.  Once calICachingCalendar lands, that may change things a bit, depending on datasize.  It's possible that we'll want to back up local, authoritative calendars, but not non-authoritative caches.  Backing up everything would be nice, though, since people may have local offline changes in the non-auth calendars.

Both of these questions seem like they may be instances of problems that may want to be informed by more general, tree-wide thinking.  Should there be general guidelines about what lives in profile.sqlite vs. what gets its own database?  If there's auto-backup code to be written, it seems like that's something that many consumers of mozStorage might be interested in.  

Answers to these questions seem likely to effect what sort of migration code is necessary / best for bug 325287, so I've marked this as blocking that for the moment.

Thoughts?

Comment 1

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 2

[Stefan Sitter [:ssitter]]

With the work on e.g. caching and offline and introducing of the new timezone database should we target this bug too and move from storage.sdb to a calendar specific .sqlite file?

Comment 3

[Martin Schröder [:mschroeder]]

(In reply to comment #2)
> With the work on e.g. caching and offline and introducing of the new timezone
> database should we target this bug too and move from storage.sdb to a calendar
> specific .sqlite file?

Renaming storage.sdb to calendar.sqlite (or something like that) would also be good for tb-intgration. Don't know about the other issues from comment#0.

Comment 4

[Daniel Boelzle [:dbo]]

My thoughts on renaming it have been that we do this with a "storage2" provider, a shiny new high perf storage provider with different schema, doing a clean cut.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

It feels to me like a new provider is a lot of work, whereas we can do the renaming soonish and get a good perf win on Windows for almost no work.  I think we should do this sooner rather than later; marking tb-integration+.

Comment 6

[Daniel Boelzle [:dbo]]

I don't think this is really something blocking calendar integration (resetting tb-integration?), but could/must be fixed towards the release (blocking-calendar1.0+).

Comment 7

[Peter Lairo]

I just had to track down the cause of a corrupt calendar by deleting the calendar file, and spent a huge amount of time trying to just find the darned file. This would have been a lot easier if the file were named "calendar.sqlite".

(similar frustration with address-book file names)

Comment 8

[Tony Mechelynck [:tonymec]]

If the default calendar is displayed as "Home Calendar", why not store it as Home Calendar.ics (well, file:///$HOME/.mozilla/sunbird/aq3c85jq.default/Home%20Calendar.ics or similar)?

Comment 9

[Peter Lairo]

(In reply to comment #8)
> If the default calendar is displayed as "Home Calendar", why not store it as
> Home Calendar.ics

Because the file also contains all subsequently created calendars (CalDAV etc.)?

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: patch - v1

This patch migrates all calendar data of storage.sdb into a new <profile-dir>/calendar-data/local.sqlite.

More QA appreciated, especially for XP/Vista!

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
> If the default calendar is displayed as "Home Calendar", why not store it as
> Home Calendar.ics (well,
> file:///$HOME/.mozilla/sunbird/aq3c85jq.default/Home%20Calendar.ics or
> similar)?
While this attempt certainly is charming, modifications would require to write down the entire ics file. Moreover one sqlite file could hold several local calendars, so we would need to spread those into different ics files.

(In reply to comment #9)
...
> Because the file also contains all subsequently created calendars (CalDAV
> etc.)?
storage.sdb only holds the registration information, but no calendar data. Calendar registration is going to migrate into the prefs (bug 403006).

Comment 12

[Simon Paquet [:sipaq]]

Daniel, is there a reason why we do not name it calendar.sqlite or local_calendar.sqlite? I think that would much better indicate what this file is for.

Comment 13

[Daniel Boelzle [:dbo]]

(In reply to comment #11)
> (In reply to comment #9)
> storage.sdb only holds the registration information, but no calendar data.
> Calendar registration is going to migrate into the prefs (bug 403006).
w.r.t. CalDAV calendars of course

(In reply to comment #12)
> Daniel, is there a reason why we do not name it calendar.sqlite or
> local_calendar.sqlite? I think that would much better indicate what this file
> is for.
The file is already hosted in a folder called 'calendar-data'. I don't think there's any need to further prefix it. The 'cache.sqlite' in that same folder isn't prefixed, too.

Comment 14

[Peter Lairo]

(In reply to comment #13)
> The file is already hosted in a folder called 'calendar-data'. I don't think
> there's any need to further prefix it. The 'cache.sqlite' in that same folder
> isn't prefixed, too.

It's still ambiguous to advanced (non-programmer) users what "local" refers to: Local "data"? Local "settings"? Local "cache for remote something"? I've been frustrated for years with these logical-sounding and minimalist approaches to the file names in the profile (cert8.db, key3.db, abook-6.mab, secmod.db, storage.sdb, and now: local.sqlite). I hope you will reconsider naming the file "local_calendar.sqlite". Not knowing what the files are for is a huge hassle & hurdle for those of us who manually copy files to other PCs (and Weave looks about 100 years from being finished).

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #14)
> It's still ambiguous to advanced (non-programmer) users what "local" refers to:
> Local "data"? Local "settings"? Local "cache for remote something"?
> [...]
> I hope you will reconsider naming the file "local_calendar.sqlite".

Why would the "local" in "local_calendar" be any clearer to those advanced users?  What decision are they trying to make based on that name, with which we can help them?  Would they be better served with a help-system entry?

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 355392 [details] [diff] [review]
patch - v1

>+                            // take over data and drop from storage.sdb tables:
>+                            for (let table in sqlTables) {

This moves all the tables from storage.sdb to the new file. But storage.sdb is not exclusively for calendar. It's a toolkit file [1]. Other extensions might have data in there too. I think it's more robust to only move known tables, so you are sure they contain calendar data and not something else.

[1] http://mxr.mozilla.org/comm-central/search?string=storage.sdb

Comment 17

[Peter Lairo]

(In reply to comment #15)
> Why would the "local" in "local_calendar" be any clearer to those advanced
> users?  

I was assuming there is another file that stores the (cached) data and/or settings for remote calendars. So it was as a contrast to (a possibly non-existent) remote_calendar.sqlite. If the local_calendar.sqlite is the only file that stores calendar data, then the name calendar.sqlite would be sufficient.

> What decision are they trying to make based on that name, with which we
> can help them?  

Here's my use case:

1. I have a bunch of events in my home PC that I would like to copy to my wife's/parent's/work PC for when I'm there, so I at least have all my calendar events up until whenever I copied them (or I do it every time I go to and use that other PC).

2. I need to easily and quickly (because I do it infrequently enough to forget some un-intuitive name, but often enough to not want to have to look it up every time) copy (hopefully only) one file to my USB stick.

Admittedly, Google's CalDAV calendars might quickly make local calendars obsolete. But I don't trust Google (http://atlasshrugs2000.typepad.com/atlas_shrugs/2008/11/sandboxed-black.html) and I (and most users) don't have the skills to (nor do we want to) set up our own CalDAV server.

> Would they be better served with a help-system entry?

F1-Help (SUMO) rarely seems to answer the questions I have, and I'd really prefer to avoid *having* to search the internet. Se "infrequently" and "often enough" above.

Thanks for asking.

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #16)
> (From update of attachment 355392 [details] [diff] [review])
> >+                            // take over data and drop from storage.sdb tables:
> >+                            for (let table in sqlTables) {
> 
> This moves all the tables from storage.sdb to the new file. But storage.sdb is
> not exclusively for calendar. It's a toolkit file [1]. Other extensions might
> have data in there too. I think it's more robust to only move known tables, so
> you are sure they contain calendar data and not something else.
> 
> [1] http://mxr.mozilla.org/comm-central/search?string=storage.sdb

Huh? Mvl, where do you see that in the patch?
I am only migrating tables from |sqlTables|, leaving any other untouched.

Comment 19

[Daniel Boelzle [:dbo]]

(In reply to comment #17)
> 1. I have a bunch of events in my home PC that I would like to copy to my
> wife's/parent's/work PC for when I'm there, so I at least have all my calendar
> events up until whenever I copied them (or I do it every time I go to and use
> that other PC).
The wording "local" is directly derived out the calendar wizard's wording.

> 2. I need to easily and quickly (because I do it infrequently enough to forget
> some un-intuitive name, but often enough to not want to have to look it up
> every time) copy (hopefully only) one file to my USB stick.
IMHO a plain ics file suits better for this purpose and is readable by other apps, too.

I still disagree prefixing each and every file with "calendar_", "cal_" or whatever since all those files are by intent hosted in a folder called "calendar-data".

Comment 20

[Bas van den Bosch]

Does doing a system-restore on windows still work when the file is called *.sqlite? Quite some people have gotten their data back, have restored their corrupt files, by doing a system-restore. See also bug 458621 comment 8 and bug 361800. If for some reason system-restore isn't working anymore, we should really keep backup-copy's of the data in ics-files.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #18)
> Huh? Mvl, where do you see that in the patch?
> I am only migrating tables from |sqlTables|, leaving any other untouched.

Yes, you are right. Sorry, I didn't look close enough. I Never mind my comment.

Comment 22

[Daniel Boelzle [:dbo]]

(In reply to comment #20)
> Does doing a system-restore on windows still work when the file is called
> *.sqlite? Quite some people have gotten their data back, have restored their
> corrupt files, by doing a system-restore. See also bug 458621 comment 8 and bug
> 361800. If for some reason system-restore isn't working anymore, we should
> really keep backup-copy's of the data in ics-files.

Getting rid of the sdb suffix is by intent, *.sqlite files are not part of system restore. Taking it out of system restore is IMHO OK since the rest of your profile isn't part of it anyway (consider bug 403006 in this context). Thus (also w.r.t. bug 438369) I think the right way to go is offering a smart and complete profile backup facility unless the system already offers one like apple's time machine. Anything else leads to partly, but incomplete backups.

Comment 23

[Bas van den Bosch]

If the SB/Lightning gets pref-driven and things like schemes are taken out of local.sqlite then there's no harm I guess, calendar-data doesn't get corrupted. But bug 438369 and bug 403006 should be fixed before releasing 1.0 (or even before this bug gets fixed). We have to prevent dataloss for those using nightlies or previous 0.x releases.

Comment 24

[Tony Mechelynck [:tonymec]]

(In reply to comment #19)
[...]
> I still disagree prefixing each and every file with "calendar_", "cal_" or
> whatever since all those files are by intent hosted in a folder called
> "calendar-data".

We are using the word "calendar" with two different meanings here: OT1H we have the Calendar application (as in Lightning or Sunbird), OTOH we have calendars (the things we pin up on the wall, stand on our desks, fold in the form of agendas in our pockets -- or store on a computer by means of the Calendar application). The calendar-data folder marks its contents as belonging to the Calendar application. Inside it there is e.g. cache.sqlite -- cache data for the Calendar application. If we want to store the Calendar application's calendars, IMHO the name calendar-data/calendar.sqlite is justified.

Comment 25

[Tony Mechelynck [:tonymec]]

Setting dependencies, see comment #23.

Comment 26

[Daniel Boelzle [:dbo]]

(In reply to comment #25)
> Setting dependencies, see comment #23.
I disagree, see my comment #22. Incomplete backups hardly help, but profile backups are IMHO what we need here.

Comment 27

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

While using system restore to restore calendar data was at times a "nice bug" it has also caused pain for other users. System restore is not something meant to restore user data.

About backups, I don't yet see why this should be a core part of Calendar and should have so high priority. If you need a backup solution then feel free to try the automatic export extension. Taking a look at other applications its also not a very common pattern. Neither firefox nor thunderbird has core mechanisms to automatically backup bookmarks or the local folder emails. This can be done with extensions like weave and thunderbird probably also has a backup solution as an extension. Therefore I don't understand why this should be a goal for 1.0.

Aside from that, do you really think the name of the profile calendar really is that important? I mean sure, its nice to know what each file does in your profile, but if you are advanced enough to rummage around in the profile, then you'll easily understand that if you want to copy calendar data to another computer, you'll need the whole calendar-data folder. You'll also be witty enough to understand that local.sqlite is a local database and probably holds some data local to your computer and might even assume that its the local calendars. This of course assumes that bug 403006 is fixed before 1.0 which I do think will be the case.

Comment 28

[Peter Lairo]

(In reply to comment #27)

> Aside from that, do you really think the name of the profile calendar really is
> that important? 

Yes. Details (can) matter. And the benefit is greater than the cost.

> I mean sure, its nice to know what each file does in your
> profile, but if you are advanced enough to rummage around in the profile, then
> you'll easily understand that if you want to copy calendar data to another
> computer, you'll need the whole calendar-data folder. You'll also be witty
> enough to understand that local.sqlite is a local database and probably holds
> some data local to your computer and might even assume that its the local
> calendars. 

I disagree. That's "Linux logic", not "Mac user-friendliness". ;-)

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #28)
> 
> I disagree. That's "Linux logic", not "Mac user-friendliness". ;-)

Well, Mac user-friendliness also means don't rummage around in your profile :-)

I could live with calendar-data/localcal.sqlite though.

Comment 30

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 355392 [details] [diff] [review]
patch - v1

>+                this.mDB.executeSimpleSQL("ATTACH DATABASE '" + localDB.databaseFile.path + "' AS local_sqlite");
What about escaping here? i.e what happens if the user installs in a folder like "/Software that starts with 'S'" ?


>+                    // hold lock on storage.sdb until we've migrated data from storage.sdb:
>+                    this.mDB.beginTransactionAs(Components.interfaces.mozIStorageConnection.TRANSACTION_EXCLUSIVE);
I hope we don't run into lock conditions here if the db is accessed more than once?


r=philipp

Comment 31

[Robert Kaiser]

(In reply to comment #30)
> (From update of attachment 355392 [details] [diff] [review])
> >+                this.mDB.executeSimpleSQL("ATTACH DATABASE '" + localDB.databaseFile.path + "' AS local_sqlite");
> What about escaping here? i.e what happens if the user installs in a folder
> like "/Software that starts with 'S'" ?

or "/db; SELECT pwd FROM mytable" :p
(one never knows where/how people might inject SQL)

Comment 32

[Daniel Boelzle [:dbo]]

Attachment: patch - v2

using a statement to auto-escape file path

carrying forward r+

Comment 33

[Daniel Boelzle [:dbo]]

Thanks to Andreas pretesting the patch!

Comment 34

[Daniel Boelzle [:dbo]]

Philipp, mvl and I opted to go with "local.sqlite".

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/daa8b76471bf>

-> FIXED

Comment 35

[Stefan Sitter [:ssitter]]

I see one issue here that should be fixed: 

Opening an existing profile from 0.9 in 1.0pre moves all the tables to the new database file local.sqlite. 

Opening the profile in a previous build like 0.9 afterwards detects that storage.sdb is empty an creates the default calendar and database tables. For the user it seems that all the data was lost. 

Opening the profile again in 1.0pre fails because it tries to recreate local.sqlite that already exists. The calendars can not be accessed.

A solution might be to keep/create the version entries in storage.sdb to ensure that previous versions can't open and abort with the 'upgraded by newer version' dialog.

Or the upgrade code must ensure that the transmission to local.sqlite happens only once.

Comment 36

[Daniel Boelzle [:dbo]]

Attachment: prohibit downgrades

This patch should prohibit downgrading.

Comment 37

[Daniel Boelzle [:dbo]]

Attachment: prohibit downgrades - v2

This fixes prohibiting a downgrade from a brand-new profile (without any tables installed in the first place); asking for further QA.

Comment 38

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 359288 [details] [diff] [review]
prohibit downgrades - v2

>                 let version = this.getSchemaVersion(db);
>                 //cal.LOG("*** Calendar schema version is: " + version);
remove comment while you are here

>                 if (version < DB_SCHEMA_VERSION) {
>                     this.upgradeDB(version, db);
>-                } else if (version > DB_SCHEMA_VERSION) {
>-                    db.rollbackTransaction();
>-                    // Schema version is newer than what we know how to deal with.
>-                    // Alert the user, and quit the app.
>-                    this.alertAndQuit();
>-                    return;
Don't quite understand why we can remove this, please elaborate.


codewise this looks fine, r=philipp

Comment 39

[Daniel Boelzle [:dbo]]

It's obsolete code, because there's no newer schema anymore once migrated to the prefs (abscense of cal_calendar_prefs signals this). I'd appreciate some more QA before checking in.

Comment 40

[Stefan Sitter [:ssitter]]

Comment on attachment 359288 [details] [diff] [review]
prohibit downgrades - v2

I did some basic testing with the new patch:

[*] upgrade from 0.3.1 and earlier to 1.0pre still fails - bug 470430

[*] upgrade from 0.5/0.7./0.8/0.9 to 1.0pre worked for me - OK

[*] opening upgraded profile with 0.5/0.7 is possible but no calendars are created/available. opening in 1.0pre again works now without errors - OK

[*] opening upgraded profile with 0.8/0.9 is rejected - OK

[*] upgrade to 1.0pre, manually remove storage.sdb, open in 0.9 (creates storage.sdb), open in 1.0pre, upgrade attempt fails - very unusual workflow, probably not worth fixing. can be workaround by removing storage.sdb before opening 1.0pre again.

Comment 41

[Daniel Boelzle [:dbo]]

Thanks for testing, Stefan!

Comment 42

[Daniel Boelzle [:dbo]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/174fa863d99c>

-> FIXED