[meta] Stop dynamically building queries

RESOLVED INCOMPLETE

Status

()

P4
normal
RESOLVED INCOMPLETE
11 years ago
5 years ago

People

(Reporter: sdwilsh, Unassigned)

Tracking

Trunk
Points:
---
Bug Flags:
blocking-firefox3 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
Spun off from bug 420354 comment 9.

There are a few places where we are building up queries dynamically.  At least one situation we are doing this because it's part of a subquery.  However, a recent post on the sqlite mailing list indicates that this isn't the best way to do this, and we should be using temporary tables:
http://www.mail-archive.com/sqlite-users@sqlite.org/msg32053.html

Requesting blocking because we *really* shouldn't be doing that...
Flags: blocking-firefox3?
Right now gonna make this a P1 as it feels like the sort of change that needs beta exposure, but Dietrich should feel free to triage that down to a P2.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P1
Yes, I agree dynamic query building is not ideal, but it's also incorrect to say that it's always slow and always dangerous.

This is fine for a meta bug, but please file individual bugs against the offending queries. I'm changing this to a P2. If you find a query exhibiting slowness or danger, please bump back up to a P1.
Priority: P1 → P2

Comment 3

11 years ago
I was too slow writing my comment on this, and after Dietrich's comment it is probably superfluous now, but if someone likes reading here comes:


mozStorage supports only dynamic queries (we have no embedded SQL). So this bug should probably read "Stop executing non prepared queries". With mozStorage each query must be prepared during runtime. As I have analyzed in bug 364272, at the moment this leads to slower Ts. I would welcome some kind of lazy preparation, which should, most likely, be built right in mozStorage.

But back to places, I assume we speak about nsNavHistory::ConstructQueryString. Yes, this method prepares all the statements before each execution. However, the preparation is usually just a small fraction of execution time because the execution is slow. It is of course possible to have some of the queries prepared (with the Ts penalty) but this would not make the code more readable. 

If we now drill down to PlacesSQLQueryBuilder::SelectAsDay(), we can say, that this one could be made static too. With bug 420354 it will be possible to bind string parameters and it should not be big effort to add support for other parameter types. However, in this special case you will end up with 6 very similar fragments (now in for loop), what does not make code better maintainable and could have small impact on binary size. 

And the last point is subqueries. In this case no temporary tables would help. There are 7 subqueries each one returning 0 to 1 row (watch the LIMIT 1), I believe that this is optimal query.
(Reporter)

Comment 4

11 years ago
(In reply to comment #3)
> mozStorage supports only dynamic queries (we have no embedded SQL). So this bug
> should probably read "Stop executing non prepared queries". With mozStorage
> each query must be prepared during runtime. As I have analyzed in bug 364272,
> at the moment this leads to slower Ts. I would welcome some kind of lazy
> preparation, which should, most likely, be built right in mozStorage.
File a bug please?  You could probably request blocking too - I foresee that only taking me about an hour or so to do...
i do not completely agree that a temp table is always better... often building a temp table is slower than the query itself, so that really depends on each query.

what Ondrej suggested (lazy statement creation on first use) is a really good point

Comment 6

11 years ago
(In reply to comment #4)
> File a bug please?  You could probably request blocking too - I foresee that
> only taking me about an hour or so to do...

I have created bug 421513. It would be nice if you can now clarify what should be done with this bug, at the moment it is a bit unclear. 

(Reporter)

Comment 7

11 years ago
(In reply to comment #5)
> i do not completely agree that a temp table is always better... often building
> a temp table is slower than the query itself, so that really depends on each
> query.
At least for the cases I saw where we are building the inner queries of a query dynamically, the sqlite folks say a temporary table is already created, so there really is no difference.

If there are other cases where we are dynamically building queries for different reasons, they should also be evaluated.

Comment 8

11 years ago
We already have a bug that should audit injection possibilities in Places - bug 405920. Should something else be analyzed?
(Reporter)

Comment 9

11 years ago
I suppose this bug is partially about bug 405920, but at the same time this caused bug 420354, so I'm not sure it's totally related.
So, I think there's consensus that we should generally favor binding over manual query building. Please file individual bugs for queries that need refactoring, and make those bugs block this one.

However, I don't think this meta bug should block the release. Please request blocking on any dependent bugs that are deserving.
Depends on: 420354
Flags: blocking-firefox3+ → blocking-firefox3?
Priority: P2 → P4
Summary: Stop dynamically building queries → [meta] Stop dynamically building queries

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+

Updated

11 years ago
Flags: blocking-firefox3+ → blocking-firefox3-

Comment 11

11 years ago
Created attachment 310275 [details]
List of dynamically built SQL statements

Here is the list of all queries that are built in run-time. I would personally suggest keeping them as they are and only try to replace them when modifying such code. I have finished the audit (bug 405920) and did not find any security risks.
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
There's no plan for this apart "try to makes queries as fast as possible", the suggestion to use temp tables to join in some cases where we query with lots of OR/AND is definitely interesting, but doesn't deserve a bug report to keep it. We will use it when needed.
Incomplete as this doesn't identify the points where to use such strategy, it only points out good practice.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.