Return number of records deleted

[inexorabletash]

Requested enhancement: allow some means to determine how many records were deleted in an operation

The IDBObjectStore.delete(query) operation is specified to yield undefined. Getting the count would be useful.

Imported from https://www.w3.org/Bugs/Public/show_bug.cgi?id=22130

[inexorabletash]

The current spec allows implementations to optimize the behavior of delete(), e.g. avoiding read-back of the records. It's unclear if that optimization is actually used in practice.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=22130 suggests allowing IDBObjectStore.clear(query) to take an optional query and have clear() be a fast-path, and have delete() yield a count.

[aliams]

Should we be limiting this to just deletions? Perhaps we can make a generalized method to return all affected entries in a previous add, put, or delete operation? I am concerned that yielding a count for deletions would make it seem inconsistent with the other methods of manipulation that would not yield counts.

[inexorabletash]

Interesting thought.

add() already signals with success or failure. put() won't tell you if you're overwriting, just success/failure (on constraint). The original w3c bug may point to more detailed use cases as to why this is desired for delete(), beyond "why not result in a count rather than useless undefined?"

[bevis-tseng]

FYI,
Performance concern and possible solution was mentioned in https://lists.w3.org/Archives/Public/public-webapps/2013JanMar/0871.html

[dmurph]

We saw negligible performance impact from the change based on our perf tests.

[sicking]

I'm surprised that is the case. Don't you guys just add an entry to an in-memory table when removing entries? And only once the transaction is committed write the deletion to LevelDB?

Whereas if you need to return a count, you'd obviously need to go to LevelDB to read a count of the number of entries in the deleted range.

That said, I'm really not particularly opinionated here. I'll just note that people tend to use whatever function meets their need and has the shortest name. So if there is a performance difference in the future, it's better to introduce deleteWithCount() now, than add fastDelete() later since fastDelete() would likely get very little usage.

[dmurph]

The way we currently delete, we iterate over a union of our 'pending' changes and the current database entries, and either update our pending change or create a new pending change with the status of 'deleted'. So all we need to do (for our implementation) is count the number of items where we either created a new 'pending' entry, or if there was already a pending entry that wasn't marked as deleted.

So we're basically adding an if statement and incrementing a counter.

[sicking]

And you're fine with being locked into that implementation strategy?

[dmurph]

Maybe. I think a general affected_entries or rows_affected feature would be more powerful, so maybe somehow optionally enabling that on a per-transaction basis would be cool. I'm not sure how that would look though.

[inexorabletash]

We've talked (internally) about a smarter approach where a range delete is covered by a single record rather than iterating, which is one of the reasons we pushed back against this suggestion before. But we wanted to float the idea if other implementers were strongly in favor of it.

[sicking]

I definitely don't feel strongly either way. But I would lean towards having an explicit delete-and-count function. Would be interesting to get feedback from the other IDB implementations.

[beidson]

We would definitely not want this to be the default.

[aliams]

We prefer that this is not the default behavior as well.

[inexorabletash]

Got it - not the default. Are there preferences between these approaches:

1. store.delete(range, {count: true});
2. store.deleteWithCount(range);
3. Other?

[sicking]

I don't have an opinion either way. 2 has fewer characters, but 1 is more expandable in the future if that matters.

[aliams]

For 1, I suppose the real question is if we actually see ourselves expanding the delete method in the future in some other way. On the other hand, I feel like the usage of 2 would be a little weird since the method signature tells you the action it will take as well as what it would return.

As a side note, if the deletion transaction is indeed guaranteed, how would it be any different from first getting the count (store.count(range)) and then deleting (store.delete(range))? Perhaps I am missing something?

[beidson]

I'm very familiar with verbose APIs like what you're going for with #2 (Objective-C, Cocoa-style) and I don't think there's a place for them in a context like this.

From an API naming perspective, "deleteWithCount" sounds much more like the function will also take a "count" argument.

I was already thinking #1 was the way to go.

A options object with "currently allows only one option" is not a problem. I wonder if "count:" is descriptive enough - It feels a little lightweight to me, though I don't have a better alternative ATM.

[inexorabletash]

Agreed, count as an option name is unclear. It could be interpreted as "delete at most count records". I also don't have a non-terrible alternative, though.

[sicking]

deleteAndCount or countAndDelete are possible options.

[inexorabletash]

I'm not a fan of repeating "delete" in the option name. countRecords ?