Create temporary tables for frequently used places tables

RESOLVED FIXED in mozilla1.9.1b2

Status

()

P1
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9.1b2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

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

Comment 1

11 years ago
Created attachment 332290 [details] [diff] [review]
v0.1

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
(Assignee)

Comment 2

11 years ago
Created attachment 332472 [details] [diff] [review]
v0.2

See http://shawnwilsher.com/archives/170 for the reasoning behind this change.
Attachment #332290 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Blocks: 449640
(Assignee)

Comment 3

11 years ago
Created attachment 333130 [details] [diff] [review]
v1.0

This is all set and ready to go now.
Attachment #332472 - Attachment is obsolete: true
Attachment #333130 - Flags: review?(dietrich)
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review dietrich]
(Assignee)

Comment 4

11 years ago
Created attachment 333235 [details] [diff] [review]
v1.1

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

Comment 6

11 years ago
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.
(Assignee)

Comment 7

11 years ago
Created attachment 333307 [details] [diff] [review]
v1.2

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)
(Assignee)

Comment 8

11 years ago
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]
(Assignee)

Comment 9

11 years ago
Created attachment 333348 [details] [diff] [review]
v1.3

Fixed a few more issues with the triggers.
Attachment #333307 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
No longer depends on: 429988
(Assignee)

Updated

11 years ago
No longer blocks: 442967
(Assignee)

Comment 10

11 years ago
Created attachment 333667 [details] [diff] [review]
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.
Attachment #333348 - Attachment is obsolete: true
Attachment #333667 - Flags: review?(dietrich)
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs refactoring] → [has patch][needs review dietrich]
(Assignee)

Comment 11

11 years ago
Created attachment 333882 [details] [diff] [review]
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.
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?
(Assignee)

Comment 14

11 years ago
(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]
(Assignee)

Comment 15

11 years ago
Created attachment 334305 [details] [diff] [review]
v1.6

fixes the temp issue.  I can't believe we managed to miss that.  Tests still pass.
Attachment #333882 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Whiteboard: [needs new patch] → [has patch][has review][can land]
(Assignee)

Comment 16

10 years ago
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]
Created attachment 338302 [details] [diff] [review]
v1.7

Version with fixed triggers
Whiteboard: [needs new patch] → [has patch][has review]
(Assignee)

Comment 20

10 years ago
(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.
Created attachment 339426 [details] [diff] [review]
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
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
(Assignee)

Comment 25

10 years ago
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).
Depends on: 456029
bug 456029 has a patch that applies UPON v1.7 patch, discussion will continue there
(Assignee)

Comment 27

10 years ago
http://hg.mozilla.org/mozilla-central/rev/85f2b54a7231
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
Target Milestone: mozilla1.9.1a2 → mozilla1.9.1b2
Depends on: 470348
No longer depends on: 470348
Depends on: 466407
You need to log in before you can comment on or make changes to this bug.