328532 - Normalize annotation names in database

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Brett Wilson]

The annotation service currently stores names as text for each row. Since it looks increasingly like we will have relatively few numbers of annotation types and they will be applied to relatively large numbers of pages, it should be a performance and space win to separate out the names into a separate table.

Comment 1

[Brett Wilson]

Need this before a2 so DB format will be final by then.

Comment 2

[Brett Wilson]

Attachment: Patch

This patch creates a new table moz_anno_name that is used for the names of annotations. There is migration code that detects an unnormalized annotation table and moves to the new format. However, this will mean you will not be able to have an old and a new build coexisting on the same profile: the tables will be changed and the old build won't work.

Comment 3

[Brett Wilson]

Attachment: Fixed patch

The previous patch broke annotation querying. This fixes it. The only difference is a one line change in nsNavHistory.cpp

Comment 4

[Brian Ryner (not reading)]

Comment on attachment 217333 [details] [diff] [review]
Fixed patch

>--- nsAnnotationService.cpp	23 Mar 2006 06:24:54 -0000	1.11
>+++ nsAnnotationService.cpp	5 Apr 2006 20:03:34 -0000
>+  // this needs to happen after moz_anno_name gets created
>+  if (migrateFromAlpha1)
>+    return MigrateFromAlpha1(aDBConn);

Do you really want to return early here?  Seems easy to add something after this "if" and forget about the migration case skipping it.

>+//
>+//    We don't remove anything from the moz_anno_name table. If we delete the
>+//    last item of a given name, that item really should go away. It will get
>+//    cleaned up on the next shutdown.

Where does it get cleaned up at?  If it happens implicitly somehow, you should comment on that.

Looks ok otherwise.

Comment 5

[Brett Wilson]

> Do you really want to return early here?  Seems easy to add something after
> this "if" and forget about the migration case skipping it.

Good point


> Where does it get cleaned up at?  If it happens implicitly somehow, you should
> comment on that.

It gets cleaned up in nsNavHistoryExpire::EraseAnnotations in the patch for bug 328598 that you just rejected :) It currently just says "// FIXME bug 319455 expire annotations". We don't do any annotation expiration yet.

Comment 6

[Brett Wilson]

Fixed on branch and trunk.

Comment 7

[Brett Wilson]

Note that this will upgrade the database format in a way that is not supportable by previous builds. Once you run a new build, you won't be able to run builds from before this patch without all kinds of things breaking like the bookmarks system. (It won't get corrupted, it just won't work).

Comment 8

[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