If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Open sqlite tables in exclusive mode

RESOLVED WONTFIX

Status

()

Toolkit
Storage
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: jrmuizel, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 568640 [details] [diff] [review]
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()
Attachment #568640 - Flags: review?(mak77)
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
Attachment #568640 - Flags: review?(mak77) → review-
PS: the right component for SQLite and storage bugs is Toolkit/Storage
Assignee: Jan.Varga → nobody
Component: SQL → Storage
Product: Core → Toolkit
QA Contact: owen.marshall+bmo → storage

Comment 3

6 years ago
(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?
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

6 years ago
(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.
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."
(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
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?
(Reporter)

Comment 9

6 years ago
(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)
(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.
(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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.