integration with sqlite for WebSQL

[Paweł Hajdan, Jr.]

Hi Blink developers!

It seems to me that removal of WebSQL at this time is still unlikely - please correct me if there is a chance for that.

The problem I'm trying to solve is that current implementation requires adding hacks to our bundled copy of sqlite that reach into os_unix.c's internals.

I was thinking about adding a custom VFS to Blink instead - a small and simple one as far as possible. I'm wondering what the requirements would be:

1. Is Blink going to call sqlite on different threads? It seems to me that it's the case, and a bug from 2011 confirms this (https://code.google.com/p/chromium/issues/detail?id=70589). But maybe it's changed.

2. What solution should be used for file locking on POSIX? POSIX locks (fcntl) have the following problems documented in os_unix.c:

** POSIX advisory locks are broken by design.  ANSI STD 1003.1 (1996)
** section 6.5.2.2 lines 483 through 490 specify that when a process
** sets or clears a lock, that operation overrides any prior locks set
** by the same process.  It does not explicitly say so, but this implies
** that it overrides locks set by the same process using a different
** file descriptor.  Consider this test case:
**
**       int fd1 = open("./file1", O_RDWR|O_CREAT, 0644);
**       int fd2 = open("./file2", O_RDWR|O_CREAT, 0644);
**
** Suppose ./file1 and ./file2 are really the same file (because
** one is a hard or symbolic link to the other) then if you set
** an exclusive lock on fd1, then try to get an exclusive lock
** on fd2, it works.  I would have expected the second lock to
** fail since there was already a lock on the file due to fd1.
** But not so.  Since both locks came from the same process, the
** second overrides the first, even though they were on different
** file descriptors opened on different file names.
**
** This means that we cannot use POSIX locks to synchronize file access
** among competing threads of the same process.  POSIX locks will work fine
** to synchronize access for threads in separate processes, but not
** threads within the same process.
**
** To work around the problem, SQLite has to manage file locks internally
** on its own.  Whenever a new database is opened, we have to find the
** specific inode of the database file (the inode is determined by the
** st_dev and st_ino fields of the stat structure that fstat() fills in)
** and check for locks already existing on that inode.  When locks are
** created or removed, we have to look at our own internal record of the
** locks to see if another thread has previously set a lock on that same
** inode.
**
** (Aside: The use of inode numbers as unique IDs does not work on VxWorks.
** For VxWorks, we have to use the alternative unique ID system based on
** canonical filename and implemented in the previous division.)
**
** The sqlite3_file structure for POSIX is no longer just an integer file
** descriptor.  It is now a structure that holds the integer file
** descriptor and a pointer to a structure that describes the internal
** locks on the corresponding inode.  There is one locking structure
** per inode, so if the same inode is opened twice, both unixFile structures
** point to the same locking structure.  The locking structure keeps
** a reference count (so we will know when to delete it) and a "cnt"
** field that tells us its internal lock status.  cnt==0 means the
** file is unlocked.  cnt==-1 means the file has an exclusive lock.
** cnt>0 means there are cnt shared locks on the file.
**
** Any attempt to lock or unlock a file first checks the locking
** structure.  The fcntl() system call is only invoked to set a 
** POSIX lock if the internal lock structure transitions between
** a locked and an unlocked state.
**
** But wait:  there are yet more problems with POSIX advisory locks.
**
** If you close a file descriptor that points to a file that has locks,
** all locks on that file that are owned by the current process are
** released.  To work around this problem, each unixInodeInfo object
** maintains a count of the number of pending locks on tha inode.
** When an attempt is made to close an unixFile, if there are
** other unixFile open on the same inode that are holding locks, the call
** to close() the file descriptor is deferred until all of the locks clear.
** The unixInodeInfo structure keeps a list of file descriptors that need to
** be closed and that list is walked (and cleared) when the last lock
** clears.


The above really complicates the implementation because it has to keep track of open file descriptors and inodes, duplicating a lot that the OS would otherwise do. A simpler alternative would be using flock, with the following drawbacks:

** flock() locking is like dot-file locking in that the various
** fine-grain locking levels supported by SQLite are collapsed into
** a single exclusive lock.  In other words, SHARED, RESERVED, and
** PENDING locks are the same thing as an EXCLUSIVE lock.  SQLite
** still works when you do this, but concurrency is reduced since
** only a single process can be reading the database at a time.


