1128675 - Duplicate indexes in browser.db

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

I was snooping around browser.db and noticed that we create some indexes and SQLite auto-creates very similar ones.

For example, 
favicons_url_index and sqlite_autoindex_favicons_1 (same except for 'allow dupes')
metadata_url_idx and sqlite_autoindex_metadata_1 (same except for 'allow dupes')
reading_list_guid and sqlite_autoindex_reading_list_1 (same)

I assume having unused indexes takes up space.

http://stackoverflow.com/questions/14590956/sqlite-and-autoindexing
http://stackoverflow.com/questions/3379292/is-an-index-needed-for-a-primary-key-in-sqlite

Comment 1

[Richard Newman [:rnewman]]

Also slows down inserts.

Comment 2

[Richard Newman [:rnewman]]

(Yay Chrome autocomplete.)

Comment 3

[ronak khandelwal]

Hey,

I'd like to work on this bug . Can you tell me just a bit more about this.

Thank you

Comment 4

[Richard Newman [:rnewman]]

SQLite creates indexes for any column marked as UNIQUE, and in some other situations.

Resolving this bug will consist of:

* Figuring out which indices might be redundant. mfinkle's Comment 0 is a good start. You might need to grab a browser.db from a device and poke around in sqlite3 on a desktop machine. You might want to use Margaret's "Copy Profile" add-on to get your DB off a device.

* Determining if similar indices are important or not. This will require some understanding of query planning.

* Implementing a database migration and altering table creation to avoid creating duplicate indices.

Comment 5

[ronak khandelwal]

Hey Richard,

Sounds Fun. I'll get right onto it ,I might have some basic understanding of query planning.I'll work on it and if I have a problem I'll ping you :)

Comment 6

[ronak khandelwal]

Sorry for the flag.

Thank you again

Comment 7

[ronak khandelwal]

Hey,

I did some research and found that we have some indices which have been automatically created in 3 realtion mentioned by mark + thumbnails , search history . From what I have learned these automatically created indices usually point to the primary key or the unique attribute . In these cases the custom created indices are more than enough for search and display queries where mostly searched contents will be in url or text etc whose indices are already there so basically these autoindices are clogging up space and in a really big database these will also affect the input for small ones not noticable.Now my question is how should I submit a patch should i upload the new browser.db file or copy it in the mozilla-central directory and then create a patch and then upload it???

Thank you,


p.s: Do tell me if I'm wrong anywhere and sorry it took me long I had to learn everthing :)

Comment 8

[ronak khandelwal]

