Closed Bug 458811 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Storage, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1b2
Attached patch v1.0 (obsolete) — Splinter Review
This wasn't that bad - just had to convert AsyncExecute to handle more than one statement.
Attachment #342674 - Flags: superreview?(jonas)
Attachment #342674 - Flags: review?(dcamp)
Posted to dev.platform about the proposed interface change:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/690e85855bdbbc55#
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.
Attachment #342674 - Flags: review?(dcamp) → review+
and if you reset statements even in the face of failure, that should probably be doc'ed.
(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."
Attached patch v1.1Splinter Review
Addresses review comments, and addresses one sr comment made in person.
Attachment #342674 - Attachment is obsolete: true
Attachment #342912 - Flags: superreview?(jonas)
Attachment #342674 - Flags: superreview?(jonas)
Attachment #342912 - Flags: superreview?(jonas) → superreview+
http://hg.mozilla.org/mozilla-central/rev/60e6f428bd13
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Flags: in-litmus-
Keywords: dev-doc-needed
Resolution: --- → FIXED
Depends on: 429986
Blocks: 459491
(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.
Oops.  OK, sorry about that.
uh, my bad...
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.
Marking as verified fixed by seeing all tests pass and no backout happened.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.