Even more context can be found at http://0pointer.de/blog/projects/locking.html

With this effort it should become easier to update the bundled copy of sqlite. It's currently at version 3.7.6.3 (released in May 2011, two years ago), while upstream is 3.7.16.2 (released in April 2013, about a week ago). For a full change log you can refer to http://sqlite.org/changes.html . I'm not saying we should necessarily update for every upstream minor release, but with a two-year lag every change from upstream makes it just harder to update.

Are there any objections about my plan to add the custom sqlite VFS to Blink that would replace custom modifications to sqlite?

Paweł

[Adam Barth]

On Mon, Apr 22, 2013 at 11:41 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Hi Blink developers!

It seems to me that removal of WebSQL at this time is still unlikely - please correct me if there is a chance for that.

That's correct.  Although we would prefer that web developers use IndexedDB instead of WebSQL, there are many existing users of WebSQL.

 

The problem I'm trying to solve is that current implementation requires adding hacks to our bundled copy of sqlite that reach into os_unix.c's internals.

What problem does that cause?  If you're worried about code health, there are much bigger code health problems to solve in WebSQL.

 

I was thinking about adding a custom VFS to Blink instead - a small and simple one as far as possible. I'm wondering what the requirements would be:

I'm not sure what you mean by a VFS.

 

1. Is Blink going to call sqlite on different threads? It seems to me that it's the case, and a bug from 2011 confirms this (https://code.google.com/p/chromium/issues/detail?id=70589). But maybe it's changed.

IMHO, Blink shouldn't call into sqlite at all.  The WebSQL code should be refactored to work more like the IndexedDB code, and the interaction with sqlite should take place in Content rather than Blink.

 

2. What solution should be used for file locking on POSIX? POSIX locks (fcntl) have the following problems documented in os_unix.c:

I'm not sure what to recommend here.  How does IndexedDB solve this problem?

 

With this effort it should become easier to update the bundled copy of sqlite. It's currently at version 3.7.6.3 (released in May 2011, two years ago), while upstream is 3.7.16.2 (released in April 2013, about a week ago). For a full change log you can refer to http://sqlite.org/changes.html . I'm not saying we should necessarily update for every upstream minor release, but with a two-year lag every change from upstream makes it just harder to update.

Is there are particular benefit to updating sqlite?  I put a big premium on not causing churn in WebSQL unless we're going to make an effort to bring its implementation quality up to par with the other features we ship.

Are there any objections about my plan to add the custom sqlite VFS to Blink that would replace custom modifications to sqlite?

I would prefer if we made decisions by consensus rather than by calls for objections.  Calling for objections doesn't seem like a friendly way to make progress in discussions.

Adam

 

[Alec Flett]

On Mon, Apr 22, 2013 at 11:55 AM, Adam Barth <aba...@chromium.org> wrote:

1. Is Blink going to call sqlite on different threads? It seems to me that it's the case, and a bug from 2011 confirms this (https://code.google.com/p/chromium/issues/detail?id=70589). But maybe it's changed.

IMHO, Blink shouldn't call into sqlite at all.  The WebSQL code should be refactored to work more like the IndexedDB code, and the interaction with sqlite should take place in Content rather than Blink.

+1

Ultimately I'd personally like to see this driven by security - if there are no real security risks with the current approach, that's fine - but if centralizing WebSQL allows chrome to lock down blink renderers more, then this architecture change is worthwhile.

 

 

2. What solution should be used for file locking on POSIX? POSIX locks (fcntl) have the following problems documented in os_unix.c:

I'm not sure what to recommend here.  How does IndexedDB solve this problem?

IndexedDB also runs entirely within the browser process, relying on leveldb's pluggable file locking (implemented in chromium_env.cc, in the chromium tree) - but leveldb already has this functionality. 

 

With this effort it should become easier to update the bundled copy of sqlite. It's currently at version 3.7.6.3 (released in May 2011, two years ago), while upstream is 3.7.16.2 (released in April 2013, about a week ago). For a full change log you can refer to http://sqlite.org/changes.html . I'm not saying we should necessarily update for every upstream minor release, but with a two-year lag every change from upstream makes it just harder to update.

Is there are particular benefit to updating sqlite?  I put a big premium on not causing churn in WebSQL unless we're going to make an effort to bring its implementation quality up to par with the other features we ship.

