384526 - Use sqlite3_prepare_v2 instead of sqlite3_prepare

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

It has some useful optimizations, but we'll have to change our code a bit.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This was a big pain, but all the tests pass, so no regressions - yay!

Comment 2

[(not reading, please use [e-mail] instead)]

hey shawn, sorry for the delay.

Some questions:

From the url:

"If the database schema changes, instead of returning SQLITE_SCHEMA as it always used to do, sqlite3_step() will automatically recompile the SQL statement and try to run it again. If the schema has changed in a way that makes the statement no longer valid, sqlite3_step() will still return SQLITE_SCHEMA. But unlike the legacy behavior, SQLITE_SCHEMA is now a fatal error. Calling sqlite3_prepare_v2() again will not make the error go away. Note: use sqlite3_errmsg() to find the text of the parsing error that results in an SQLITE_SCHEMA return."

1) So, shouldn't we be handling SQLITE_SCHEMA error code still?

2) Why do we still need to retrun on SQLITE_SCHEMA errors after calling sqlite3_prepare_v2()?

3)  Do we still want the "shouldn't get here" comment?

     // shouldn't get here
-    return NS_ERROR_FAILURE;
+    return ConvertResultCode(srv);

4)  Do you have a test case?

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> 1) So, shouldn't we be handling SQLITE_SCHEMA error code still?
We still do with ConvertResultCode.

> 2) Why do we still need to retrun on SQLITE_SCHEMA errors after calling
> sqlite3_prepare_v2()?
From your quoted text:
"SQLITE_SCHEMA is now a fatal error"
:)

> 3)  Do we still want the "shouldn't get here" comment?
Yes, that should be gone.

> 4)  Do you have a test case?
We have test cases that test the behavior of mozIStorageStatement, and those still pass with this.  I'm not sure how to explicitly test this otherwise though.

Comment 4

[(not reading, please use [e-mail] instead)]

Comment on attachment 270272 [details] [diff] [review]
v1.0

chatting with shawn.  he explained how ConvertResultCode() will convert SQLITE_SCHEMA to NS_ERROR_FAILURE

Please remove the "shouldn't get here" comment before checking in.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

wow, bitrot sucks

Comment 6

[Shawn Wilsher :sdwilsh]

Checking in storage/src/mozStorageConnection.cpp;
new revision: 1.25; previous revision: 1.24
Checking in storage/src/mozStorageStatement.cpp;
new revision: 1.20; previous revision: 1.19

Comment 7

[Shawn Wilsher :sdwilsh]

Suddenly failing test_storage_progresshandler.js.  Backed out.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Minor changes and a follow-up bug filed pending fix upstream.

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 271755 [details] [diff] [review]
v1.2

r=sspitzer (thanks for logging that spin off bug #387609)

Comment 10

[Shawn Wilsher :sdwilsh]

Checking in storage/src/mozStorageConnection.cpp;
new revision: 1.27; previous revision: 1.26
Checking in storage/src/mozStorageStatement.cpp;
new revision: 1.22; previous revision: 1.21
Checking in storage/src/mozStorage.h;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_storage_progresshandler.js;
new revision: 1.2; previous revision: 1.1