481257 - Insertion triggers are unnecessarily slow

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

Spun out of bug 481113.  In our insertion triggers, we have a bit of SQL that looks like this:
MAX((SELECT IFNULL(MAX(id), 0) FROM moz_places_temp), (SELECT IFNULL(MAX(id), 0) FROM moz_places)) + 1
We use this to determine the new insertion id.  The trigger for moz_historyvisits is very similar.
The bad news is that how this is written, it requires a full table scan of each table.  According to drh, this is the likely culprit since we are spending a bunch of time in sqlite3VdbeExec.

We can let the optimizer use the index instead of a table scan if we change those bits to the following:
MAX(IFNULL((SELECT MAX(id) FROM moz_places_temp), 0), (IFNULL(SELECT MAX(id) FROM moz_places), 0)) + 1
Note the change of location for IFNULL.

Comment 1

[Boris Zbarsky [:bzbarsky]]

If there's a quick-n-dirty way that I can convert an existing profile to one that uses the new trigger, I'd love to give this a test!

I assume there's more to it than just replacing that string in our code, since we'd need to convert existing tables?

Comment 2

[Shawn Wilsher :sdwilsh]

because it uses temp tables, it's created at every startup (sucky, yes).  It is just a simple string replace.

Comment 3

[David Dahl :ddahl]

Attachment: WIP patch 0

I had an assertion thrown, but all tests passed. Please apply this and run the test suite - let me know if there are any failures

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 365294 [details] [diff] [review]
WIP patch 0

r=sdwilsh, thanks!

Comment 5

[Boris Zbarsky [:bzbarsky]]

If all tests passed, the test suite is broken.  Both the strings in comment 0 and the ones in that patch are misparenthesized, no?  From the patch:

+    "VALUES  MAX(IFNULL((SELECT MAX(id) FROM moz_places_temp), 0),"\
+                "(IFNULL(SELECT MAX(id) FROM moz_places), 0)) + 1, " \

This is missing a parenthesis before the first "MAX".  It's got an extra parenthesis before the second IFNULL.  I would fully expect this query to fail to execute (and in fact when I initially tried the suggested fix that's exactly what I saw happening here, until I fixed the parens).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: What I was just testing

I haven't run the relevant test suite on this, but I _did_ run reftest as described in bug 481113.  Instead of taking 17 minutes, it took about 8, which is pretty close to what it takes in a vanilla profile.  So as far as I can tell this patch fixes that bug.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
Yes, it was subtly incorrect, but really hard to see until I did some indentation:
MAX(
  IFNULL(
    (SELECT MAX(id) FROM moz_places_temp),
    0),
  IFNULL((SELECT MAX(id) FROM moz_places),
    0)
) + 1
Which is what your patch does.  Parens are hard to read sometimes :)

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 365297 [details] [diff] [review]
What I was just testing

r=sdwilsh on this, with the correct parens.

Comment 9

[Shawn Wilsher :sdwilsh]

Yes, this should absolutely block as it's a major slowdown.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We should add some tests that would catch the incorrect parens too.

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> We should add some tests that would catch the incorrect parens too.

sync tests should notice a problem in syncing queries

Comment 12

[Shawn Wilsher :sdwilsh]

yeah, don't understand how tests were passing to be honest.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Good to see our new friend "bz" fixing a "good first bug". ;p

Comment 14

[Boris Zbarsky [:bzbarsky]]

It's shawn's fix, I tell you!

Oh, and and as far as parens being hard to read, just put (show-paren-mode t) in your .emacs.  If you're not using emacs, maybe it's time to start!  ;)

Comment 15

[Shawn Wilsher :sdwilsh]

With the original attachment on this bug, every test fails for me, with this showing up in the log:
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Code/moz/central/mozilla/toolkit/components/places/src/nsNavHistory.cpp, line 1046
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Code/moz/central/mozilla/toolkit/components/places/src/nsNavHistory.cpp, line 895
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Code/moz/central/mozilla/toolkit/components/places/src/nsNavHistory.cpp, line 1046
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Code/moz/central/mozilla/toolkit/components/places/src/nsNavHistory.cpp, line 895
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Code/moz/central/mozilla/toolkit/components/places/src/nsNavHistory.cpp, line 448

So, I'm not sure how they passed before.

Comment 16

[Shawn Wilsher :sdwilsh]

Boris - can you check this in, or should someone on the places team do this for you?

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, those warnings were exactly what I saw when I screwed up the parens.

I can push this.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/92ea141a0969

Comment 19

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/875b7fd92b0b

Comment 20

[Nochum Sossonko [:Natch]]

I think this was pushed to the wrong branch, i.e. the 3.1 beta3 branch...

Comment 21

[Marco Bonardo [:mak]]

oh right. this should be on "default" branch, not b3 :\

Comment 22

[Shawn Wilsher :sdwilsh]

yup - bz landed this on the branch.  Oops :)

Comment 23

[Nick Thomas [:nthomas] (UTC+12)]

Please revert revision 875b7fd92b0b on mozilla-1.9.1.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, crap.  Silly tip vs not tip stuff.  Fixing.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fd8ca820c460 to back out on the relbranch.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Gah.  Also pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/790666cd7d3d to back out the part that accidentally landed with the backout.

I have now verified that current GECKO191b3_20090304_RELBRANCH is identical to the parent of rev 875b7fd92b0b.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c17faff8dd0c on the right branch.