386303 - Duplicate indexes in nsFaviconService and nsAnnotationService

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; it; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; it; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4

nsFaviconService and nsAnnotationService create tables on the fly, but they have duplicated indexes.

example in nsFaviconService:

CREATE TABLE moz_favicons (id INTEGER PRIMARY KEY, url LONGVARCHAR UNIQUE, data BLOB, mime_type VARCHAR(32), expiration LONG)

then it creates an index with:

CREATE INDEX moz_favicons_url ON moz_favicons (url)

That index is a duplicate, there is already a UNIQUE index on url, if you open places.sqlite with sqlite explorer you can clearly see that are present 2 indexes:

moz_favicons_url, not unique, url
sqlite_autoindex_moz_favicons_1, unique, url

Having 2 indexes could slow down INSERTs because they have to be updated both, and can also slowdown SELECTs because the db engine has to benchmark and choose between the 2 indexes.

Similar problem in nsAnnotationService on table moz_anno_attributes

I attach patches to remove create index, but this is a schema change, so probably something other needs to be done if approved.

Forgive me if this is expected behaviour

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Marco Bonardo [:mak]]

Attachment: nsFaviconService remove duplicate index

Comment 2

[Marco Bonardo [:mak]]

Attachment: nsAnnotationService duplicate index

Comment 3

[(not reading, please use [e-mail] instead)]

Marco, once again, thanks!

I can confirm that we have duplicate indexes (indicies?) by doing this in the SQLite Database Browser:

PRAGMA index_info(sqlite_autoindex_moz_favicons_1)
PRAGMA index_info(moz_favicons_url)

PRAGMA index_info(sqlite_autoindex_moz_anno_attributes_1)
PRAGMA index_info(moz_anno_attributes_nameindex)

I have been looking for documentation on if SQLite will always create those autoindexes for us, and I have found http://groundwalker.com/blog/2007/05/sqlite_auto_indice.html (but nothing yet on sqlite.org)

But I think we should take Marco's patches but also add code to gracefully drop the moz_favicons_url index and the moz_anno_attributes_nameindex index if they exist.

Comment 4

[Shawn Wilsher :sdwilsh]

I think it is part of the SQL language that says that if you specify UNIQUE or PRIMARY KEY it creates an index (for the former to verify that it is in fact unique, and the latter to ensure uniqueness as well as fast searching).

Comment 5

[Marco Bonardo [:mak]]

yes it HAS to create an index for UNIQUEs, because it can't duplicate, so it has to do a quick seach for duplicates...

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: a fix based on marco's patches

Comment 7

[(not reading, please use [e-mail] instead)]

Comment on attachment 270954 [details] [diff] [review]
a fix based on marco's patches

would also gladly take a review from dietrich.

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 270954 [details] [diff] [review]
a fix based on marco's patches

after chatting with shawn, clearing review requests

Comment 9

[(not reading, please use [e-mail] instead)]

here's a summar of what shawn said:

we should stop creating the indexes (as marco did in his original patch), but we don't need to call DROP INDEX IF EXISTS every time we run.

Instead, we should move that code to migration, and bump the schema version and drop the indexes. (We should do this in places, without dropping all the other tables.)

I'm not sure I 100% agree that the schema has changed (although shawn thinks so), but I do agree that we should only call drop once.

dietrich, do you know if we have any other pending schema changes?  (possibly for thunder's guid work for sync?)  If so, lumping them together would be handy (and make things easier).

Comment 10

[Dietrich Ayala (:dietrich)]

> dietrich, do you know if we have any other pending schema changes?  (possibly
> for thunder's guid work for sync?)  If so, lumping them together would be handy
> (and make things easier).
> 

i can include it in my patch for bug 319455.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: marco's original patch

r=sspitzer on marco's original patch.  for dropping the indexes, I'll log a spin off bug so that dietrich can include it with his patch for bug #386303

Comment 12

[(not reading, please use [e-mail] instead)]

will land for marco as soon as the tree opens.

> dietrich can include it with his patch for bug #386303

oops, I meant bug #319455.

see bug #387166 about adding the DROP INDEX IF EXISTS as part of bug #319455

Comment 13

[(not reading, please use [e-mail] instead)]

fixed.

Checking in toolkit/components/places/src/nsAnnotationService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsAnnotationService.cpp,v  <--  n
sAnnotationService.cpp
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/components/places/src/nsFaviconService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsFaviconService.cpp,v  <--  nsFa
viconService.cpp
new revision: 1.13; previous revision: 1.12
done

Comment 14

[Al Billings [:abillings - ex-MoCo]]

How do I verify the fix? Run the following in the SQLite Database Browser?

PRAGMA index_info(sqlite_autoindex_moz_favicons_1)
PRAGMA index_info(moz_favicons_url)

PRAGMA index_info(sqlite_autoindex_moz_anno_attributes_1)
PRAGMA index_info(moz_anno_attributes_nameindex)

I'm not sure what I'm looking for here though... :-)

Comment 15

[Marco Bonardo [:mak]]

yes that should work, it should return infos for
PRAGMA index_info(sqlite_autoindex_moz_favicons_1)
nothing for
PRAGMA index_info(moz_favicons_url)

same for attributes... 

but this works only on a newly created profile, there is not yet the code for old index removal, it will we added in Bug 319455

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Yes, I ran it on a new profile on the 7/11 nightly trunk build and it behaved correctly.

I'm marking this as verified then.

Comment 17

[(not reading, please use [e-mail] instead)]

> I ran it on a new profile on the 7/11 nightly trunk build and it behaved
> correctly.

With or without the indexes, things would work.

To properly verify:

1)  with a build before this fix, create a new profile, see that we are creating the moz_favicons_url and 
moz_anno_attributes_nameindex indexes
2)  with a build after this fix, create a new profile, and see that we are not creating those indexes.

unverifying for now.

Comment 18

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv