Closed
Bug 1128675
Opened 10 years ago
Closed 9 years ago
Duplicate indexes in browser.db
Categories
(Firefox for Android Graveyard :: Data Providers, defect)
Tracking
(firefox46 fixed)
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: mfinkle, Assigned: vjoshi, Mentored)
References
Details
(Keywords: perf, Whiteboard: [good first bug][lang=java])
Attachments
(1 file, 7 obsolete files)
7.94 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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
Updated•10 years ago
|
OS: Mac OS X → Android
Hardware: x86 → All
Comment 1•10 years ago
|
||
Also slows down inserts.
Comment 3•10 years ago
|
||
Hey,
I'd like to work on this bug . Can you tell me just a bit more about this.
Thank you
Comment 4•10 years ago
|
||
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•10 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 7•10 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•10 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•10 years ago
|
Flags: needinfo?(mark.finkle)
Comment 9•10 years ago
|
||
(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•10 years ago
|
||
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•9 years ago
|
||
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 12•9 years ago
|
||
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•9 years ago
|
||
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•9 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?
Comment 15•9 years ago
|
||
(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•9 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 17•9 years ago
|
||
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•9 years ago
|
||
Added IF EXISTS to DROP INDEX statements
Attachment #8695712 -
Attachment is obsolete: true
Attachment #8698516 -
Flags: review?(rnewman)
Comment 19•9 years ago
|
||
Comment on attachment 8698516 [details] [diff] [review]
removeDuplicateIndices.patch
Review of attachment 8698516 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good!
Attachment #8698516 -
Flags: review?(rnewman) → review+
Comment 20•9 years ago
|
||
Okay Varun, can you push this to Try so we can set checkin-needed?
Assignee | ||
Comment 21•9 years ago
|
||
Sure! but I don't have access. How do I go about applying for it?
Comment 22•9 years ago
|
||
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•9 years ago
|
||
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)
Assignee | ||
Comment 24•9 years ago
|
||
Updated•9 years ago
|
Attachment #8565081 -
Attachment is obsolete: true
Attachment #8565081 -
Flags: feedback?(rnewman)
Comment 25•9 years ago
|
||
Varun, you lost your changes to TabsProvider.java and URLMetadataTable.java.
Flags: needinfo?(varunj.1011)
Assignee | ||
Comment 26•9 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•9 years ago
|
||
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)
Assignee | ||
Comment 28•9 years ago
|
||
Comment 29•9 years ago
|
||
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+
Comment 30•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6e76cb77eb861edcb133fdfa5bae0184731c0f9a
Bug 1128675 - Duplicate indexes in browser.db. r=rnewman
Assignee | ||
Comment 31•9 years ago
|
||
Removed trailing whitespaces
Attachment #8699113 -
Attachment is obsolete: true
Attachment #8699313 -
Flags: review?(rnewman)
Assignee | ||
Comment 32•9 years ago
|
||
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•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Assignee | ||
Comment 34•9 years ago
|
||
@rnewman: Could you let me know if there are more bugs where I could help?
Flags: needinfo?(rnewman)
Comment 35•9 years ago
|
||
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+
Comment 36•9 years ago
|
||
(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•9 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)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•