+1 The only benefit I can imagine is security fixes - beyond that I think it would not be worth the risk of breaking any web apps that may rely on sqlite bugs, especially as this may cause divergence between Blink and WebKit in a way that seems to benefit nobody. (and given that they're the only two engines to support WebSQL, and WebSQL progress is more or less frozen, it seems a shame to fragment them even further)

FWIW we've also talked about reducing chromium's actual dependencies on SQLLite, such as appcache's reliance on having sqllite embedded and instead using leveldb or some other abstraction on top of that... 

I would bet on the future: WebSQL is destined to be booted from Chromium at some point.. probably literally not for years, but someday. (Note this is a personal prediction, not a promise or commitment)

 

Alec

[Michael Nordman]

Keeping the SQL out of the main browser process is a feature.

Not running content provided SQL statements thru SQLite in the main browser process is a significant security oriented feature of the way WebSQL works. We poke holes in the sandbox to allow the renderer to read/write websql db filehandles directly to pull that off (yuck but big deal). IDB doesn't have a language interpreter in the backing store in quite the same way. Performing leveldb ops in the main browser process != running arbitrary SQL in the main browser process. 

Seeing as its slated to die (eventually), do we have a concensus to just let it lie until such time?

[Paweł Hajdan, Jr.]

On Mon, Apr 22, 2013 at 5:07 PM, Michael Nordman <mich...@google.com> wrote:

Seeing as its slated to die (eventually), do we have a concensus to just let it lie until such time?

Given that it's there since 2010, and the timeframe of removal is rather years, I'd like to improve how it interfaces with sqlite - nothing bigger than this, but still no "not touching it at all" also.

 

On Mon, Apr 22, 2013 at 12:11 PM, Alec Flett <alec...@chromium.org> wrote:

On Mon, Apr 22, 2013 at 11:55 AM, Adam Barth <aba...@chromium.org> wrote:

1. Is Blink going to call sqlite on different threads? It seems to me that it's the case, and a bug from 2011 confirms this (https://code.google.com/p/chromium/issues/detail?id=70589). But maybe it's changed.

IMHO, Blink shouldn't call into sqlite at all.  The WebSQL code should be refactored to work more like the IndexedDB code, and the interaction with sqlite should take place in Content rather than Blink.

+1

Ultimately I'd personally like to see this driven by security - if there are no real security risks with the current approach, that's fine - but if centralizing WebSQL allows chrome to lock down blink renderers more, then this architecture change is worthwhile.

 

 

2. What solution should be used for file locking on POSIX? POSIX locks (fcntl) have the following problems documented in os_unix.c:

I'm not sure what to recommend here.  How does IndexedDB solve this problem?

IndexedDB also runs entirely within the browser process, relying on leveldb's pluggable file locking (implemented in chromium_env.cc, in the chromium tree) - but leveldb already has this functionality. 

From my reading of code, env_chromium.cc uses sort of dotfile-locking, i.e. relies on O_EXCL on POSIX. This seems different from what sqlite does, because SQLite puts locks on existing files, and AFAIK O_EXCL only prevents open(2) from returning successfully if the file already exists.

Bottom line is that the IndexDB case doesn't help here, it's too different. Please correct me if I didn't read the code carefully enough.

Is there are particular benefit to updating sqlite?  I put a big premium on not causing churn in WebSQL unless we're going to make an effort to bring its implementation quality up to par with the other features we ship.

+1 The only benefit I can imagine is security fixes

Yes, and when there is a security fix wouldn't it be cool to just cleanly apply an upstream patch rather than have to merge it manually, potentially incorrectly?

I've heard a lot of opposition against people trying to backport Chromium security fixes, and we just say the only correct version of Chrome to use is the latest stable version, same for V8 - even though patches may apply cleanly to earlier versions, it's not obvious whether that fixes the issue completely for the older version, and whether there were some other issues fixed in the new version, that also happened to be security-related but haven't been marked as security bugs.

I'm at least a bit surprised a different approach is used for the third party libraries we use.

 

I would bet on the future: WebSQL is destined to be booted from Chromium at some point.. probably literally not for years, but someday. (Note this is a personal prediction, not a promise or commitment)

I know. See above though what I think about it: it's going to stick around long enough that I'd like to fix some things about it.

On Mon, Apr 22, 2013 at 11:55 AM, Adam Barth <aba...@chromium.org> wrote:

The problem I'm trying to solve is that current implementation requires adding hacks to our bundled copy of sqlite that reach into os_unix.c's internals.

What problem does that cause?  If you're worried about code health, there are much bigger code health problems to solve in WebSQL.

I'd like to learn about these bigger problems as well then (that doesn't necessarily mean I'd fix them, but it's definitely helpful to get the bigger picture).

