bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Duplicate indexes in browser.db

RESOLVED FIXED in Firefox 46

Status

()

Firefox for Android
Data Providers
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mfinkle, Assigned: vjoshi, Mentored)

Tracking

(Blocks: 1 bug, {perf})

unspecified
Firefox 46
All
Android
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(1 attachment, 7 obsolete attachments)

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
OS: Mac OS X → Android
Hardware: x86 → All
Also slows down inserts.
Blocks: 942609
Mentor: margaret.leibovic, rnewman
Keywords: perf
Whiteboard: [good
(Yay Chrome autocomplete.)
Whiteboard: [good → [good first bug][lang=java]

Comment 3

4 years ago
Hey,

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

Thank you
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

4 years ago
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 :)
Flags: needinfo?(rnewman)

Comment 6

4 years ago
Sorry for the flag.

Thank you again
Flags: needinfo?(rnewman)

Comment 7

4 years ago
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 :)
Flags: needinfo?(rnewman)

Comment 8

4 years ago
(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
Flags: needinfo?(mark.finkle)

Updated

4 years ago
Flags: needinfo?(mark.finkle)
(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
Flags: needinfo?(rnewman)

Comment 10

4 years ago
Created attachment 8565081 [details] [diff] [review]
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,
Attachment #8565081 - Flags: feedback?(rnewman)
Assignee: nobody → ronak9896
Assignee: ronak9896 → nobody
(Assignee)

Comment 11

3 years ago
Created attachment 8694761 [details] [diff] [review]
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.
Attachment #8694761 - Flags: review?(rnewman)
Attachment #8694761 - Flags: review?(margaret.leibovic)
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?
Attachment #8694761 - Flags: review?(rnewman)
Attachment #8694761 - Flags: review?(margaret.leibovic)
Attachment #8694761 - Flags: feedback+
(Assignee)

Comment 13

3 years ago
Created attachment 8695712 [details] [diff] [review]
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
Attachment #8694761 - Attachment is obsolete: true
Attachment #8695712 - Flags: review?(rnewman)
(Assignee)

Comment 14

3 years ago
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?
(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.
Assignee: nobody → varunj.1011
Status: NEW → ASSIGNED
(Assignee)

Comment 16

3 years ago
(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 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.
Attachment #8695712 - Flags: review?(rnewman) → review+
(Assignee)

Comment 18

3 years ago
Created attachment 8698516 [details] [diff] [review]
removeDuplicateIndices.patch

Added IF EXISTS to DROP INDEX statements
Attachment #8695712 - Attachment is obsolete: true
Attachment #8698516 - Flags: review?(rnewman)
Comment on attachment 8698516 [details] [diff] [review]
removeDuplicateIndices.patch

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

Looks good!
Attachment #8698516 - Flags: review?(rnewman) → review+
Okay Varun, can you push this to Try so we can set checkin-needed?
(Assignee)

Comment 21

3 years ago
Sure! but I don't have access. How do I go about applying for it?
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
(Assignee)

Comment 23

3 years ago
Created attachment 8698998 [details] [diff] [review]
removeDuplicateIndices.patch

Changed location of files to from mobile/base/db to mobile/base/java/org/mozilla/gecko/db
Attachment #8698516 - Attachment is obsolete: true
Attachment #8698998 - Flags: review?(rnewman)
Attachment #8565081 - Attachment is obsolete: true
Attachment #8565081 - Flags: feedback?(rnewman)
Varun, you lost your changes to TabsProvider.java and URLMetadataTable.java.
Flags: needinfo?(varunj.1011)
(Assignee)

Comment 26

3 years ago
(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.
Flags: needinfo?(varunj.1011)
(Assignee)

Comment 27

3 years ago
Created attachment 8699113 [details] [diff] [review]
removeDuplicateIndices.patch

Reinstated lost changes to URLMetadataTable.java and TabsProvider.java
Attachment #8698998 - Attachment is obsolete: true
Attachment #8698998 - Flags: review?(rnewman)
Attachment #8699113 - Flags: review?(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.
Attachment #8699113 - Flags: review?(rnewman) → review+
(Assignee)

Comment 31

3 years ago
Created attachment 8699313 [details] [diff] [review]
removeDuplicateIndices.patch

Removed trailing whitespaces
Attachment #8699113 - Attachment is obsolete: true
Attachment #8699313 - Flags: review?(rnewman)
(Assignee)

Comment 32

3 years ago
Created attachment 8699323 [details] [diff] [review]
removeDuplicateIndices.patch

A couple of whitespaces had been left behind, removing those
Attachment #8699313 - Attachment is obsolete: true
Attachment #8699313 - Flags: review?(rnewman)
Attachment #8699323 - Flags: review?(rnewman)

Comment 33

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e76cb77eb86
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(Assignee)

Comment 34

3 years ago
@rnewman: Could you let me know if there are more bugs where I could help?
Flags: needinfo?(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?
Flags: needinfo?(rnewman)
Flags: needinfo?(nalexander)
Flags: needinfo?(michael.l.comella)
Attachment #8699323 - Flags: review?(rnewman) → review+
(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.
Flags: needinfo?(nalexander)
(Assignee)

Comment 37

3 years ago
(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.
I'll likely have more front-end Android work so perhaps bug 1232861 or more simply bug 1227321?
Flags: needinfo?(michael.l.comella)
You need to log in before you can comment on or make changes to this bug.