RE: [IndexedDB] IDBObjectStore.delete should accept a KeyRange

Forgot some things!

On Thursday, October 27, 2011 6:00 PM, Jonas Sicking wrote:
> On Thu, Oct 27, 2011 at 9:33 AM, Israel Hilerio <israelh@microsoft.com>
> wrote:
> > On Wednesday, October 26, 2011 6:38 PM, Jonas Sicking wrote:
> >> On Wed, Oct 26, 2011 at 11:31 AM, Israel Hilerio
> >> <israelh@microsoft.com>
> >> wrote:
> >> > On Wednesday, October 26, 2011 9:35 AM, Joshua Bell wrote:
> >> >>On Tue, Oct 25, 2011 at 4:50 PM, Israel Hilerio
> >> >><israelh@microsoft.com>
> >> wrote:
> >> >>>On Monday, October 24, 2011 7:40 PM, Jonas Sicking wrote:
> >> >>>>
> >> >>>> While I was there it did occur to me that the fact that the
> >> >>>> .delete function "returns" (through request.result in the async
> >> >>>> API) true/false depending on if any records were removed or not
> >> >>>> might be
> >> bad for performance.
> >> >>>>
> >> >>>> I suspect that this highly depends on the implementation and
> >> >>>> that in some implementations knowing if records were deleted
> >> >>>> will be free and in others it will be as costly as a .count()
> >> >>>> and then a .delete(). In yet others it could depend on if a
> >> >>>> range, rather than a key, was used, or if the objectStore has
> >> >>>> indexes which might need
> >> updating.
> >> >>>>
> >> >>>> Ultimately I don't have a strong preference either way, though
> >> >>>> it seems unfortunate to slow down implementations for what
> >> >>>> likely is a
> >> rare use case.
> >> >>>>
> >> >>>> Let me know what you think.
> >> >>>>
> >> >>>> / Jonas
> >> >>>>
> >> >
> >> >>>To clarify, removing the return value from the sync call would
> >> >>>change its
> >> return signature to void.  In this case, >>successfully returning
> >> from the IDBObjectStore.delete call would mean that the information
> >> was successfully
> >> >>deleted, correct?  If the information was not successfully deleted,
> >> >>would we
> >> throw an exception?
> >> >>>
> >> >>>In the async case, we would keep the same return value of
> >> >>>IDBRequest for
> >> IDBObjectStore.delete.  The only change is >>that the request.result
> >> would be null, correct?  If no information is deleted or if part of
> >> the keyRange data is deleted, >>should we throw an error event?  It seems
> reasonable to me.
> >> >
> >> >>When you write "If no information is deleted ... should we throw an
> >> >>error
> >> event?" do you mean (1) there was no >matching key so the delete was
> >> a no- op, or (2) there was a matching key but an internal error
> >> occurred preventing the >delete? I ask because the second clause, "if
> >> part of the keyRange data is deleted, should we throw an error
> >> event?" >doesn't make sense to me in interpretation (1) since I'd expect
> sparse ranges in many cases.
> >> >
> >> > I was originally referring to (1) and (2).  However, after
> >> > discussing this with
> >> a couple of folks we believe that the better approach would be to:
> >> > * continue to return true or false in the result.  This will take
> >> > care of (1) and
> >> the successful deletion of all records.
> >> > * publish an error event if (2).  What I meant by (2) is that if
> >> > there was a
> >> successful set of matches that were able to be returned by the
> >> keyRange, we should guarantee the deletion of all the matches or none.
> >> >
> >> > However, (2) brings up a bigger issue.  We are basically saying
> >> > that if we
> >> support deletion of keyRanges we are guaranteeing that the batch
> >> operation will all happen or none of it will happen.  This implies
> >> some type of inner- transaction associated only with the delete
> >> operation, which could also be rolledback as part of the
> >> outer-transaction.  Otherwise, you could potentially preventDefault
> >> on any record that failed to be deleted and have your database in some
> type of inconsistent state.  Was that the intent?
> >>
> >> This is already the case. For example when a inserting a value into
> >> an object store the implementation might need to go update several
> >> indexes. Updating one of these indexes might result in the violation
> >> of a 'unique' constraint at which point all changes to all indexes as
> >> well as the change to the object store must be rolled back. However
> >> no other changes done as part of the transaction should be rolled
> >> back (unless the resulting "error" event isn't canceled).
> >>
> >> This is required in step 7 of the "Steps for asynchronously executing
> >> a request" (though I now see that it's missing in the "Steps for
> >> synchronously executing a request").
> >>
> >> dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#steps-for-
> >> asynchronously-executing-a-request
> >>
> >> In the firefox implementation we create a "mini transaction" for each
> >> database request and if any part of the request fail we just roll
> >> back the "mini transaction".
> >
> > You are correct!  I forgot that we also do something similar.
> > So if we fail to remove any one record from the keyRange set, should we
> throw an InvalidStateError, UnknownError, other?
> 
> I think for failed reads/writes like that we should use UnknownError.
> InvalidStateError indicates a fault on the side of the web page which isn't the
> case here.
 
Sounds good!

> During our internal security review of IndexedDB we came to the conclusion
> that for IO errors we generally will not want to use more specific errors than
> UnknownError for fear of exposing sensitive information about the user's
> environment. At least unless there are clear use cases for it.
 
Make a lot of sense.

> >> >>In the async case, interpretation (1) matches Chrome's current behavior:
> >> success w/ null result if something was >deleted, error if there was
> >> nothing to delete. But I was about to land a patch to match the spec:
> >> success w/ true/false, >so this thread is timely.
> >> >
> >> > Our current implementation matches the spec, so we are okay keeping
> >> > it the
> >> way it is in the spec.  In either case, we need to figure out what to
> >> do when the partial deletion of keyRange records takes place.
> >>
> >> So you are not worried that this might make it impossible to do
> >> further optimizations in the future?
> >>
> >> Ultimately I'm ok either way. I don't have enough database experience
> >> to judge if we might loose out on important performance optimizations
> >> in the future. And I definitely think it's the case that if we make
> >> it return null then we'll force some people to do a .count() and then
> >> .delete() which is almost always slower.
> >>
> >> / Jonas
> >>
> >
> > We don't see this being a perf issue for our implementation.  While it could
> be an issue for some implementations, based on our experience we don't see
> this being a big perf hit.
> 
> Ok, if no-one has performance concerns about this (which somewhat
> surprises me), then let's leave the spec as-is. I.e. returning true/false
> indicating if any records were deleted.
> 
> Or should we return a count indicating how many records were deleted?
> 
> / Jonas

We believe count could potentially introduce a perf hit on the delete operation.  We see how this could have more dependencies on the backend DB.  While I can see how some users could care about the number of records that got deleted, our users/devs are more concerned about whether the operation was successful or not vs. the number of records that got deleted.

To that effect, we rather keep it as a Boolean.

Israel

Received on Friday, 28 October 2011 16:36:40 UTC