If "adding hacks that reach into library's internals bypassing its public interface" doesn't make it obvious what are issues with that, here is what I'm trying to do:

1. Make it possible to use system sqlite.

2. Make it easier to update the bundled sqlite copy, keep it more in sync with upstream (less custom patching).

 

I was thinking about adding a custom VFS to Blink instead - a small and simple one as far as possible. I'm wondering what the requirements would be:

I'm not sure what you mean by a VFS.

Oops, right - it's an abstraction in SQLite for functions like open, read, write, etc. See http://www.sqlite.org/vfs.html , http://www.sqlite.org/c3ref/vfs.html and http://www.sqlite.org/c3ref/io_methods.html for more info.

 

1. Is Blink going to call sqlite on different threads? It seems to me that it's the case, and a bug from 2011 confirms this (https://code.google.com/p/chromium/issues/detail?id=70589). But maybe it's changed.

IMHO, Blink shouldn't call into sqlite at all.  The WebSQL code should be refactored to work more like the IndexedDB code, and the interaction with sqlite should take place in Content rather than Blink.

I think that'd be a step in the right direction. I could do that refactoring as a part of this effort.

 

Are there any objections about my plan to add the custom sqlite VFS to Blink that would replace custom modifications to sqlite?

I would prefer if we made decisions by consensus rather than by calls for objections.  Calling for objections doesn't seem like a friendly way to make progress in discussions.

I'm sorry if this sounded unfriendly - that was definitely not my intention.

Paweł 

[James Robinson]

On Tue, Apr 23, 2013 at 10:17 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I'd like to learn about these bigger problems as well then (that doesn't necessarily mean I'd fix them, but it's definitely helpful to get the bigger picture).

If "adding hacks that reach into library's internals bypassing its public interface" doesn't make it obvious what are issues with that, here is what I'm trying to do:

1. Make it possible to use system sqlite.

This isn't a goal of the project, so it's not sufficient motivation on its own to push a change.

 

2. Make it easier to update the bundled sqlite copy, keep it more in sync with upstream (less custom patching).

Do we actually sync with upstream sqlite in practice, or is this just a theoretical win?

- James

 

[Darin Fisher]

I wouldn't recommend supporting different versions of sqlite being used to power WebSQL DB.  There's a web compat issue here.  Unfortunately WebSQL DB implies certain behaviors and quirks of sqlite.  Using a different version of sqlite might incur some risk of breaking sites.

-Darin

[Paweł Hajdan, Jr.]

On Tue, Apr 23, 2013 at 10:21 AM, James Robinson <jam...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 10:17 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I'd like to learn about these bigger problems as well then (that doesn't necessarily mean I'd fix them, but it's definitely helpful to get the bigger picture).

If "adding hacks that reach into library's internals bypassing its public interface" doesn't make it obvious what are issues with that, here is what I'm trying to do:

1. Make it possible to use system sqlite.

This isn't a goal of the project, so it's not sufficient motivation on its own to push a change.

Please make sure to take big picture into account. The way sqlite is integrated into this is just hacky.

Using system libraries is one thing, but it also promotes good engineering practices.

Code health issues related to WebSQL have been mentioned in this thread. Same with moving integration with sqlite out of Blink to content. I could do these things, that I understand are good for the project in general.

Please avoid discouraging people from doing work useful in general that also happens to make the project FOSS-friendly. If I didn't mention using system sqlite, would you write the above?

 

2. Make it easier to update the bundled sqlite copy, keep it more in sync with upstream (less custom patching).

Do we actually sync with upstream sqlite in practice, or is this just a theoretical win?

http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/?view=log shows active development in third_party/sqlite . Scott is doing or reviewing most of that work, so he can tell for sure, but in general things are easier when code is not too far from upstream.

Some highlights:

https://chromiumcodereview.appspot.com/10387026 - this fixes an issue related to gcc4.7 - which we would hit at some point on Ubuntu upgrade; the author is a Linux packager and has contributed the fix way before this became an actual problem for us

https://codereview.chromium.org/6990047 mentions that "SQLite changes are going to be hard/impossible to review. It's a year

