562756 - Use transactions and exclusive database locking to improve database performance

Status: VERIFIED FIXED

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

Description

[Dave Townsend [:mossop]]

Mak has suggested using openUnsharedConnection, using createAsyncStatement and adding primary keys to all tables.

Comment 1

[Marco Bonardo [:mak]]

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)

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch pt 1 rev 1

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.

Comment 3

[Robert Strong (they/them - no direct email)]

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.

Comment 4

[Robert Strong (they/them - no direct email)]

(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 5

[Shawn Wilsher :sdwilsh]

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.

Comment 6

[Dave Townsend [:mossop]]

(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.

Comment 7

[Robert Strong (they/them - no direct email)]

(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.

Comment 8

[Shawn Wilsher :sdwilsh]

(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.

Comment 9

[Dave Townsend [:mossop]]

(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).

Comment 10

[Robert Strong (they/them - no direct email)]

That was my understanding of it as well

Comment 11

[Shawn Wilsher :sdwilsh]

(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 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 447341 [details] [diff] [review]
patch pt 1 rev 1

per comment 11

Comment 13

[Robert Strong (they/them - no direct email)]

(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

Comment 14

[Dave Townsend [:mossop]]

Attachment: patch rev 2

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.

Comment 15

[Dave Townsend [:mossop]]

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.

Comment 16

[Dave Townsend [:mossop]]

Filed bug 568739, bug 568740 and bug 568741

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 18

[Dave Townsend [:mossop]]

> (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