291991 - update storage to check if a statement needs to be recreated, to avoid SQLITE_SCHEMA

Status: RESOLVED INVALID

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

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Patch attached; new sqlite3 function should be going into core SQLite at some
point as well.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: storage-stmt-fix-0.patch

Comment 2

[Mike Shaver (:shaver emeritus)]

Comment on attachment 181899 [details] [diff] [review]
storage-stmt-fix-0.patch

>+    if (mExecuting == PR_FALSE) {

(Checking against == PR_FALSE specifically seems awkward, but if that's
the style you prefer to |if (!mExecuting)|, it's your code!)

>         } else if (srv == SQLITE_ERROR) {
>             // so we may end up with a SQLITE_SCHEMA only after
>-            // we finalize, because SQLite's error reporting story
>+            // we reset, because SQLite's error reporting story
>             // sucks.
>             if (mExecuting == PR_TRUE) {
>+                PR_LOG(gStorageLog, PR_LOG_ERROR, ("SQLITE_SCHEMA after mExecuting was true!"));
>+
>                 mExecuting = PR_FALSE;
>                 return NS_ERROR_FAILURE;

It doesn't look like this block has to do with SQLITE_SCHEMA (the
one before it would be entered instead), so the comment and PR_LOG
message seem misleading.  Should those be SQLITE_ERROR instead?

>+nsresult
>+mozStorageStatement::Recreate()
>+{
>+    nsresult rv;
>+    sqlite3_stmt *savedStmt = mDBStatement;
>+    mDBStatement = nsnull;
>+    rv = Initialize(mDBConnection, mStatementString);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // copy over the param bindings
>+    sqlite3_transfer_bindings(savedStmt, mDBStatement);

If this fails, do we want to just keep going? I think we'd want to
return an error to the caller, which would trickle out for the
mozStorage consumer to handle.

Does one of those cases (the VDBE_MAGIC_RUN test?) catch us if
the database has been DETACHed?

r=shaver with error checking of transfer_bindings, at least, and
probably the SCHEMA->ERROR changes.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Is this the same as bug 291335? Will that be fixed with this?

Comment 4

[Shawn Wilsher :sdwilsh]

Turns out we don't have to worry about this anymore