and a half of changes, might as well be a new package." We are even more behind now.

Paweł

[Adam Barth]

On Tue, Apr 23, 2013 at 4:55 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 10:21 AM, James Robinson <jam...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 10:17 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I'd like to learn about these bigger problems as well then (that doesn't necessarily mean I'd fix them, but it's definitely helpful to get the bigger picture).

If "adding hacks that reach into library's internals bypassing its public interface" doesn't make it obvious what are issues with that, here is what I'm trying to do:

1. Make it possible to use system sqlite.

This isn't a goal of the project, so it's not sufficient motivation on its own to push a change.

Please make sure to take big picture into account. The way sqlite is integrated into this is just hacky.

Using system libraries is one thing, but it also promotes good engineering practices.

Code health issues related to WebSQL have been mentioned in this thread. Same with moving integration with sqlite out of Blink to content. I could do these things, that I understand are good for the project in general.

Please avoid discouraging people from doing work useful in general that also happens to make the project FOSS-friendly. If I didn't mention using system sqlite, would you write the above?

I agree with James that making it possible to use the system sqlite shouldn't motivate us to make this change.

  

2. Make it easier to update the bundled sqlite copy, keep it more in sync with upstream (less custom patching).

Do we actually sync with upstream sqlite in practice, or is this just a theoretical win?

http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/?view=log shows active development in third_party/sqlite . Scott is doing or reviewing most of that work, so he can tell for sure, but in general things are easier when code is not too far from upstream.

Some highlights:

https://chromiumcodereview.appspot.com/10387026 - this fixes an issue related to gcc4.7 - which we would hit at some point on Ubuntu upgrade; the author is a Linux packager and has contributed the fix way before this became an actual problem for us

https://codereview.chromium.org/6990047 mentions that "SQLite changes are going to be hard/impossible to review. It's a year

and a half of changes, might as well be a new package." We are even more behind now.

That's why we're worried about web compatibility issues that might arise from updating to a new version.  The compatibility risks of updating sqlite outweigh the hygiene benefits of using the latest version.

Adam

[Dirk Pranke]

Several people have mentioned possible security fixes as the only real reason to want to change or upgrade sqlite. Presumably not tracking tip-of-tree sqlite will make merging security fixes harder (and means we'll have to actively be looking to merge security fixes from upstream). Are you saying that's what we should do (and maybe are already doing)? 

(I don't have a dog in this fight, just trying to follow the implications. It seems to me that normally we try to address compat concerns through outreach and writing tests, not through being afraid to change things, but I understand the desire to not spend a lot of time on WebSQL; maybe we should be actively finding out who is using WebSQL so we can remove it, if we aren't already?).

-- Dirk

[Adam Barth]

On Tue, Apr 23, 2013 at 6:40 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 5:07 PM, Adam Barth <aba...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 4:55 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 10:21 AM, James Robinson <jam...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 10:17 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I'd like to learn about these bigger problems as well then (that doesn't necessarily mean I'd fix them, but it's definitely helpful to get the bigger picture).

If "adding hacks that reach into library's internals bypassing its public interface" doesn't make it obvious what are issues with that, here is what I'm trying to do:

1. Make it possible to use system sqlite.

This isn't a goal of the project, so it's not sufficient motivation on its own to push a change.

Please make sure to take big picture into account. The way sqlite is integrated into this is just hacky.

Using system libraries is one thing, but it also promotes good engineering practices.

Code health issues related to WebSQL have been mentioned in this thread. Same with moving integration with sqlite out of Blink to content. I could do these things, that I understand are good for the project in general.

Please avoid discouraging people from doing work useful in general that also happens to make the project FOSS-friendly. If I didn't mention using system sqlite, would you write the above?

I agree with James that making it possible to use the system sqlite shouldn't motivate us to make this change.

  

2. Make it easier to update the bundled sqlite copy, keep it more in sync with upstream (less custom patching).

Do we actually sync with upstream sqlite in practice, or is this just a theoretical win?

http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/?view=log shows active development in third_party/sqlite . Scott is doing or reviewing most of that work, so he can tell for sure, but in general things are easier when code is not too far from upstream.

Some highlights:

https://chromiumcodereview.appspot.com/10387026 - this fixes an issue related to gcc4.7 - which we would hit at some point on Ubuntu upgrade; the author is a Linux packager and has contributed the fix way before this became an actual problem for us

