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

On Sunday, November 06, 2011 4:14 PM, Jonas Sicking wrote:
> On Fri, Oct 28, 2011 at 9:55 AM, Jonas Sicking <jonas@sicking.cc> wrote:
> > On Fri, Oct 28, 2011 at 9:29 AM, Israel Hilerio <israelh@microsoft.com>
> wrote:
> >> 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.
> >>>
> >>> 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.
> >>>
> >>> >> >>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.
> >
> > Hmm.. why would getting an exact count when deleting a range be more
> > expensive than getting an exact count when deleting a single key (the
> > latter being required in order to return true/false as currently
> > specified)?
> >
> > This isn't so much an argument for returning a count as it is an
> > argument for not returning true/false :)
> 
> Based on discussions at TPAC, it sounded like microsoft were fine with
> dropping the return value here and preferred to return undefined in all
> situations for delete. If so, I'll go ahead and change the spec to that effect.
> 
> Let me know if this is an ok interpretation. And also speak up if anyone
> disagrees.
> 
> / Jonas

Yes, we are okay with this approach.

Israel

Received on Monday, 7 November 2011 17:30:54 UTC