449086 - Create temporary tables for frequently used places tables

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Shawn Wilsher :sdwilsh]

And views with triggers to help manage some of the complexity (but not all of it because that hurts perf!).

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

Strictly additive changes to add all the needed tables, views, and triggers.  No logic changes here, and all tests still pass.

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v0.2

See http://shawnwilsher.com/archives/170 for the reasoning behind this change.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This is all set and ready to go now.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

There was a bug in the insert trigger for moz_historyvisits_view.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 333235 [details] [diff] [review]
v1.1

i don't understand the selective segregation of SQL from code. tables and triggers are kept in separate header files, but indexes and views aren't? this is not intuitive. i think indexes should be stored w/ the tables they index. views are like virtual tables, so maybe views and their indexes should be kept w/ the tables as well.

Comment 6

[Shawn Wilsher :sdwilsh]

I think you are right (and I've had that very idea myself).  I just haven't prioritized it since I can't get most of the unit tests to pass yet.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

A few more issues cropped up.  I'm fairly certain that things are working as expected now with regards to the triggers.

Comment 8

[Shawn Wilsher :sdwilsh]

If someone else wants to take on that refactoring, that'd be awesome.  It's low priority for me until I get these unit tests passing.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

Fixed a few more issues with the triggers.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

Moving the indicies I'm going to punt to bug 450516 since I really want to make sure this stuff lands in a2, and that's not terribly important.

Addresses your other comments.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.5

Last change, really.  I swear!

This fixes the visit_count modification bits for the moz_historyvisits_view triggers.  They were not filtering on visit_type, which just so happens to cause some test failures.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 333882 [details] [diff] [review]
v1.5


>+nsresult
>+nsNavHistory::InitTempTables()
>+{
>+  nsresult rv;
>+
>+  // moz_places_temp
>+  rv = mDBConn->ExecuteSimpleSQL(CREATE_MOZ_PLACES_TEMP);
>+  NS_ENSURE_SUCCESS(rv, rv);

nit: declare rv inline w/ first-use

>+nsresult
>+nsNavHistory::InitViews()
>+{
>+  nsresult rv;
>+
>+  // moz_places_view
>+  rv = mDBConn->ExecuteSimpleSQL(CREATE_MOZ_PLACES_VIEW);
>+  NS_ENSURE_SUCCESS(rv, rv);

ditto

r=me

Comment 13

[Marco Bonardo [:mak]]

-#define CREATE_MOZ_PLACES NS_LITERAL_CSTRING( \
-  "CREATE TABLE moz_places ( " \
+#define CREATE_MOZ_PLACES_BASE(__name) NS_LITERAL_CSTRING( \
+  "CREATE TABLE " __name " ( " \
     "  id INTEGER PRIMARY KEY" \
     ", url LONGVARCHAR" \
     ", title LONGVARCHAR" \
     ", rev_host LONGVARCHAR" \
     ", visit_count INTEGER DEFAULT 0" \
     ", hidden INTEGER DEFAULT 0 NOT NULL" \
     ", typed INTEGER DEFAULT 0 NOT NULL" \
     ", favicon_id INTEGER" \
     ", frecency INTEGER DEFAULT -1 NOT NULL" \
   ")" \
 )

maybe i'm crazy or blind, but should not moz_places_temp (and historyvisits_temp) be CREATE (TEMP|TEMPORARY) TABLE?

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> maybe i'm crazy or blind, but should not moz_places_temp (and
> historyvisits_temp) be CREATE (TEMP|TEMPORARY) TABLE?
Uh...yeah.  I'll get that fixed today.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v1.6

fixes the temp issue.  I can't believe we managed to miss that.  Tests still pass.

Comment 16

[Shawn Wilsher :sdwilsh]

Marco and I found a bug in the insertion triggers.  Four lines changed.  Updated the patch locally.

Comment 17

[Marco Bonardo [:mak]]

about that wrong insert trigger yesterday we were talking about doing IFNULL(MAX(id), 1), well the correct instead should be IFNULL(MAX(id), 0) because after that we do +1. So instead of having moz_places starting from 1 we would have it starting from 2, it would still work but better fixing it.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 334305 [details] [diff] [review]
v1.6

patch obsololete cause bogus

Comment 19

[Marco Bonardo [:mak]]

Attachment: v1.7

Version with fixed triggers

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> about that wrong insert trigger yesterday we were talking about doing
> IFNULL(MAX(id), 1), well the correct instead should be IFNULL(MAX(id), 0)
> because after that we do +1. So instead of having moz_places starting from 1 we
> would have it starting from 2, it would still work but better fixing it.
I do believe we talked about doing it with zero, because that's what I have in my version of the patch.

Comment 21

[Marco Bonardo [:mak]]

Attachment: v1.8

this changes the insert and delete triggers to update both tables instead of the view. greatly speeds up inserting and removing visits. Could help with hangs.

Shawn: if this is ok for you plase obsolete the previous, i'm not doing it until you review the change

Comment 22

[Marco Bonardo [:mak]]

see https://bugzilla.mozilla.org/show_bug.cgi?id=450705#c40 for reasoning

Comment 23

[Marco Bonardo [:mak]]

mh, oh wait this could again increase fsync on every visit insert if we have the place on disk, we need a better strategy probably :\

Comment 24

[Marco Bonardo [:mak]]

well, we could do that for delete since we are doing a delete from moz_historyvisits that will most likely do an fsync

Comment 25

[Shawn Wilsher :sdwilsh]

We could nest the update triggers fully and it would solve the problem, right?  To keep our sanity, I'd say do this with a #define value for the trigger body so we can just use the same sql in both places (and then are less likely to make a change in only one spot down the line breaking things).

I think we should do this in a new bug for our own sanity (and it'll be easier to review the change).

Comment 26

[Marco Bonardo [:mak]]

bug 456029 has a patch that applies UPON v1.7 patch, discussion will continue there

Comment 27

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/85f2b54a7231