https://codereview.chromium.org/6990047 mentions that "SQLite changes are going to be hard/impossible to review. It's a year

and a half of changes, might as well be a new package." We are even more behind now.

That's why we're worried about web compatibility issues that might arise from updating to a new version.  The compatibility risks of updating sqlite outweigh the hygiene benefits of using the latest version.

Several people have mentioned possible security fixes as the only real reason to want to change or upgrade sqlite. Presumably not tracking tip-of-tree sqlite will make merging security fixes harder (and means we'll have to actively be looking to merge security fixes from upstream). Are you saying that's what we should do (and maybe are already doing)? 

That's a good question from the security team.  Paweł hasn't said that there's a particular security fix that's been difficult to backport.  So far, all the reasons he's raised (with the exception of being able to use the system sqlite) have been theoretical concerns, not practical issues.

My belief is that he's primarily motivated by wanting to make it possible to use the system sqlite, which is why the other motivations he has advanced have been weak.  As I've (and others) have been clear about in this thread and in many other threads on this topic, being able to use the system versions of libraries is not a goal of the Chromium project.

(I don't have a dog in this fight, just trying to follow the implications. It seems to me that normally we try to address compat concerns through outreach and writing tests, not through being afraid to change things, but I understand the desire to not spend a lot of time on WebSQL; maybe we should be actively finding out who is using WebSQL so we can remove it, if we aren't already?).

WebSQL is an evolutionary dead end.  Other browser vendors and the W3C have rejected the API.  The best course of action for us is to evangalize users of WebSQL to move to IndexedDB.  Understandably, some developers are hesitant to move to IndexedDB until it's supported on iOS.

It's unlikely we're going to be able to drop support of WebSQL in the near (or even intermediate) term.  That leaves us in the unenviable position of having a bunch of legacy code to maintain.  I would support an effort to improve the implementation quality of the WebSQL code in Blink, particularly updating the structure of the code based on what we've learned from implementing other storage systems, but I don't support rolling the compatibility dice by updating sqlite.

Adam

[Scott Hess]

I apologize if my response seems out-of-touch - I tried getting up-to-sync with the thread, but I'm actively not paying attention to blink-dev ... I'm going to try real hard not to even look at the "Use system SQLite" side of things, I think others have covered that and I don't want to start ranting in public.

1) If there is any reason why we implicitly guarantee a specific version of SQLite, then we have a problem - if someone wanted specific behavior, they should have put a test on it.  As it is, I'm working on a CL to bring us to a more recent version of SQLite, completely unrelated to this thread.  [All that said, SQLite takes compatibility very seriously, I'd be surprised if anyone notices.  They didn't notice last time.]

1.1) Complexity of importing a new version of SQLite is irrelevant.  It's a good project, they do a good job, but it is in active development, so if you're doing an import every couple years, things will have changed.  But it usually only takes a few hours to prep the codebase for a new version, plus another few hours doing a broad review of things to make sure that the high-level syntax and other changes aren't likely to cause a problem.

2) The work I've been doing in third_party/sqlite is on a loadable module for corruption recovery which should be independent of SQLite core.  Testing that was specifically why I was put together the start of a CL to upgrade versions.  Once I have more experience with using the module, I'm going to campaign to get it pulled into SQLite core, but meanwhile even though it is a substantial amount of code, I think the changes to core SQLite are less invasive than many of our other changes.

3) I strongly disagreed with the form of the WebSQL changes to SQLite, but I was unable to convince the implementor to take the VFS approach.  Their arguments at the time weren't without merit, we just didn't hold the same values as primary, and now the implementor has moved on and the feature is deprecated.  Since then, I believe someone added a proxy module (or example) to SQLite, which may provide a means to accomplish most of what we would need less invasively, and I would support experimenting with it.  Or, worst-case, I don't think a VFS should really be that hard.  BUT, I would not support barging off with the final goal set in concrete.  There is an existing system in place, so we can no longer change the spec to fit implementation difficulties.  It's possible that reimplementing as a VFS will result in a horrible result with dozens of hacks to workaround problems, and I don't think we should commit to that goal up front for any of the reasons presented in this thread.  That said, someone should be able to put together a proof of concept relatively easily, in spite of the churn in the codebase.  Worst case, just lay down a dedicated git client and don't sync things while proving that it's an awesome alternative.  If you need me to review something for commentating purposes, I can setup a parallel client and we can pull changes across manually using git-format-patch and git-am.

