Re: [IndexedDB] Atomic schema changes

On Sat, Jun 26, 2010 at 11:44 AM, Jeremy Orlow <jorlow@chromium.org> wrote:
> On Sat, Jun 26, 2010 at 10:09 AM, Jonas Sicking <jonas@sicking.cc> wrote:
>>
>> On Fri, Jun 25, 2010 at 2:43 AM, Jeremy Orlow <jorlow@chromium.org> wrote:
>> >> > I'm OK with making createObjectStore/createIndex synchronous.  It
>> >> > would
>> >> > definitely make such code cleaner and I don't see a major downside,
>> >> > but
>> >> > at
>> >> > the same time I feel like this API is starting to get kind of ad-hoc
>> >> > and
>> >> > unintuitive to a new user.  Having the create and the remove
>> >> > functions
>> >> > completely different and in different places seems weird.
>> >> > So I agree with either A or leaving things as originally proposed by
>> >> > Jonas
>> >> > in "[IndexDB] Proposal for async API changes".
>> >>
>> >> Well, there's no reason not to make the same changes to
>> >> removeObjectStore/removeIndex. Though the more I think with this
>> >> design createObjectStore and removeObjectStore can stay on IDBDatabase
>> >> and simply throw if called outside the appropriate transaction
>> >> callback.
>> >>
>> >> Here is the revised proposal:
>> >>
>> >> interface IDBDatabase {
>> >>    ...
>> >>    IDBObjectStore createObjectStore (in DOMString name, in optional
>> >> DOMString keyPath, in optional boolean autoIncrement);
>> >>    void removeObjectStore (in DOMString storeName);
>> >>    ...
>> >> };
>> >>
>> >> interface IDBObjectStore {
>> >>    ...
>> >>    IDBIndex createIndex (in DOMString name, in DOMString keyPath, in
>> >> optional boolean unique);
>> >>    IDBRequest removeIndex (in DOMString indexName);
>> >>    ...
>> >> };
>> >>
>> >> Where createObjecStore/createIndex throws if a objectStore or index of
>> >> the given name already exists, if the keyPath has an invalid syntax,
>> >> or if the function is called when not inside a version-change
>> >> transaction callback. And removeObjectStore/removeIndex throws if the
>> >> objectStore or index doesn't exist or if the function is called when
>> >> not inside a version-change transaction callback.
>> >>
>> >> Throwing if not inside the right type of callback isn't a super clean
>> >> solution, but is really not that different from how add/put throws if
>> >> called when not inside a READ_WRITE transaction.
>> >
>> > Just to be clear, no async function (one that returns an IDBRequest)
>> > should
>> > _ever_ throw.  (They should only call onerror.)  I assume you didn't
>> > mean
>> > "add/put throws" literally?
>>
>> Well, there's no reason we couldn't make add/put throw if called from
>> inside the wrong type of transaction. I guess I don't feel strongly
>> about it, but it seems to me that calling put from inside a READ_ONLY
>> transaction is a much bigger mistake than writing a duplicate key or
>> running out of disc space. Additionally it's a situation where we
>> synchronously know that there is a bug in the program.
>
> When this came up before, the thought was that it'd be confusing to web
> developers which errors would raise immediately and which would call the
> error callback.  My feeling is that consistency is more useful than raising
> right away for errors like this.  (Plus, if we find it hard to get web
> developers to do any error checking, it seems even harder to get them to do
> it in 2 ways.  :-)

Well, I wouldn't expect them to check for put-in-wrong transaction,
and thus have that error bubble up and become a top level error, which
is handled through the usual error reporting mechanisms which should
alert the developer of a bug in their code. So it's actually
intentional that this error is handled different from other errors.

But if everyone else feels otherwise I'll shut up and accept that strategy.

