Make mozStorage a little more developer friendly

RESOLVED FIXED

Status

()

Toolkit
Storage
--
enhancement
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Brett Wilson, Assigned: Brett Wilson)

Tracking

({fixed1.8.1})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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
(Assignee)

Comment 1

13 years ago
Created attachment 198091 [details] [diff] [review]
Implementations for TXN types an in-memory DB
Assignee: vladimir → brettw
Status: UNCONFIRMED → ASSIGNED
(Assignee)

Comment 2

13 years ago
Created attachment 198092 [details]
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.
(Assignee)

Comment 3

13 years ago
Created attachment 201759 [details] [diff] [review]
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.
Attachment #198091 - Attachment is obsolete: true
Attachment #198092 - Attachment is obsolete: true
Attachment #201759 - Flags: second-review?
Attachment #201759 - Flags: first-review?
(Assignee)

Updated

13 years ago
Attachment #201759 - Flags: second-review?(vladimir)
Attachment #201759 - Flags: second-review?
Attachment #201759 - Flags: first-review?(bryner)
Attachment #201759 - Flags: first-review?
Attachment #201759 - Flags: first-review?(bryner) → first-review+
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.
(Assignee)

Comment 5

13 years ago
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?
(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.
Blocks: 324274
checked in this patch on the branch.
Keywords: fixed1.8.1
(Assignee)

Comment 8

13 years ago
This appears to be done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

11 years ago
Attachment #201759 - Flags: second-review?(vladimir) → review?

Updated

11 years ago
Attachment #201759 - Flags: review? → review?(vladimir)
You need to log in before you can comment on or make changes to this bug.