Basically, I don't think this is hardly worth putting it before the central committee.  If the code makes a good argument, that's great, but I don't think the negatives of the WebSQL/SQLite integration warrant committing to a refactor just because the current solution is gross.

-scott

[Michael Nordman]

On Tue, Apr 23, 2013 at 8:50 PM, Scott Hess <sh...@chromium.org> wrote:

I apologize if my response seems out-of-touch - I tried getting up-to-sync with the thread, but I'm actively not paying attention to blink-dev ... I'm going to try real hard not to even look at the "Use system SQLite" side of things, I think others have covered that and I don't want to start ranting in public.

1) If there is any reason why we implicitly guarantee a specific version of SQLite, then we have a problem - if someone wanted specific behavior, they should have put a test on it.  As it is, I'm working on a CL to bring us to a more recent version of SQLite, completely unrelated to this thread.  [All that said, SQLite takes compatibility very seriously, I'd be surprised if anyone notices.  They didn't notice last time.]

1.1) Complexity of importing a new version of SQLite is irrelevant.  It's a good project, they do a good job, but it is in active development, so if you're doing an import every couple years, things will have changed.  But it usually only takes a few hours to prep the codebase for a new version, plus another few hours doing a broad review of things to make sure that the high-level syntax and other changes aren't likely to cause a problem.

2) The work I've been doing in third_party/sqlite is on a loadable module for corruption recovery which should be independent of SQLite core.  Testing that was specifically why I was put together the start of a CL to upgrade versions.  Once I have more experience with using the module, I'm going to campaign to get it pulled into SQLite core, but meanwhile even though it is a substantial amount of code, I think the changes to core SQLite are less invasive than many of our other changes.

To ask a clarifying question, are the changes in support of websql specifically in the more or less invasive category compared to 'many of our other changes'? Sounds like 'less' but just wanting to pin that down.

[Scott Hess]

On Wed, Apr 24, 2013 at 1:20 PM, Michael Nordman <mich...@google.com> wrote:

To ask a clarifying question, are the changes in support of websql specifically in the more or less invasive category compared to 'many of our other changes'? Sounds like 'less' but just wanting to pin that down.

I think the websql changes are a pretty gross hack to inject additional APIs into SQLite for the websql stuff to use.  The changes hack internal SQLite stuff which most definitely shouldn't be exposed.

-scott

 

[Paweł Hajdan, Jr.]

Thank you Scott for pointing out the hacky way sqlite is currently integrated with Blink.

Here's my attempt at summarizing this thread so far:

1. Current design of WebSQL in Blink is a hack (confirmed by Scott Hess). An alternative, cleaner design has been proposed before - using a sqlite VFS: http://www.sqlite.org/vfs.html

2. Blink shouldn't know about sqlite at all (said by Adam Barth). This fits well with porting at the content layer.

3. There are much bigger code health issues in WebSQL (said by Adam Barth). I'm not sure if that refers to #2 above or something else.

4. Concerns have been raised about updating bundled sqlite having impact on WebSQL compatibility. It looks like our copy of sqlite will be updated anyway.

5. Given that the bundled copy of sqlite is updated very rarely, it seems people don't see much benefit in making it easier to update by having less custom changes to it.

Adam Barth said:

I would support an effort to improve the implementation quality of the WebSQL code in Blink, particularly updating the structure of the code based on what we've learned from implementing other storage systems

I'd like to move this effort forward. Let's put the issue of "system sqlite" aside for a moment. There apparently are issues to be fixed here that are just good for the health of the project.

Adam, do you have some specific recommendations about what to do with current WebSQL design in Blink to make it better? Would you have any hints about how usage of sqlite should be moved out of Blink to content? Are there other things that could be improved as well?

Paweł

[Adam Barth]

On Mon, Apr 29, 2013 at 10:03 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Thank you Scott for pointing out the hacky way sqlite is currently integrated with Blink.

Here's my attempt at summarizing this thread so far:

1. Current design of WebSQL in Blink is a hack (confirmed by Scott Hess). An alternative, cleaner design has been proposed before - using a sqlite VFS: http://www.sqlite.org/vfs.html

2. Blink shouldn't know about sqlite at all (said by Adam Barth). This fits well with porting at the content layer.

3. There are much bigger code health issues in WebSQL (said by Adam Barth). I'm not sure if that refers to #2 above or something else.

