Closed Bug 328603 Opened 14 years ago Closed 11 years ago

Calendar sqlite database issues; renaming of storage.sdb

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dmose, Assigned: dbo)

References

Details

Attachments

(2 files, 2 obsolete files)

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?
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
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?
(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.
Flags: wanted-calendar1.0?
Flags: tb-integration?
Summary: calendar sqlite database issues → Calendar sqlite database issues; renaming of storage.sdb
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.
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+.
Flags: tb-integration? → tb-integration+
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+).
Flags: wanted-calendar1.0?
Flags: tb-integration?
Flags: tb-integration+
Flags: blocking-calendar1.0+
Keywords: helpwanted
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)
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)?
(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.)?
Attached patch patch - v1 (obsolete) β€” β€” Splinter Review
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!
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #355392 - Flags: review?(philipp)
(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).
Keywords: helpwantedqawanted
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.
(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.
(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).
(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 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
(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.
(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.
(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".
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.
(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.
(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.
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.
(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.
(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.
No longer depends on: 403006, 438369
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.
(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". ;-)
(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.
Attachment #355392 - Flags: review?(philipp) → review+
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
(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)
Attached patch patch - v2 β€” β€” Splinter Review
using a statement to auto-escape file path

carrying forward r+
Attachment #355392 - Attachment is obsolete: true
Attachment #359036 - Flags: review+
Thanks to Andreas pretesting the patch!
Flags: tb-integration?
Keywords: qawanted
Philipp, mvl and I opted to go with "local.sqlite".

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

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
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.
Attached patch prohibit downgrades (obsolete) β€” β€” Splinter Review
This patch should prohibit downgrading.
Attachment #359242 - Flags: review?(ssitter)
Attached patch prohibit downgrades - v2 β€” β€” Splinter Review
This fixes prohibiting a downgrade from a brand-new profile (without any tables installed in the first place); asking for further QA.
Attachment #359242 - Attachment is obsolete: true
Attachment #359288 - Flags: review?(philipp)
Attachment #359242 - Flags: review?(ssitter)
Keywords: qawanted
Attachment #359288 - Flags: review?(philipp) → review+
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
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 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.
Thanks for testing, Stefan!
Keywords: qawanted
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/174fa863d99c>

-> FIXED
Duplicate of this bug: 458621
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.