310636 - Make mozStorage a little more developer friendly

Status: RESOLVED FIXED

(Toolkit :: Storage, enhancement)

Description

[Brett Wilson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5

mozStorage should expose the different types of sqlite transactions and provide
more better support for C++ programs.

Reproducible: Always

Comment 1

[Brett Wilson]

Attachment: Implementations for TXN types an in-memory DB

Comment 2

[Brett Wilson]

Attachment: Helper classes for C++

This file (should be in storage/public) implements helper classes to scope
transactions and statements based on C++ scope to help eliminate dangling ones.

Comment 3

[Brett Wilson]

Attachment: New helper class and in-memory database support

This new patch adds some more capabilities to the transaction and statement helper classes. It also includes support for in-memory databases (use the name "memory" to GetProfileStorage) and different transaction types.

Comment 4

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

Comment on attachment 201759 [details] [diff] [review]
New helper class and in-memory database support

(Unbelievable.. my laptop crashed with my unsubmitted comments in the buffer this afternoon. Argh.)



>+class mozStorageTransaction
>+{
>+public:
>+  mozStorageTransaction(mozIStorageConnection* aConnection,
>+                        PRBool aCommitOnComplete,

I would have aCommitOnComplete default to FALSE, or better yet, just omit it entirely and always Rollback in any case other than an explicit Commit().  This is the way most db transaction wrappers work, and it makes sense -- any error return or exception thrown will cause a rollback, and it makes the commit action explicit.



>+  /**
>+   * Commits the transaction if one is in progress. If one is not in progress,
>+   * this is a NOP since the actual owner of the transaction outside of our
>+   * scope is in charge of finally comitting or rolling back the transaction.
>+   */
>+  nsresult Commit()
>+  {
>+    if (mCompleted)
>+      return NS_OK; // already done
>+    mCompleted = PR_TRUE;
>+    if (! mHasTransaction)
>+      return NS_OK; // transaction not ours, ignore
>+    return mConnection->CommitTransaction();
>+  }
>+
>+  /**
>+   * Rolls back the transaction in progress. You should only call this function
>+   * if this object has a real transaction (HasTransaction() = true) because
>+   * otherwise, there is no transaction to roll back.
>+   */
>+  nsresult Rollback()
>+  {
>+    if (mCompleted)
>+      return NS_OK; // already done
>+    mCompleted = PR_TRUE;
>+    if (! mHasTransaction)
>+      return NS_ERROR_FAILURE;
>+    return mConnection->RollbackTransaction();
>+  }

Have you thought about tying the Connection and the current transaction object more closely?  That would allow you to have nested transactions, such that a rollback in any transaction opened within an existing one would cause everything to be rolled back; otherwise, there's no way for a sub-function to do any kind of rollback if it's given invalid input.

Calling Rollback() in a sub-transaction would flag the master transaction as to be rolled back, so that no matter if Commit() or Rollback() were called, it would allways do a rollback (and if commit() was called, it would return an appropriate error code saying that the commit failed).


>+  /**
>+   * Call this to make the statement not reset. You might do this if you know
>+   * that the statement has been reset.
>+   */
>+  void Abandon()
>+  {
>+    mStatement = nsnull;
>+  }

Probably no need for Abandon (as you say, resetting an already-reset statement has little cost), and I'm not sure if it makes things any clearer.

Comment 5

[Brett Wilson]

I had the 'aCommitOnComplete' have a default value in a previous implementation, but decided I liked being forced to think about what would happen on error whenever I used the class. I'm not much opposed to changing this, though.

> Have you thought about tying the Connection and the current transaction object
> more closely?  That would allow you to have nested transactions, such that a
> rollback in any transaction opened within an existing one would cause
> everything to be rolled back; otherwise, there's no way for a sub-function to
> do any kind of rollback if it's given invalid input.
> 
> Calling Rollback() in a sub-transaction would flag the master transaction as to
> be rolled back, so that no matter if Commit() or Rollback() were called, it
> would allways do a rollback (and if commit() was called, it would return an
> appropriate error code saying that the commit failed).

I implemented this before in connection's implementation of transactions: BeginTransaction would always succeed and increase a counter. Commit would decrease the counter, and the actual commit happens when it reaches 0. If a flag is set by rollback, the transaction is rolled back instead.

We discussed this on IRC (maybe two months ago) but I was left with the impression that you didn't like it. I don't know if there was some miscommunication or you just had a problem with some detail of it. Did you want to have it on a different layer than in the connection transaction functions?

> Probably no need for Abandon (as you say, resetting an already-reset statement
> has little cost), and I'm not sure if it makes things any clearer. 

Do we actually know what happens during reseting? There were a number of cases where I used this object to make sure a statement gets reset on error, but then used Execute() which resets the statement itself. I didn't want to have it reset twice, even though it's pretty fast. Do we know that it's almost 0 cost, or is it merely low cost to reset a statement more than once?

Comment 6

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

(In reply to comment #5)
> I had the 'aCommitOnComplete' have a default value in a previous
> implementation, but decided I liked being forced to think about what would
> happen on error whenever I used the class. I'm not much opposed to changing
> this, though.

Yeah, but commit-on-error is almost always wrong, so I'd rather people not have that option to choose from :)

> > Have you thought about tying the Connection and the current transaction object
> > more closely?  That would allow you to have nested transactions, such that a
> > rollback in any transaction opened within an existing one would cause
> > everything to be rolled back; otherwise, there's no way for a sub-function to
> > do any kind of rollback if it's given invalid input.
> > 
> > Calling Rollback() in a sub-transaction would flag the master transaction as to
> > be rolled back, so that no matter if Commit() or Rollback() were called, it
> > would allways do a rollback (and if commit() was called, it would return an
> > appropriate error code saying that the commit failed).
> 
> I implemented this before in connection's implementation of transactions:
> BeginTransaction would always succeed and increase a counter. Commit would
> decrease the counter, and the actual commit happens when it reaches 0. If a
> flag is set by rollback, the transaction is rolled back instead.
> 
> We discussed this on IRC (maybe two months ago) but I was left with the
> impression that you didn't like it. I don't know if there was some
> miscommunication or you just had a problem with some detail of it. Did you want
> to have it on a different layer than in the connection transaction functions?

I think when we discussed it there wasn't a way for an inner transaction to flag for rollback; but with that addition I think it should be ok.  I worry a little about code that calls a function that does a sub-transaction which flags for a rollback, but the code doesn't do any error checking and assumes that it worked and goes ahead and does other work based on that assumption (non-db work, that won't be rolled back)... but that's just a coding practice issue that can be deal with.

I would prefer this kind of implementation, as it allows you to at least have some form of error handling with nested transactions/calls; otherwise, inner errors won't be caught, and we could have garbage committed to the database.  Sorry for the miscommunication!

> > Probably no need for Abandon (as you say, resetting an already-reset statement
> > has little cost), and I'm not sure if it makes things any clearer. 
> 
> Do we actually know what happens during reseting? There were a number of cases
> where I used this object to make sure a statement gets reset on error, but then
> used Execute() which resets the statement itself. I didn't want to have it
> reset twice, even though it's pretty fast. Do we know that it's almost 0 cost,
> or is it merely low cost to reset a statement more than once?

It's low cost, though there is some cost.  I guess Abandon doesn't hurt anything -- worst case a statement will be reset twice.

Comment 7

[Brian Ryner (not reading)]

checked in this patch on the branch.

Comment 8

[Brett Wilson]

This appears to be done