4. Concerns have been raised about updating bundled sqlite having impact on WebSQL compatibility. It looks like our copy of sqlite will be updated anyway.

5. Given that the bundled copy of sqlite is updated very rarely, it seems people don't see much benefit in making it easier to update by having less custom changes to it.

Adam Barth said:

I would support an effort to improve the implementation quality of the WebSQL code in Blink, particularly updating the structure of the code based on what we've learned from implementing other storage systems

I'd like to move this effort forward. Let's put the issue of "system sqlite" aside for a moment. There apparently are issues to be fixed here that are just good for the health of the project.

Adam, do you have some specific recommendations about what to do with current WebSQL design in Blink to make it better? Would you have any hints about how usage of sqlite should be moved out of Blink to content? Are there other things that could be improved as well?

Yeah.  There shouldn't be a database thread in Blink.  The way this thread is managed is very error prone.  For example, there are many RefCountedThreadSafe objects whose destructors need to run on specific threads.  That approach is very fragile because adding a new reference to the object can change which thread the destructor runs on.

Instead, all the WebSQL code in Blink should run on the JavaScript thread (either the main thread or the worker thread depending on whether we're dealing with a worker).  The code should then call through the Platform API into Chromium.  On the embedder side, we can either talk to SQLite in the render process of via IPC.  (It sounds like Michael would prefer an in-process implementation.)  Either way, the decision should be invisible to Blink.

Make this change is a substantial amount of work.  It will basically involve a re-write of a large portion of the WebSQL implementation because when we move the threaded parts to the Chromium side, we'll want to re-write them to use the excellent threading primitives available in Chromium (which is actually the main benefit of this change).

Adam

[Michael Nordman]

On Mon, Apr 29, 2013 at 10:14 AM, Adam Barth <aba...@chromium.org> wrote:

On Mon, Apr 29, 2013 at 10:03 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Thank you Scott for pointing out the hacky way sqlite is currently integrated with Blink.

Here's my attempt at summarizing this thread so far:

1. Current design of WebSQL in Blink is a hack (confirmed by Scott Hess). An alternative, cleaner design has been proposed before - using a sqlite VFS: http://www.sqlite.org/vfs.html

fyi: the current design does utilize a vfs

 

2. Blink shouldn't know about sqlite at all (said by Adam Barth). This fits well with porting at the content layer.

3. There are much bigger code health issues in WebSQL (said by Adam Barth). I'm not sure if that refers to #2 above or something else.

4. Concerns have been raised about updating bundled sqlite having impact on WebSQL compatibility. It looks like our copy of sqlite will be updated anyway.

5. Given that the bundled copy of sqlite is updated very rarely, it seems people don't see much benefit in making it easier to update by having less custom changes to it.

Adam Barth said:

I would support an effort to improve the implementation quality of the WebSQL code in Blink, particularly updating the structure of the code based on what we've learned from implementing other storage systems

I'd like to move this effort forward. Let's put the issue of "system sqlite" aside for a moment. There apparently are issues to be fixed here that are just good for the health of the project.

Adam, do you have some specific recommendations about what to do with current WebSQL design in Blink to make it better? Would you have any hints about how usage of sqlite should be moved out of Blink to content? Are there other things that could be improved as well?

Yeah.  There shouldn't be a database thread in Blink.  The way this thread is managed is very error prone.  For example, there are many RefCountedThreadSafe objects whose destructors need to run on specific threads.  That approach is very fragile because adding a new reference to the object can change which thread the destructor runs on.

Instead, all the WebSQL code in Blink should run on the JavaScript thread (either the main thread or the worker thread depending on whether we're dealing with a worker).  The code should then call through the Platform API into Chromium.  On the embedder side, we can either talk to SQLite in the render process of via IPC.  (It sounds like Michael would prefer an in-process implementation.)  Either way, the decision should be invisible to Blink.

Make this change is a substantial amount of work.  It will basically involve a re-write of a large portion of the WebSQL implementation because when we move the threaded parts to the Chromium side, we'll want to re-write them to use the excellent threading primitives available in Chromium (which is actually the main benefit of this change).

> Makin this change is a substantial amount of work

Yes re-factor, re-vamp, re-form, re-write, whatever you want to call it , would be a considerable amount of work.  Redoing websql and reworking the vfs to be hermetic are two largely independent things, either of which could be done w/o doing to the other. Both could make sense to do, but at some cost. The question is... are the costs worth it?