Closed Bug 712427 Opened 8 years ago Closed 8 years ago

Differentiate identical PRAGMA strings executed on different DBs

Categories

(Toolkit :: Places, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: vladan, Assigned: froydnj)

References

Details

Attachments

(1 file, 1 obsolete file)

We should add comments to identical PRAGMA strings executed against different DBs. This would help clarify slow SQL Telemetry results (bug 699051).

Examples of such PRAGMAs that have been reported to Telemetry:

PRAGMA page_size
PRAGMA cache_size
PRAGMA journal_mode = wal
PRAGMA temp_store
Summary: Differentiate PRAGMA strings executed on different DBs → Differentiate identical PRAGMA strings executed on different DBs
Attached patch annotating PRAGMA queries (obsolete) — Splinter Review
Here's a patch for the interesting queries mentioned by Vladan.

I chose to annotate each PRAGMA query, rather than inventing some grand interface for annotation, because:

1) PRAGMA queries are executed several different ways (ExecuteSimpleSQL, prepared statements, etc.) and designing a single interface for all those cases didn't seem worthwhile.  The method chosen also works well for JavaScript;
2) It's generally obvious given the source filename what database is being poked at.  If it turns out we really do need the db filename, then we'll have to revisit this, but this should be good enough.
Attachment #585041 - Flags: feedback?(mak77)
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
For cases like "PRAGMA key=value", your annotation ends up making "PRAGMA key=/* filepath comment */value", that is not really nice to read, maybe we should just annotate with a prefix comment, to avoid these cases.
In the cpp case, rather than making a wrapping macro, could we just have a string macro to add in front, like MOZ_STORAGE_UNIQUIFY_QUERY "PRAGMA something"? This is mostly because our queries are already 90% wrapped by NS_LITERAL macros, and it's becoming an heavy wrapped syntax.
To __FILE__ annotation, you may even add __LINE__, just to distinguish in case the same pragma happens in multiple lines of the same file.
For the js case you may maybe use Components.stack.filename and Components.stack.lineNumber
(In reply to Marco Bonardo [:mak] from comment #2)
> In the cpp case, rather than making a wrapping macro, could we just have a
> string macro to add in front, like MOZ_STORAGE_UNIQUIFY_QUERY "PRAGMA
> something"? This is mostly because our queries are already 90% wrapped by
> NS_LITERAL macros, and it's becoming an heavy wrapped syntax.

This is not a bad idea, though /MOZ_STORAGE_UNIQUIFY_QUERY PRAGMA_STR/ is only one character shorter than /MOZ_STORAGE_UNIQUIFY_QUERY(PRAGMA_STR)/, even if the former has slightly nicer wrapping properties.

> To __FILE__ annotation, you may even add __LINE__, just to distinguish in
> case the same pragma happens in multiple lines of the same file.
> For the js case you may maybe use Components.stack.filename and
> Components.stack.lineNumber

I considered using __LINE__, but decided not to on the grounds that 1) we don't have instances of the same pragma in the same source file and we seem unlikely to in the future; and 2) I wanted the pragmas to remain stable when the source code changes and the pragmas get moved around.  For the lone JS instance, I opted to manually disambiguate them.
(In reply to Nathan Froyd (:froydnj) from comment #3)
> This is not a bad idea, though /MOZ_STORAGE_UNIQUIFY_QUERY PRAGMA_STR/ is
> only one character shorter than /MOZ_STORAGE_UNIQUIFY_QUERY(PRAGMA_STR)/,
> even if the former has slightly nicer wrapping properties.

The length is a secondary problem to nested wrapping, btw you may avoid the _PRAGMA part, ideally this may be used for any query, where needed. so MOZ_STORAGE_UNIQUIFY_QUERY_STR

> I considered using __LINE__, but decided not to on the grounds that 1) we
> don't have instances of the same pragma in the same source file and we seem
> unlikely to in the future; and 2) I wanted the pragmas to remain stable when
> the source code changes and the pragmas get moved around.  For the lone JS
> instance, I opted to manually disambiguate them.

I see. Then maybe you could get the function name from the js stack rather than hardcoding it.
New patch, using MOZ_STORAGE_UNIQUIFY_QUERY_STR prefixing technique, as discussed.

JS queries left untouched, as I think the uniquification there is slightly better.

I suppose I have to get netwerk approval for the lone nsCookieService.cpp change, huh?
Attachment #585041 - Attachment is obsolete: true
Attachment #585041 - Flags: feedback?(mak77)
Attachment #585757 - Flags: feedback?(mak77)
Comment on attachment 585757 [details] [diff] [review]
annotating PRAGMA queries

Review of attachment 585757 [details] [diff] [review]:
-----------------------------------------------------------------

Well, if you want to get further feedback it's ok, but the changes are pretty much trivial

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +912,4 @@
>  
>        PLACES_DATABASE_SIZE_PER_PAGE_B: function() {
>          // Cannot use the filesize here, due to chunked growth.
> +        let stmt = DBConn.createStatement("PRAGMA page_size /* PlacesDBUtils.jsm SIZE_PER_PAGE_B */");

you can avoid the .jsm here, PlacesDBUtils is a unique object.
Attachment #585757 - Flags: feedback?(mak77) → feedback+
Attachment #585757 - Flags: feedback+ → review+
(In reply to Marco Bonardo [:mak] from comment #6)
>> ::: toolkit/components/places/PlacesDBUtils.jsm
> @@ +912,4 @@
> >  
> >        PLACES_DATABASE_SIZE_PER_PAGE_B: function() {
> >          // Cannot use the filesize here, due to chunked growth.
> > +        let stmt = DBConn.createStatement("PRAGMA page_size /* PlacesDBUtils.jsm SIZE_PER_PAGE_B */");
> 
> you can avoid the .jsm here, PlacesDBUtils is a unique object.

I'm just going to keep it so that we have filenames everywhere.
Keywords: checkin-needed
feels like this wont be super-useful unless we add filenames to some of these more generic ones..ie vacuum helper.
(In reply to Taras Glek (:taras) from comment #8)
> feels like this wont be super-useful unless we add filenames to some of
> these more generic ones..ie vacuum helper.

this shouldn't block checkin-needed. can be addressed in followups.
Thanks Rafael, http://hg.mozilla.org/integration/mozilla-inbound/rev/9136ddb5f8a1 is the hg url
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
it will be fixed when lands in central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/9136ddb5f8a1
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Depends on: 743650
You need to log in before you can comment on or make changes to this bug.