Re: [IndexedDB] Atomic schema changes

On Sun, Jun 27, 2010 at 8:27 AM, Jonas Sicking <jonas@sicking.cc> wrote:

> 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.
>

Lets start another thread and examine this.  If all the errors can pretty
clearly/intuitively be broken into two groups, then maybe this does make
sense.  (I'll try to do it later today unless you beat me to it.)


> >> >> > 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
>