Closed Bug 449086 Opened 11 years ago Closed 11 years ago

Create temporary tables for frequently used places tables

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 10 obsolete files)

And views with triggers to help manage some of the complexity (but not all of it because that hurts perf!).
Attached patch v0.1 (obsolete) — Splinter Review
Strictly additive changes to add all the needed tables, views, and triggers.  No logic changes here, and all tests still pass.
Blocks: 442967
Priority: -- → P1
Attached patch v0.2 (obsolete) — Splinter Review
See http://shawnwilsher.com/archives/170 for the reasoning behind this change.
Attachment #332290 - Attachment is obsolete: true
Blocks: 449640
Attached patch v1.0 (obsolete) — Splinter Review
This is all set and ready to go now.
Attachment #332472 - Attachment is obsolete: true
Attachment #333130 - Flags: review?(dietrich)
Whiteboard: [has patch][needs review dietrich]
Attached patch v1.1 (obsolete) — Splinter Review
There was a bug in the insert trigger for moz_historyvisits_view.
Attachment #333130 - Attachment is obsolete: true
Attachment #333235 - Flags: review?(dietrich)
Attachment #333130 - Flags: review?(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.
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.
Attached patch v1.2 (obsolete) — Splinter Review
A few more issues cropped up.  I'm fairly certain that things are working as expected now with regards to the triggers.
Attachment #333235 - Attachment is obsolete: true
Attachment #333235 - Flags: review?(dietrich)
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.
Whiteboard: [has patch][needs review dietrich] → [has patch][needs refactoring]
Attached patch v1.3 (obsolete) — Splinter Review
Fixed a few more issues with the triggers.
Attachment #333307 - Attachment is obsolete: true
No longer depends on: 429988
No longer blocks: 442967
Attached patch v1.4 (obsolete) — Splinter Review
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.
Attachment #333348 - Attachment is obsolete: true
Attachment #333667 - Flags: review?(dietrich)
Whiteboard: [has patch][needs refactoring] → [has patch][needs review dietrich]
Attached patch v1.5 (obsolete) — Splinter Review
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.
Attachment #333667 - Attachment is obsolete: true
Attachment #333882 - Flags: review?(dietrich)
Attachment #333667 - Flags: review?(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
Attachment #333882 - Flags: review?(dietrich) → review+
-#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.
Whiteboard: [has patch][needs review dietrich] → [needs new patch]
Attached patch v1.6 (obsolete) — Splinter Review
fixes the temp issue.  I can't believe we managed to miss that.  Tests still pass.
Attachment #333882 - Attachment is obsolete: true
Whiteboard: [needs new patch] → [has patch][has review][can land]
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
Attachment #334305 - Attachment is obsolete: true
Whiteboard: [has patch][has review][can land] → [needs new patch]
Attached patch v1.7Splinter Review
Version with fixed triggers
Whiteboard: [needs new patch] → [has patch][has review]
(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.
Attached patch v1.8 (obsolete) — Splinter Review
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
Attachment #339426 - Flags: review?(sdwilsh)
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 :\
Attachment #339426 - Attachment is obsolete: true
Attachment #339426 - Flags: review?(sdwilsh)
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
http://hg.mozilla.org/mozilla-central/rev/85f2b54a7231
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
Target Milestone: mozilla1.9.1a2 → mozilla1.9.1b2
No longer depends on: 470348
You need to log in before you can comment on or make changes to this bug.