696364 - Open sqlite tables in exclusive mode

Status: RESOLVED WONTFIX

(Toolkit :: Storage, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Open sqlite databases with an exclusive lock

This seems to reduce the cost of sqlite's multiprocess support. i.e. It avoids the read of the lock page in sqlite3PagerSharedLock()

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 568640 [details] [diff] [review]
Open sqlite databases with an exclusive lock

Review of attachment 568640 [details] [diff] [review]:
-----------------------------------------------------------------

exclusive mode does not always work correctly, for example Places would stop working. should be done for single implementers, where they allow to do so.

also drh told me with WAL is better to avoid exclusive mode since it's more performant without it

Comment 2

[Marco Bonardo [:mak]]

PS: the right component for SQLite and storage bugs is Toolkit/Storage

Comment 3

[(dormant account)]

(In reply to Marco Bonardo [:mak] from comment #1)
> 
> exclusive mode does not always work correctly, for example Places would stop
> working. should be done for single implementers, where they allow to do so.

What is the reason for places to not work correctly?

Comment 4

[Marco Bonardo [:mak]]

If I'm not wrong exclusive mode is per connection, thus we cannot clone separate connections and we lose concurrency (the locationbar performances would suck). Last time I tried reintroducing exclusive mode I was unable to create those.

Comment 5

[(dormant account)]

(In reply to Marco Bonardo [:mak] from comment #4)
> If I'm not wrong exclusive mode is per connection, thus we cannot clone
> separate connections and we lose concurrency (the locationbar performances
> would suck). Last time I tried reintroducing exclusive mode I was unable to
> create those.

Sqlite still does locking in the same fashion. It just does not read a page from disk on every operation to check if other processes are locking the db. Jeff's firefox continued to work correctly after his change.

Comment 6

[Marco Bonardo [:mak]]

This is what drh told me:
"I say just leave EXCLUSIVE turned off.  The original reason for turning it own was a performance optimization that only appears in rollback journal modes.  (In EXCLUSIVE mode, you don't have to read the first page of the database file at the beginning of each read transaction to check to see if another process has modified the database and that the in-memory cache needs to be cleared.) If you are now using WAL for everything, EXCLUSIVE locking mode does not buy you anything, as far as I can think of."

Comment 7

[Marco Bonardo [:mak]]

(In reply to Taras Glek (:taras) from comment #5)
> Jeff's firefox continued to work correctly after his change.

Did you try creating a new profile?
Maybe I just missed something

Comment 8

[Marco Bonardo [:mak]]

Btw, this is also hitting Thunderbird and any other xulrunner implementers. I don't know if it's something we should do in Storage rather than in single components since I don't know if their use-cases may differ. Even if the lock would be per-process, are we sure this won't hurt future e10s work by closing db acess across processes?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Marco Bonardo [:mak] from comment #6)
> This is what drh told me:
> "I say just leave EXCLUSIVE turned off.  The original reason for turning it
> own was a performance optimization that only appears in rollback journal
> modes.  (In EXCLUSIVE mode, you don't have to read the first page of the
> database file at the beginning of each read transaction to check to see if
> another process has modified the database and that the in-memory cache needs
> to be cleared.) If you are now using WAL for everything, EXCLUSIVE locking
> mode does not buy you anything, as far as I can think of."

But we are not using WAL for everything. It seems like we're only using WAL for places and cookies.

(In reply to Marco Bonardo [:mak] from comment #7)
> (In reply to Taras Glek (:taras) from comment #5)
> > Jeff's firefox continued to work correctly after his change.
> 
> Did you try creating a new profile?
> Maybe I just missed something

I tried, again and it did seem like things were broken. It looks like using EXCLUSIVE for all the databases won't work.

Would you be opposed to using it from some databases? (e.g search.sqlite)

Comment 10

[Marco Bonardo [:mak]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> I tried, again and it did seem like things were broken. It looks like using
> EXCLUSIVE for all the databases won't work.
> 
> Would you be opposed to using it from some databases? (e.g search.sqlite)

No, indeed I think it's a sane idea for all databases using a rollback journal.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> But we are not using WAL for everything. It seems like we're only using WAL
> for places and cookies.
That's because those are the only two things I pushed to get updated.  Nothing else is strongly owned, so it hasn't been updated (those two are also the only primary consumers of the asynchronous API in core).

> Would you be opposed to using it from some databases? (e.g search.sqlite)
Using WAL and `PRAGMA synchronous = normal` is probably better for them, but that's also a per-component decision.

As it stands, we cannot fix this globally like this.