Bug 29218 - Write transactions should start with a BEGIN IMMEDIATE command
Summary: Write transactions should start with a BEGIN IMMEDIATE command
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-11 18:22 PDT by Dumitru Daniliuc
Modified: 2009-09-21 19:34 PDT (History)
6 users (show)

See Also:


Attachments
patch (3.16 KB, patch)
2009-09-11 18:35 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (3.66 KB, patch)
2009-09-17 15:55 PDT, Dumitru Daniliuc
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
patch (4.25 KB, patch)
2009-09-18 15:51 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2009-09-11 18:22:46 PDT
Thanks to Michael for pointing this out!

http://www.sqlite.org/lockingv3.html:
SHARED lock: The database may be read but not written. Any number of processes can hold SHARED locks at the same time, hence there can be many simultaneous readers. But no other thread or process is allowed to write to the database file while one or more SHARED locks are active.
RESERVED lock: A RESERVED lock means that the process is planning on writing to the database file at some point in the future but that it is currently just reading from the file. Only a single RESERVED lock may be active at one time, though multiple SHARED locks can coexist with a single RESERVED lock.

Currently, when we start a transaction, we issue a BEGIN command. According to http://www.sqlite.org/lang_transaction.html, BEGIN does not acquire any lock on the DB file. Then the first read operation acquires a SHARED lock, and the first write transaction escalates the lock to RESERVED. This can lead to failing write transactions. For example:

transaction_1:
SELECT ...
UPDATE ...

transaction_2:
INSERT ...

transaction_1 is allowed to execute the first statement. Since it's a SELECT operation, transaction_1 acquires only a SHARED lock. Now transaction_2 runs; it acquires a RESERVED lock and changes the database. Now transaction_1 resumes and tries to update the database too, but since the database was changed by transaction_2, the UPDATE operation fails.

We think the correct way to do this is to start all write transactions with a BEGIN IMMEDIATE command, which acquires a RESERVED lock before executing any statement. This would make transaction_2 wait for transaction_1 to finish, and both transactions would complete successfully.
Comment 1 Dumitru Daniliuc 2009-09-11 18:35:35 PDT
Created attachment 39500 [details]
patch
Comment 2 Eric Seidel (no email) 2009-09-14 09:55:02 PDT
Comment on attachment 39500 [details]
patch

Can we test this?  The ChangegLog really should explain why we can't test something anytime there are no tests in the patch.
Comment 3 Dumitru Daniliuc 2009-09-16 18:41:49 PDT
(In reply to comment #2)
> (From update of attachment 39500 [details])
> Can we test this?  The ChangegLog really should explain why we can't test
> something anytime there are no tests in the patch.

I don't think we can test this in a layout test. In order to test this, we need to run the steps of 2 transactions on 2 different DB threads in a very specific order, and I don't think it's possible to achieve this when the 2 DB threads share the same main thread (as they would in a layout test).
Comment 4 Michael Nordman 2009-09-17 13:13:37 PDT
lgtm
Comment 5 Michael Nordman 2009-09-17 15:15:54 PDT
sorry for the bum brackets steer
Comment 6 Dumitru Daniliuc 2009-09-17 15:55:07 PDT
Created attachment 39730 [details]
patch

Same patch. Added comments to ChangeLog why we can't test it.
Comment 7 Eric Seidel (no email) 2009-09-18 13:50:06 PDT
Comment on attachment 39730 [details]
patch

OK.  I guess 2 more things I would have done if posting this patch to make the reviewer's job a nobrainer would be:

1.  link to the SQLLite docs about this in the ChangeLog (assuming a direct link to the section could be found).

2.  I would consider adding a comment next to:
+        if (m_readOnly)
+            m_inProgress = m_db.executeCommand("BEGIN;");
+        else
+            m_inProgress = m_db.executeCommand("BEGIN IMMEDIATE;");
to explain why we use BEGIN vs. BEGIN IMMEDIATE and what they do.  Possibly also referencing the SQLLite docs url if you feel that's necessary.

So r+, but I would like you to make modifications like described above when landing.  Or if you aren't a committer yet (I can never remember) and want this commit-queue'd, then please post a new patch to be marked r+/cq+.
Comment 8 Dumitru Daniliuc 2009-09-18 15:51:05 PDT
Created attachment 39801 [details]
patch

Addressed Eric's comments.

Eric: I'm not a committer yet. Can you please re-approve the patch (if you're happy with it) and put it in the commit queue?
Comment 9 Eric Seidel (no email) 2009-09-21 18:30:58 PDT
Comment on attachment 39801 [details]
patch

YES!  That's soooo much clearer.  Thank you!
Comment 10 WebKit Commit Bot 2009-09-21 18:49:09 PDT
Comment on attachment 39801 [details]
patch

Rejecting patch 39801 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is probably out-of-date: resource out of date; try updating at /usr/local/libexec/git-core//git-svn line 469
Comment 11 Eric Seidel (no email) 2009-09-21 18:49:57 PDT
Comment on attachment 39801 [details]
patch

bug 28316, still working on a fix.
Comment 12 WebKit Commit Bot 2009-09-21 19:34:22 PDT
Comment on attachment 39801 [details]
patch

Clearing flags on attachment: 39801

Committed r48617: <http://trac.webkit.org/changeset/48617>
Comment 13 WebKit Commit Bot 2009-09-21 19:34:27 PDT
All reviewed patches have been landed.  Closing bug.