Closed Bug 384526 Opened 12 years ago Closed 12 years ago

Use sqlite3_prepare_v2 instead of sqlite3_prepare

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

()

Details

Attachments

(1 file, 2 obsolete files)

It has some useful optimizations, but we'll have to change our code a bit.
Blocks: 291335
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9beta1
Attached patch v1.0 (obsolete) — Splinter Review
This was a big pain, but all the tests pass, so no regressions - yay!
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #270272 - Flags: review?(sspitzer)
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?
Flags: in-testsuite?
(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 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.
Attachment #270272 - Flags: review?(sspitzer) → review+
Attached patch v1.1 (obsolete) — Splinter Review
wow, bitrot sucks
Attachment #270272 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Suddenly failing test_storage_progresshandler.js.  Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 387609
Attached patch v1.2Splinter Review
Minor changes and a follow-up bug filed pending fix upstream.
Attachment #271689 - Attachment is obsolete: true
Attachment #271755 - Flags: review?(sspitzer)
Comment on attachment 271755 [details] [diff] [review]
v1.2

r=sspitzer (thanks for logging that spin off bug #387609)
Attachment #271755 - Flags: review?(sspitzer) → review+
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.