(In reply to Mark Finkle (:mfinkle) from comment #0)
 
> favicons_url_index and sqlite_autoindex_favicons_1 (same except for 'allow
> dupes')
> metadata_url_idx and sqlite_autoindex_metadata_1 (same except for 'allow
> dupes')
> reading_list_guid and sqlite_autoindex_reading_list_1 (same)
> 


I think that these indices are pointing at diffrent columns "Implicit indexes are indexes that are automatically created by the database server when an object is created. Indexes are automatically created for primary key constraints and unique constraints." The custom index are pointing to url/guid none of them is the primary key.


Thank you

Comment 9

[Richard Newman [:rnewman]]

(In reply to ronak khandelwal from comment #7)
> Now my question is how should I submit a patch
> should i upload the new browser.db file or copy it in the mozilla-central
> directory and then create a patch and then upload it???

You'll need to make a change to BrowserDatabaseHelper.java, as part of a database migration. That'll involve:

* Not creating a duplicate index when creating the relevant tables.
* Dropping the duplicate indices in a migration.
* Bumping the DB schema version.

You'll want to submit Mercurial changes for review, not a modified database.

Check out the docs here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

Comment 10

[ronak khandelwal]

Attachment: Bug1128675.diff

Hey,

Well I read https://www.sqlite.org/autoinc.html and realized that we don't need auto increment and this is causng the automatic indices creation(?). And if we have an integer primary key it will always increment the value just one after the last one. 

Thank you,

Comment 11

[Varun Joshi (:vjoshi)]

Attachment: removeDuplicateIndices.patch

Removed:
tabs_guid_index -> works like sqlite_autoindex_tabs_1
thumbnails_url_index -> works like sqlite_autoindex_thumbnails_1
favicons_url_index -> works like sqlite_autoindex_favicons_1
metadata_url_idx -> works like sqlite_autoindex_metadata_1

reading_list_guid seems to have been removed in an earlier patch, I couldn't find it in my version of the database.

I'm not 100% sure whether the indices I removed were completely redundant, so it would be helpful if someone would review the query planning.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8694761 [details] [diff] [review]
removeDuplicateIndices.patch

Review of attachment 8694761 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Just some safety-related advice.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +38,5 @@
>  
>  final class BrowserDatabaseHelper extends SQLiteOpenHelper {
>      private static final String LOGTAG = "GeckoBrowserDBHelper";
>  
> +    public static final int DATABASE_VERSION = 26;

Careful that this doesn't race with Bug 1102924. Might be worth phrasing this as 

   … = 26;     // Bug 1128675.

so that you'll get a merge conflict when rebasing.

@@ +1024,5 @@
>      }
>  
> +    private void upgradeDatabaseFrom25to26(SQLiteDatabase db) {
> +        debug("Dropping unnecessary indices");
> +        db.execSQL("DROP INDEX " + TabsProvider.INDEX_CLIENTS_GUID);

Remove this line:

    static final String INDEX_CLIENTS_GUID = "clients_guid_index";

from TabsProvider, and just use the string here. We shouldn't keep the constant around for an index that no longer exists, and inlining the current value removes the possibility that someone changes that constant and breaks an untested migration.

::: mobile/android/base/db/URLMetadataTable.java
@@ -43,5 @@
>              TILE_IMAGE_URL_COLUMN + " STRING, " +
>              TILE_COLOR_COLUMN + " STRING);";
>          db.execSQL(create);
> -
> -        db.execSQL("CREATE INDEX metadata_url_idx ON " + TABLE + " (" + URL_COLUMN + ")");

Why no migration to remove this?

Comment 13

[Varun Joshi (:vjoshi)]

Attachment: removeDuplicateIndices.patch

Added a comment next to database version to prevent races.
Removed INDEX_CLIENTS_GUID from TabsProvider
Moved the statement for dropping metadata_url_idx from the migration method in BrowserDatabaseHelper to URLMetadataTable's onUpgrade method

Comment 14

[Varun Joshi (:vjoshi)]

Thanks for reviewing my patch! The new one resolves the concerns you pointed out, but I had one doubt:

> > -        db.execSQL("CREATE INDEX metadata_url_idx ON " + TABLE + " (" + URL_COLUMN + ")");
> 
> Why no migration to remove this?

Should I place the statement that drops metadata_url_idx in the upgradeDatabaseFrom25to26 method in BrowserDatabaseHelper or should I place it in URLMetadataTable's onUpgrade method?

Comment 15

[Richard Newman [:rnewman]]

(In reply to Varun Joshi from comment #14)

> Should I place the statement that drops metadata_url_idx in the
> upgradeDatabaseFrom25to26 method in BrowserDatabaseHelper or should I place
> it in URLMetadataTable's onUpgrade method?

Sorry for the delayed reply.

Please put it in URLMetadataTable. The version number is managed by BrowserDatabaseHelper, so you shouldn't need to add anything apart from the drop in onUpgrade.

Comment 16

[Varun Joshi (:vjoshi)]

(In reply to Richard Newman [:rnewman] from comment #15)
> (In reply to Varun Joshi from comment #14)
> 
> > Should I place the statement that drops metadata_url_idx in the
> > upgradeDatabaseFrom25to26 method in BrowserDatabaseHelper or should I place
> > it in URLMetadataTable's onUpgrade method?
> 
> Sorry for the delayed reply.
> 
> Please put it in URLMetadataTable. The version number is managed by
> BrowserDatabaseHelper, so you shouldn't need to add anything apart from the
> drop in onUpgrade.

Sorry for the late reply.

If you check out the updated patch, that's what I've done. I saw that BrowserDatabaseHelper's onUpgrade calls URLMetadataTable's onUpgrade, so I've just modified it to drop the index.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8695712 [details] [diff] [review]
removeDuplicateIndices.patch

Review of attachment 8695712 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with that change *and appropriate testing*. Let's make sure the upgrade path works!

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +1027,5 @@
> +        debug("Dropping unnecessary indices");
> +        db.execSQL("DROP INDEX clients_guid_index");
> +        db.execSQL("DROP INDEX thumbnails_url_index");
> +        db.execSQL("DROP INDEX favicons_url_index");
> +    }

For safety, I'd make these IF EXISTS, too.

::: mobile/android/base/db/URLMetadataTable.java
@@ +53,5 @@
>          }
> +
> +        // Removed the redundant metadata_url_idx index in version 26
> +        if (newVersion >= 26 && oldVersion < 26) {
> +            db.execSQL("DROP INDEX metadata_url_idx");

This needs to be DROP INDEX IF EXISTS, otherwise upgrades from versions earlier than 21 will fail.

Comment 18

[Varun Joshi (:vjoshi)]

Attachment: removeDuplicateIndices.patch

Added IF EXISTS to DROP INDEX statements

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 8698516 [details] [diff] [review]
removeDuplicateIndices.patch

Review of attachment 8698516 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 20

[Richard Newman [:rnewman]]

Okay Varun, can you push this to Try so we can set checkin-needed?

Comment 21

[Varun Joshi (:vjoshi)]

Sure! but I don't have access. How do I go about applying for it?

Comment 22

[Richard Newman [:rnewman]]

You'll need to apply for Level 1 access:

https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

CC me on the bug and I'll vouch for you.

Then you can follow the instructions here:

https://wiki.mozilla.org/Try

Comment 23

[Varun Joshi (:vjoshi)]

Attachment: removeDuplicateIndices.patch

Changed location of files to from mobile/base/db to mobile/base/java/org/mozilla/gecko/db

Comment 24

[Varun Joshi (:vjoshi)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7d5640730c

Comment 25

[Richard Newman [:rnewman]]

Varun, you lost your changes to TabsProvider.java and URLMetadataTable.java.

Comment 26

[Varun Joshi (:vjoshi)]

(In reply to Richard Newman [:rnewman] from comment #25)
> Varun, you lost your changes to TabsProvider.java and URLMetadataTable.java.

Oops! I'll correct this right away.

Comment 27

[Varun Joshi (:vjoshi)]

Attachment: removeDuplicateIndices.patch

Reinstated lost changes to URLMetadataTable.java and TabsProvider.java

Comment 28

[Varun Joshi (:vjoshi)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6e6f53522f2

Comment 29

[Richard Newman [:rnewman]]

Comment on attachment 8699113 [details] [diff] [review]
removeDuplicateIndices.patch

Review of attachment 8699113 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but this version of the patch introduces some trailing whitespace -- click the Review link and see the red sections.

This is good to land with that trailing whitespace removed.

Comment 30

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/6e76cb77eb861edcb133fdfa5bae0184731c0f9a
Bug 1128675 - Duplicate indexes in browser.db. r=rnewman

Comment 31

[Varun Joshi (:vjoshi)]

Attachment: removeDuplicateIndices.patch

Removed trailing whitespaces

Comment 32

[Varun Joshi (:vjoshi)]

Attachment: removeDuplicateIndices.patch

A couple of whitespaces had been left behind, removing those

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e76cb77eb86

Comment 34

[Varun Joshi (:vjoshi)]

@rnewman: Could you let me know if there are more bugs where I could help?

Comment 35

[Richard Newman [:rnewman]]

Comment on attachment 8699323 [details] [diff] [review]
removeDuplicateIndices.patch

Michael and Nick are much more aware of the set of Android bugs right now; folks, any thoughts on a follow-up bug for Varun?

Comment 36

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #35)
> Comment on attachment 8699323 [details] [diff] [review]
> removeDuplicateIndices.patch
> 
> Michael and Nick are much more aware of the set of Android bugs right now;
> folks, any thoughts on a follow-up bug for Varun?

Varun,

I have a couple of non-trivial Sync tickets if you're interested.

https://bugzilla.mozilla.org/show_bug.cgi?id=802749 is an old one, and much of the discussion is out-dated, but the feature request remains.

https://bugzilla.mozilla.org/show_bug.cgi?id=1182206 is a non-trivial clean-up ticket.

Neither of these are really straight forward; let's wait for mcomella to see if he has a well-defined ticket, and if not, I can clarify what has to happen for these tickets.

Comment 37

[Varun Joshi (:vjoshi)]

(In reply to Nick Alexander :nalexander from comment #36)
> (In reply to Richard Newman [:rnewman] from comment #35)
> > Comment on attachment 8699323 [details] [diff] [review]
> > removeDuplicateIndices.patch
> > 
> > Michael and Nick are much more aware of the set of Android bugs right now;
> > folks, any thoughts on a follow-up bug for Varun?
> 
> Varun,
> 
> I have a couple of non-trivial Sync tickets if you're interested.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=802749 is an old one, and much
> of the discussion is out-dated, but the feature request remains.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1182206 is a non-trivial
> clean-up ticket.
> 
> Neither of these are really straight forward; let's wait for mcomella to see
> if he has a well-defined ticket, and if not, I can clarify what has to
> happen for these tickets.

Both of these seem really interesting, and if mcomella doesn't have anything more suited to me, I'd love to work on them.

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I'll likely have more front-end Android work so perhaps bug 1232861 or more simply bug 1227321?