421115 - [meta] Stop dynamically building queries

Status: RESOLVED INCOMPLETE

(Firefox :: Bookmarks & History, defect, P4)

Description

[Shawn Wilsher :sdwilsh]

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/[e-mail]/msg32053.html

Requesting blocking because we *really* shouldn't be doing that...

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 2

[Dietrich Ayala (:dietrich)]

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.

Comment 3

[Ondrej Brablc]

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.

Comment 4

[Shawn Wilsher :sdwilsh]

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

Comment 5

[Marco Bonardo [:mak]]

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

[Ondrej Brablc]

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

Comment 7

[Shawn Wilsher :sdwilsh]

(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

[Ondrej Brablc]

We already have a bug that should audit injection possibilities in Places - bug 405920. Should something else be analyzed?

Comment 9

[Shawn Wilsher :sdwilsh]

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.

Comment 10

[Dietrich Ayala (:dietrich)]

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.

Comment 11

[Ondrej Brablc]

Attachment: 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.

Comment 12

[Gervase Markham [:gerv]]

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

Comment 13

[Marco Bonardo [:mak]]

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.