upstream suggestions about our sqlite vfs usage

[Paweł Hajdan, Jr.]

Currently we are adding some chromium_sqlite3 functions to our bundled sqlite copy to allow the renderer to use file descriptors it gets from the browser process via ChromiumBridge.

When we try to compile Chrome with a system-provided sqlite (Linux distributions want to do that), the linking fails because the system sqlite does not have those chromium_sqlite3 functions.

I asked sqlite upstream for suggestions how to solve it in the cleanest way possible. We need some symbol definitions that are only visible (static) in os_unix.c. D. Richard Hipp suggested that we copy os_unix.c file to say os_chromium.c, and make our modifications in that file. Please note that we are already forking os_unix.c by adding those functions, and upstream claims os_unix.c is rarely modified, and that they consider it the cleanest solution to the problem.

For more information please see my discussion with upstream:

http://www.mail-archive.com/sqlite...@sqlite.org/msg54926.html

http://www.mail-archive.com/sqlite...@sqlite.org/msg54929.html

What do you think?

[Dumitru Daniliuc]

in general, i'm not opposed to making chromium work with the standard sqlite library, but i think there are a few things to take into account:

1. does the standard library come with the fts2, fts3, and icu extensions compiled in? we need those modules.

2. copying os_unix.c to os_chromium_unix.c: our custom VFS has < 200 lines right now. os_unix.c has 5448 lines. that's quite a few extra lines of duplicated code that might need patching at some point.

3. where do we put os_chromium_unix.c: our VFS currently lives in webcore, and i think that's the right place for it, because all features that use it are in webcore. if we decide to keep it in webcore, some non-chromium webkit developers might strongly object to duplicating 5K+ lines of code (which doesn't even follow webkit's coding style). if we move it to chromium's repository, then we'll have code somewhere in chrome/renderer/* that registers a VFS that nobody (in chromium code) uses -- doesn't sound ideal to me.

4. other patches we applied to sqlite: we have a few patches we have applied to sqlite. some of them are small and could probably be upstreamed fairly easily. others should be upstream-able, but are big (fts3.patch is a 2K-line patch for a 7K-line file) and would probably take a considerable amount of time to upstream. and some of them will probably never be accepted upstream (os_symbian.c, building sqlite with fts2 and fts3 support at the same time, and probably a few other minor changes we needed).

5. future patches: as far as i know, only a small number of people can make changes to sqlite. so switching to using the system sqlite library could greatly reduce our ability to apply patches to sqlite in the future. we would also have to wait for those changes to make it into a sqlite release that would be available on all linux distributions we support. (we probably have the same problem already with other packages we depend on, but i don't think it's a good reason to add one more to the list).

dumi

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Paweł Hajdan, Jr.]

On Tue, Aug 31, 2010 at 18:20, Dumitru Daniliuc <du...@chromium.org> wrote:

in general, i'm not opposed to making chromium work with the standard sqlite library, but i think there are a few things to take into account:

Right, I think those are good points.

 

1. does the standard library come with the fts2, fts3, and icu extensions compiled in? we need those modules.

I'm not sure. Linux distros of course have control over which extensions get compile in, but enabling fts2 and fts3 simultaneously might require some non-trivial patches. That would be the next step then.

 

2. copying os_unix.c to os_chromium_unix.c: our custom VFS has < 200 lines right now. os_unix.c has 5448 lines. that's quite a few extra lines of duplicated code that might need patching at some point.

Yes, I wouldn't be happy about that. I'm going to discuss it with SQLite developers some more.

 

3. where do we put os_chromium_unix.c: our VFS currently lives in webcore, and i think that's the right place for it, because all features that use it are in webcore. if we decide to keep it in webcore, some non-chromium webkit developers might strongly object to duplicating 5K+ lines of code (which doesn't even follow webkit's coding style). if we move it to chromium's repository, then we'll have code somewhere in chrome/renderer/* that registers a VFS that nobody (in chromium code) uses -- doesn't sound ideal to me.

I was thinking about putting it in the chromium repo. It wouldn't be much different from the current situation.

 

4. other patches we applied to sqlite: we have a few patches we have applied to sqlite. some of them are small and could probably be upstreamed fairly easily. others should be upstream-able, but are big (fts3.patch is a 2K-line patch for a 7K-line file) and would probably take a considerable amount of time to upstream. and some of them will probably never be accepted upstream (os_symbian.c, building sqlite with fts2 and fts3 support at the same time, and probably a few other minor changes we needed).

If it works without our patches, and Linux distros like it, I think that's fine.

 

5. future patches: as far as i know, only a small number of people can make changes to sqlite. so switching to using the system sqlite library could greatly reduce our ability to apply patches to sqlite in the future. we would also have to wait for those changes to make it into a sqlite release that would be available on all linux distributions we support. (we probably have the same problem already with other packages we depend on, but i don't think it's a good reason to add one more to the list).