>> >> > But, I feel pretty strongly that a setVersion/schema change
>> >> > transaction
>> >> > should not simply kill off anything else currently running.  The
>> >> > reason
>> >> > is
>> >> > that it's not hard for apps to recover from a connection failing, but
>> >> > it
>> >> > is
>> >> > hard to handle a transaction failing in an unpredictable way.
>> >> >  Especially
>> >> > static transactions (which should rarely fail committing since
>> >> > serialization
>> >> > can be guaranteed before the transaction starts).
>> >>
>> >> That might be a good idea for v1. I was planning on doing a separate
>> >> thread for setVersion, but maybe it's tied enough to the topic of
>> >> schema changes that it makes sense to bring up here.
>> >>
>> >> What I suggest is that when setVersion is called, we fire
>> >> 'versionchange' event on all other open IDBDatabase objects. This
>> >> event contains information of what the desired new version number is.
>> >> If no other IDBDatabase objects are open for the specific database, no
>> >> 'versionchange' events are fired. This allows pages using the old
>> >> schema version to automatically save any pending data (for example any
>> >> draft emails) and display UI to the user suggesting that the tab be
>> >> closed. If possible without dataloss, the tab could even reload itself
>> >> to automatically load an updated version of the page which uses the
>> >> new schema version.
>> >>
>> >> The 'versionchange' event would use an interface like
>> >>
>> >> interface IDBVersionEvent : IDBEvent {
>> >>  readonly attribute string version;
>> >> };
>> >
>> > First of all, what I was originally advocating (sorry for not being
>> > clear)
>> > is that we should kill the database connection but not until all active
>> > transactions are complete.  Though we should probably block new
>> > transactions
>> > from starting once setVersion is called.
>> > But I really like your versionchange event idea regardless.  I agree
>> > that
>> > letting the app sync any data that might be in memory (for example, a
>> > draft
>> > email) is important.  And the idea that the web app could refresh itself
>> > (or
>> > download new application code or something) seems pretty cool and
>> > useful.
>> >  I'm fine with it firing on all frames except the one that initiated
>> > (like
>> > storage events).  If we go with the "kill the connection once all active
>> > transactions are done and block new ones from starting", we'd want to
>> > start
>> > the blocking only after all versionchange events have finished.
>> > The main reason that I like the idea of not stating the version change
>> > until
>> > all active connections have closed is that not all apps will handle
>> > versionchange.  My original idea was that we should just break such web
>> > apps
>> > and let the user refresh, but now that you've pointed out the potential
>> > for
>> > data loss I'm not sure that's an option.  Savvy web apps can kill all
>> > existing database connections when they get the versionchange and thus
>> > avoid
>> > stalling things.
>> >
>> >> Additionally, if there are open IDBDatabase objects, we fire a
>> >> 'blocked' event at the IDBRequest object returned from the setVersion
>> >> call. This allows the page to display UI to the user asking the user
>> >> to close all other relevant tabs.
>> >>
>> >> Once all other IDBDatabase objects are closed, we create a transaction
>> >> and fire the normal 'success' event.
>> >>
>> >> While there are pending version change requests, no success events are
>> >> fired for calls to IDBFactory.open for the relevant database.
>> >>
>> >> We might want to support forcefully killing off open IDBDatabase
>> >> objects in the future, but I think that can wait until after v1.
>> >
>> > Really?  I can't see an app like gmail ever asking users to close tabs.
>> >  I
>> > bet they'd sooner run all the application logic in an iframe and
>> > navigate it
>> > away when doing a schema change.
>> > And I don't see many people correctly implementing a blocked event
>> > handler.
>> >  If anything, it should be an error code.
>> > It doesn't seem that hard to have an explicit way to tell the database
>> > explicitly "OK, I'm done".  Or, at very least, we could make it so that
>> > when
>> > there's an existing setVersion happening, all new connection requests
>> > stall.
>> >  That way all pages can reload themselves but they won't connect to the
>> > database until the upgrade is complete.
>> > But really...all of this is really hacky.  I'm starting to wonder if we
>> > should just kill the database connections on a setVersion as I
>> > originally
>> > tried to suggest.
>>
>> I'm pretty concerned though that sites will need to take asynchronous
>> actions in order to save all required data. While gmail happily
>> automatically saves every few minutes, and presumably could
>> immediately do so upon a 'versionchange' event, I don't think all
>> editors are willing t. For example many editors ask the user if they
>> want to save the current changes when they are closed, in order to not
>> overwrite correct data.
>>
>> Additionally, there is always the risk that developers will forget to
>> use a versionchange event handler to protect their data. I think a
>> good design principal is that if sites do the minimal amount of work,
>> that should default to safe behavior.
>>
>> I do realize that not all applications are willing to do the "please
>> close all other tabs" UI thing. But for those we would provide enough
>> tools do something better. If we add a IDBDatabase.close() function
>> then applications that can easily emulate the "force-close open
>> connections" using that and the versionchange event. And they could
>> even use that to implement asynchronous data saving for when that is
>> required.
>
> Yeah.  I think giving developers a .close() (or maybe even a
> .closeOthers()?) should be enough.  It still feels clunky to me, but I also
> think it'd be dangerous to over-design this without some feedback from
> several developers using this in the real world...which won't happen until
> we ship something (even if its basic).

Ok, I'll file a bug.

/ Jonas

Received on Sunday, 27 June 2010 07:34:48 UTC