458811 - Allow for multiple statements to be executed at in a transaction asynchronously

Status: VERIFIED FIXED

(Toolkit :: Storage, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

Right now it's really hard to run a series of statements that need proper ordering with the async API.  This isn't terrible to fix.

The hard part about this is how to handle a transaction.  These operations would likely want to be batched in a transaction.  However, there are a few issues:
1) a transaction could already be open - how do we handle that?
   a) wait until we get one (don't think this is good - see (2))
   b) carry on without it, and hope everything goes OK
2) transactions are not thread independent, so while we may be able to get a transaction, someone else can dump something in us without a problem.

I'm not really sure what a good solution is here, so feedback welcome.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This wasn't that bad - just had to convert AsyncExecute to handle more than one statement.

Comment 2

[Shawn Wilsher :sdwilsh]

Posted to dev.platform about the proposed interface change:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/690e85855bdbbc55#

Comment 3

[Dave Camp (:dcamp)]

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

>diff --git a/storage/src/mozStorageConnection.cpp b/storage/src/mozStorageConnection.cpp

>+
>+nsresult
>+mozStorageConnection::ExecuteAsync(mozIStorageStatement ** aStatements,
... 
>+        if (!stmts.AppendElement(new_stmt)) {
>+            rc = SQLITE_NOMEM;
>+            break;
>+        }
>+
>+        // Reset this statement
>+        (void)aStatements[i]->Reset();
>+    }

As mentioned on irc, in the unlikely event of a failure partway through, you have a partially-reset array.  Reset should probably be an all-or-nothing thing.

Comment 4

[Dave Camp (:dcamp)]

and if you reset statements even in the face of failure, that should probably be doc'ed.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> and if you reset statements even in the face of failure, that should probably
> be doc'ed.
I think it's covered by "These statements can be reused immediately, and reset does not need to be called."

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses review comments, and addresses one sr comment made in person.

Comment 7

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/60e6f428bd13

Comment 8

[Eric Shepherd [:sheppy]]

This has been documented for a while:

https://developer.mozilla.org/En/MozIStorageStatement

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> This has been documented for a while:
> 
> https://developer.mozilla.org/En/MozIStorageStatement
Not there - this bug is about adding the same method name to mozIStorageConnection, which takes an array of mozIStorageStatements.

Comment 10

[Eric Shepherd [:sheppy]]

Oops.  OK, sorry about that.

Comment 11

[Eric Shepherd [:sheppy]]

But it's already documented there, too.

https://developer.mozilla.org/en/mozIStorageConnection#executeAsync%28%29

:)

Comment 12

[Shawn Wilsher :sdwilsh]

uh, my bad...

Comment 13

[Eric Shepherd [:sheppy]]

Not really -- I did look in the wrong place; I just lucked out it had been done already in both places.  Sorry about that confusion.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking as verified fixed by seeing all tests pass and no backout happened.