We wouldn't switch. It's just about providing an option for Linux distributions that package Chromium (not Google Chrome). Please also note that being able to compile and work with vanilla sqlite is a good metric of how much forked is our copy.

[Scott Hess]

On Wed, Sep 1, 2010 at 5:46 PM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> On Tue, Aug 31, 2010 at 18:20, Dumitru Daniliuc <du...@chromium.org> wrote:
>> 1. does the standard library come with the fts2, fts3, and icu extensions
>> compiled in? we need those modules.
>
> I'm not sure. Linux distros of course have control over which extensions get
> compile in, but enabling fts2 and fts3 simultaneously might require some
> non-trivial patches. That would be the next step then.

Distros should not be enabling fts2 because people should not be using
it at all. We're using it due to a historical accident.

For our history database, we could provide a custom version to use
with both the system SQLite and our SQLite. It's annoying, but
doable. It's also barely possible that with certain assumptions about
our usage, we could register the fts3.c code as the fts2 driver
Equivalently, we could do brain surgery on the sqlite_master table
directly.

[I do not really like those options, and would kind of like to see a
better justification for doing this than "Distros prefer the system
SQLite". It's an embedded database library, not a system-provided
service.]

>> 2. copying os_unix.c to os_chromium_unix.c: our custom VFS has < 200 lines
>> right now. os_unix.c has 5448 lines. that's quite a few extra lines of
>> duplicated code that might need patching at some point.
>
> Yes, I wouldn't be happy about that. I'm going to discuss it with SQLite
> developers some more.

I think drh's point that it doesn't matter is a reasonable point.
It's not like we're having to develop that code. If you're concerned
about missing out on bug-fixes or something, you could perhaps branch
the file from the original in third_party/sqlite, and then modify the
shell script in third_party/sqlite to down-integrate any changes to
the original (sorry, I know how I'd accomplish this with perforce, but
I don't know about svn).

BTW, it should not be named os_chromium_unix.c, it is a much more
specific usage than that. Our other uses of SQLite are in the
browser, this one's in the renderer.

>> 3. where do we put os_chromium_unix.c: our VFS currently lives in webcore,
>> and i think that's the right place for it, because all features that use it
>> are in webcore. if we decide to keep it in webcore, some non-chromium webkit
>> developers might strongly object to duplicating 5K+ lines of code (which
>> doesn't even follow webkit's coding style). if we move it to chromium's
>> repository, then we'll have code somewhere in chrome/renderer/* that
>> registers a VFS that nobody (in chromium code) uses -- doesn't sound ideal
>> to me.
>
> I was thinking about putting it in the chromium repo. It wouldn't be much
> different from the current situation.

Maybe it belongs in third_party/sqlite/?

>> 4. other patches we applied to sqlite: we have a few patches we have
>> applied to sqlite. some of them are small and could probably be upstreamed
>> fairly easily. others should be upstream-able, but are big (fts3.patch is a
>> 2K-line patch for a 7K-line file) and would probably take a considerable
>> amount of time to upstream. and some of them will probably never be accepted
>> upstream (os_symbian.c, building sqlite with fts2 and fts3 support at the
>> same time, and probably a few other minor changes we needed).
>
> If it works without our patches, and Linux distros like it, I think that's
> fine.

os_symbian.c is only there because of unifying the Chromium SQLite
with the Gears SQLite. Since we're unlikely to run Chromium on
symbian, we can get rid of it.

>> 5. future patches: as far as i know, only a small number of people can
>> make changes to sqlite. so switching to using the system sqlite library
>> could greatly reduce our ability to apply patches to sqlite in the future.
>> we would also have to wait for those changes to make it into a sqlite
>> release that would be available on all linux distributions we support. (we
>> probably have the same problem already with other packages we depend on, but
>> i don't think it's a good reason to add one more to the list).
>
> We wouldn't switch. It's just about providing an option for Linux
> distributions that package Chromium (not Google Chrome). Please also note
> that being able to compile and work with vanilla sqlite is a good metric of
> how much forked is our copy.

Note that the vast majority of our "forked" code in SQLite is in code
which we originally wrote for SQLite in the first place :-).

-scott

[Scott Hess]

We could also use -Dopen=myopen type tricks to shim out selected
system calls for our build of SQLite, and then provide different
implementations in the browser versus the renderer. Then we could use
black magic to replace them in the renderer when using the system
SQLite. Since those calls should be forbidden to the renderer anyhow,
it might even be reasonable to implement. Skanky, though.

Since WebDatabase is async, we could also IPC the entire interface to
the browser process. It would probably be reasonable, except for the
security considerations of executing wild SQL statements in the
browser.

The more I think about it, the more I think we can just let time work
for us on fts3/fts2. Once 6.0 goes stable, we'll stop generating new
fts2 tables, I think. So around 8.0, we could push a conversion
system and drop fts2 for 10.0. Skipping 7.0 and 9.0 because I'm more
comfortable with the longer periods involved (think in terms of the
worst-cast-scenario of losing history generated before 6.0).

-scott