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

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

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

This is all set and ready to go now.

There was a bug in the insert trigger for moz_historyvisits_view.

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.

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.

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

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.

Fixed a few more issues with the triggers.

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.

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 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

-#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?

(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.

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

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

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 on attachment 334305 [details] [diff] [review] v1.6 patch obsololete cause bogus

Version with fixed triggers

(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.

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

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

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 :\

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

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).

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