Bug 29218 – Write transactions should start with a BEGIN IMMEDIATE command

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

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.

[Dumitru Daniliuc]

Created attachment 39500 [details]
patch

[Eric Seidel (no email)]

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.

[Dumitru Daniliuc]

(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).

[Michael Nordman]

lgtm

[Michael Nordman]

sorry for the bum brackets steer

[Dumitru Daniliuc]

Created attachment 39730 [details]
patch

Same patch. Added comments to ChangeLog why we can't test it.

[Eric Seidel (no email)]

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+.

[Dumitru Daniliuc]

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?

[Eric Seidel (no email)]

Comment on attachment 39801 [details]
patch

YES!  That's soooo much clearer.  Thank you!

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 39801 [details]
patch

bug 28316, still working on a fix.

[WebKit Commit Bot]

Comment on attachment 39801 [details]
patch

Clearing flags on attachment: 39801

Committed r48617: <http://trac.webkit.org/changeset/48617>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.