Closed Bug 562756 Opened 13 years ago Closed 12 years ago

Use transactions and exclusive database locking to improve database performance

Categories

(Toolkit :: Add-ons Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- alpha5+

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Whiteboard: [rewrite])

Attachments

(1 file, 1 obsolete file)

34.90 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
Mak has suggested using openUnsharedConnection, using createAsyncStatement and adding primary keys to all tables.
you could also evaluate (perf & protection)
PRAGMA locking_mode = EXCLUSIVE

Places also uses PRAGMA journal_mode = TRUNCATE, without this one we had a Ts hit on Linux due to fsyncs (but I don't have numbers to tell if this has been fixed in SQLite in the meantime).

Your createSchema should be wrapped in a transaction.

Notice that according to SQLite docs our getResultByName will only work reliably if columns are aliased, otherwise names are not guaranteed
http://www.sqlite.org/c3ref/column_name.html
I just discovered this yesterday, i'll add a note to the Storage idl in Bug 562787.
Thus as a form of protection you should add an AS columnname to all of your select fields (yeah that sux)
Blocks: 563006
Attached patch patch pt 1 rev 1 (obsolete) — Splinter Review
This is the first round of improvements particularly focusing on logging any errors that occur, wrapping multiple operations into transactions and switching to exclusive access to the database. It doesn't yet do anything clever with rolling back after failures but I'd like to get this in now anyway as it should significantly fix startup time issues on migration.

Rob you reviewed this in bug 567319 comment 5, I just wanted to check you're still ok with it in this context?

Shawn, Rob wanted you to look over the code I have in shutdownManager that waits until the DB is unlocked to check it looks ok.
Attachment #447341 - Flags: review?(sdwilsh)
Attachment #447341 - Flags: review?(robert.bugzilla)
Comment on attachment 447341 [details] [diff] [review]
patch pt 1 rev 1

Looks good to me but as you noted I'd like to get sdwish to verify this is the best way to verify the DB is unloacked, etc. Wouldn't hurt to have him look at the rest in case it could be improved.
Attachment #447341 - Flags: review?(robert.bugzilla) → review+
(In reply to comment #3)
> (From update of attachment 447341 [details] [diff] [review])
> Looks good to me but as you noted I'd like to get sdwish to verify this is the
s/sdwish/sdwilsh/ - I really shouldn't do reviews, etc. when home sick
Comment on attachment 447341 [details] [diff] [review]
patch pt 1 rev 1

For review comments with more context, please see http://reviews.visophyte.org/r/447341/

on file: toolkit/mozapps/extensions/XPIProvider.jsm line 1546
>     // We have to hold the DB scheme in prefs so we don't need to load the
>     // database to see if we need to migrate data
>     let schema = Prefs.getIntPref(PREF_DB_SCHEMA, 0);

This is cool but sadly I don't think we can use it elsewhere :(


on file: toolkit/mozapps/extensions/XPIProvider.jsm line 1552
>     if (schema != DB_SCHEMA) {
>       // The schema has changed so migrate data from the old schema
>       migrateData = XPIDatabase.migrateData(schema);
>     }
>     else {
>       // If the database exists then the previous file cache can be trusted
>       // otherwise create an empty database
>       let db = FileUtils.getFile(KEY_PROFILEDIR, [FILE_DATABASE], true);
>       if (db.exists()) {
>         cache = Prefs.getCharPref(PREF_INSTALL_CACHE, null);
>       }
>       else {
>         LOG("Database is missing, recreating");
>         XPIDatabase.openConnection();
>         XPIDatabase.createSchema();
>       }
>     }
> 
>     XPIDatabase.beginTransaction();

You really want to wrap the creation of the schema and migration in a
transaction as well.  Probably the same one.


on file: toolkit/mozapps/extensions/XPIProvider.jsm line 1596
>       XPIDatabase.commitTransaction();
>       return true;
>     }
> 
>     LOG("No changes found");
>     XPIDatabase.commitTransaction();

Generally speaking, you really want to have all the code that runs inside a
transaction wrapped in a try catch.  Inside the try block you should try to
commit the transaction, and in the catch, you want to rollback.  Otherwise you
could end up with a transaction being open during the course of the entire
application run.


on file: toolkit/mozapps/extensions/XPIProvider.jsm line 2433
>    * Begins a new transaction in the database. Transactions may be nested. Data
>    * written by an inner transaction may be rolled back on its own. Rolling back
>    * an outer transaction will rollback all the changes made by inner
>    * transactions even if they were committed. No data is written to the disk
>    * until the outermost transaction is committed. Transactions can be started
>    * even when the database is not yet open in which case they will be started
>    * when the database is first opened.

I suppose you don't have any tests that use nested transactions, do you? 
Savepoints need to have a unique name, and you always use 'default' so once
you try to nest, I'd expect it to error.  You can make this unique by
appending a number to the name (perhaps the transaction count?)


on file: toolkit/mozapps/extensions/XPIProvider.jsm line 2443
>       this.connection.executeSimpleSQL("SAVEPOINT 'default'");

oh cool, you are using savepoints! (we are going to use them in indexedDB too)


on file: toolkit/mozapps/extensions/XPIProvider.jsm line 2492
>     // Begin any pending transactions
>     for (let i = 0; i < this.transactionCount; i++)
>       this.connection.executeSimpleSQL("SAVEPOINT 'default'");
>     return this.connection;

I do not understand why this is here.
Attachment #447341 - Flags: review?(sdwilsh) → review-
(In reply to comment #5)
> (From update of attachment 447341 [details] [diff] [review])
> on file: toolkit/mozapps/extensions/XPIProvider.jsm line 1552
> >     if (schema != DB_SCHEMA) {
> >       // The schema has changed so migrate data from the old schema
> >       migrateData = XPIDatabase.migrateData(schema);
> >     }
> >     else {
> >       // If the database exists then the previous file cache can be trusted
> >       // otherwise create an empty database
> >       let db = FileUtils.getFile(KEY_PROFILEDIR, [FILE_DATABASE], true);
> >       if (db.exists()) {
> >         cache = Prefs.getCharPref(PREF_INSTALL_CACHE, null);
> >       }
> >       else {
> >         LOG("Database is missing, recreating");
> >         XPIDatabase.openConnection();
> >         XPIDatabase.createSchema();
> >       }
> >     }
> > 
> >     XPIDatabase.beginTransaction();
> 
> You really want to wrap the creation of the schema and migration in a
> transaction as well.  Probably the same one.

The schema creation is already wrapped in its own transaction. I guess I could put it in the same one though.

> on file: toolkit/mozapps/extensions/XPIProvider.jsm line 1596
> >       XPIDatabase.commitTransaction();
> >       return true;
> >     }
> > 
> >     LOG("No changes found");
> >     XPIDatabase.commitTransaction();
> 
> Generally speaking, you really want to have all the code that runs inside a
> transaction wrapped in a try catch.  Inside the try block you should try to
> commit the transaction, and in the catch, you want to rollback.  Otherwise you
> could end up with a transaction being open during the course of the entire
> application run.

I want to look at improving the error handling separately if possible since I know Rob doesn't generally like large try blocks and it is a priority to fix the migration speed right now. Currently if anything fails everything will get rolled back on shutdown.

> on file: toolkit/mozapps/extensions/XPIProvider.jsm line 2433
> >    * Begins a new transaction in the database. Transactions may be nested. Data
> >    * written by an inner transaction may be rolled back on its own. Rolling back
> >    * an outer transaction will rollback all the changes made by inner
> >    * transactions even if they were committed. No data is written to the disk
> >    * until the outermost transaction is committed. Transactions can be started
> >    * even when the database is not yet open in which case they will be started
> >    * when the database is first opened.
> 
> I suppose you don't have any tests that use nested transactions, do you? 
> Savepoints need to have a unique name, and you always use 'default' so once
> you try to nest, I'd expect it to error.  You can make this unique by
> appending a number to the name (perhaps the transaction count?)

Savepoints do not have to have a unique name, from the docs: "The SAVEPOINT command starts a new transaction with a name. The transaction names need not be unique". And yes this is tested, addAddonMetadata includes its own transaction and is called in the middle of the startup transaction.

> on file: toolkit/mozapps/extensions/XPIProvider.jsm line 2492
> >     // Begin any pending transactions
> >     for (let i = 0; i < this.transactionCount; i++)
> >       this.connection.executeSimpleSQL("SAVEPOINT 'default'");
> >     return this.connection;
> 
> I do not understand why this is here.

This allows us to open and close transactions before the database has been opened. This saves us from having to track whether we needed the database during startup in multiple places. Instead we can just wrap all of startup in a fake transaction that will become real if we ever actually open the database.
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 447341 [details] [diff] [review] [details])
>...
> I want to look at improving the error handling separately if possible since I
> know Rob doesn't generally like large try blocks and it is a priority to fix
> the migration speed right now. Currently if anything fails everything will get
> rolled back on shutdown.
I'm fine with a large try when they don't obscure what their actual purpose is. In this instance a comment at the beginning of the try would suffice.
(In reply to comment #6)
> The schema creation is already wrapped in its own transaction. I guess I could
> put it in the same one though.
For performance reasons, you'll want to.

> I want to look at improving the error handling separately if possible since I
> know Rob doesn't generally like large try blocks and it is a priority to fix
> the migration speed right now. Currently if anything fails everything will get
> rolled back on shutdown.
Right, but any changes during the runtime of the application would be rolled back too, right?  That seems like it's not desirable unless you shutdown right away.

> Savepoints do not have to have a unique name, from the docs: "The SAVEPOINT
> command starts a new transaction with a name. The transaction names need not be
> unique". And yes this is tested, addAddonMetadata includes its own transaction
> and is called in the middle of the startup transaction.
I should have been more clear.  Everything will be rolled back, when I think you just want that small section?  Maybe you do want everything rolled back, and in which case you should maybe make that clearer.

You should also cache that statement if it is never going to change.

> This allows us to open and close transactions before the database has been
> opened. This saves us from having to track whether we needed the database
> during startup in multiple places. Instead we can just wrap all of startup in a
> fake transaction that will become real if we ever actually open the database.
That seems a little strange to me, but I don't care enough r- on that.
(In reply to comment #8)
> > Savepoints do not have to have a unique name, from the docs: "The SAVEPOINT
> > command starts a new transaction with a name. The transaction names need not be
> > unique". And yes this is tested, addAddonMetadata includes its own transaction
> > and is called in the middle of the startup transaction.
> I should have been more clear.  Everything will be rolled back, when I think
> you just want that small section?  Maybe you do want everything rolled back,
> and in which case you should maybe make that clearer.

From the sqlite docs: "The ROLLBACK command with a TO clause rolls back transactions going backwards in time back to the most recent SAVEPOINT with a matching name"

So only the most recently begun transaction would get rolled back (the same is true for release).
That was my understanding of it as well
(In reply to comment #9)
> From the sqlite docs: "The ROLLBACK command with a TO clause rolls back
> transactions going backwards in time back to the most recent SAVEPOINT with a
> matching name"
> 
> So only the most recently begun transaction would get rolled back (the same is
> true for release).
Oy, I misread that...and it's confusing.  OK, I'd like to see the try-catch block done, but r=me with that.
Comment on attachment 447341 [details] [diff] [review]
patch pt 1 rev 1

per comment 11
Attachment #447341 - Flags: review- → review+
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 447341 [details] [diff] [review] [details] [details])
> >...
> > I want to look at improving the error handling separately if possible since I
> > know Rob doesn't generally like large try blocks and it is a priority to fix
> > the migration speed right now. Currently if anything fails everything will get
> > rolled back on shutdown.
> I'm fine with a large try when they don't obscure what their actual purpose is.
> In this instance a comment at the beginning of the try would suffice.
Also, if a try block is for managing a transaction or transactions then a comment is sufficient... similar to some of the old EM code for managing files during install /  upgrade / uninstall. What I am against is over use of try blocks where the only purpose is to group a couple of lines that could throw that contains a bunch of lines that don't throw, etc
Attached patch patch rev 2Splinter Review
Addressed Shawn's comments, added try catch blocks and rollbacks and caching the statements. I've also added modified the migration path slightly so we don't need to create a transaction until just before the new database might get opened. There are also some checks for some of the cases where we can recover a little from DB failures.
Attachment #447341 - Attachment is obsolete: true
Attachment #447801 - Flags: review?(robert.bugzilla)
Attachment #447801 - Flags: review?(robert.bugzilla) → review+
Status: NEW → ASSIGNED
Going to file follow-up bugs for the other bits we may want to do here. Landed this patch: http://hg.mozilla.org/mozilla-central/rev/317fcd674ee7

There isn't really any way to test this either automatically or in litmus.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Summary: Make better use of sqlite → Use transactions and exclusive database locking to improve database performance
Target Milestone: --- → mozilla1.9.3a5
Verified fixed by check-in and a green test-run.

Bug 563006 which depends on this bug is an alpha5 blocker. We should nominate this one too, just in case we have to backout.

(In reply to comment #16)
> Filed bug 568739, bug 568740 and bug 568741

Do we have to add those bugs to the dependency list of bug 563006?
Status: RESOLVED → VERIFIED
blocking2.0: --- → ?
> (In reply to comment #16)
> > Filed bug 568739, bug 568740 and bug 568741
> 
> Do we have to add those bugs to the dependency list of bug 563006?

No, they would have little effect on bug 563006
blocking2.0: ? → alpha5+
You need to log in before you can comment on or make changes to this bug.