Bug 29729 – SQLiteTransaction should use BEGIN DEFERRED instead of BEGIN to begin read-only transactions

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Dumitru Daniliuc]

If a port builds its own sqlite lib and uses flags to turn BEGIN into BEGIN IMMEDIATE by default (like chromium), then scheduling multiple read-only transactions on the DB on the same DB thread will result in a deadlock. The DB thread will essentially be stuck on a task that tries to acquire a RESERVED lock on the DB file, while that lock is held by another task that waits in the queue. In order to prevent this, we should change SQLiteTransaction to explicitly issue a BEGIN DEFERRED command for read-only transactions. This change will also make sure that read-only transactions don't unnecessarily acquire a higher level lock that would delay other potential transactions on the same DB.

[Mark Rowe (bdash)]

Which flag is SQLite being built with to cause this change in behavior?  Why is Chromium building SQLite in this non-standard manner?

[Dumitru Daniliuc]

Created attachment 40098 [details]
patch

[Michael Nordman]

Being explicit in webcore code about the type of transaction is a good thing regardless of how the underlying sqlite library is built.

typo in the ChangeLog, DIFERRED s/b DEFERRED, otherwise lgtm (fwiw)

@mark, to avoid the possibility of starting a transaction that ultimately fails for extraneous reasons, this suited Chrome's internal use of sqlite.

[Eric Seidel (no email)]

Comment on attachment 40098 [details]
patch

I think it's fine to be explicit, assuming that "BEGIN" normally means "BEGIN DEFERRED".  You need to answer Mark's question before we can commit this though.

Also the ordering of the comments seems a bit odd. Maybe the two comments should just be unified into one to explain both cases?  The links seem useful for explaining both.

r+ but please consider the comment modifications, and make sure to answer Mark's query before any commit.

[Mark Rowe (bdash)]

(In reply to comment #3)
> Being explicit in webcore code about the type of transaction is a good thing
> regardless of how the underlying sqlite library is built.

As far as I can tell SQLite has no compilation option that alters the behavior of the "BEGIN" statement, and "BEGIN" is explicitly documented as being equivalent to "BEGIN DEFERRED". 


> @mark, to avoid the possibility of starting a transaction that ultimately fails
> for extraneous reasons, this suited Chrome's internal use of sqlite.

It sounds like the fix is for Chrome to be explicit about the type of transaction it uses rather than patching SQLite.

[Mark Rowe (bdash)]

We're relying on SQLite to behave as documented.  I don't believe that WebKit should change to support patched versions of SQLite that no longer match the documented behavior.

[Michael Nordman]

@mark, well if you really want to force the chrome-team to jump thru some
hoops...

I still think being explicit about the nature of the transaction started by
webcore's SQLiteTransaction class can only be considered a good thing. What's
the downside?

[Mark Rowe (bdash)]

Using SQLite patched in this way is a problem because we can no longer rely on the documentation when writing new code that uses SQLite.  Changing this instance to "BEGIN DEFERRED" may work around the current problem, but as new code is written and *correctly* uses only "BEGIN" it will be subtly incorrect in your configuration.  The solution there is not to insist that we always use "BEGIN DEFERRED" but to not change the behavior of SQLite in the first place.

[Michael Nordman]

(In reply to comment #8)
> Using SQLite patched in this way is a problem because we can no longer rely on
> the documentation when writing new code that uses SQLite.  Changing this
> instance to "BEGIN DEFERRED" may work around the current problem, but as new
> code is written and *correctly* uses only "BEGIN" it will be subtly incorrect
> in your configuration.  The solution there is not to insist that we always use
> "BEGIN DEFERRED" but to not change the behavior of SQLite in the first place.

Maybe the chrome team should re-visit its decision to apply the publicly available patch to sqlite that has been applied. I'll mention this to brett. But that chrome change seems a bit out-of-scope for this particular code change.

BEGIN == BEGIN DEFERRED by default, so I still don't see any downside in the context of this narrowly constrained code change.

[Dumitru Daniliuc]

Mark, I talked to dglazkov about this, and he brought up a good point. Every time we execute a SQL statement, SQLite implicitly creates a transaction. So even if we wanted to change all Chromium code that uses SQLite, but doesn't do it through WebCore, it wouldn't be as easy as grepping for BEGIN and changing all occurrences to BEGIN IMMEDIATE. We'd have to:

1. Find every SQL statement that we execute.
2. Figure out if it goes through WebCore or not.
3. If it doesn't, wrap each statement into a "BEGIN IMMEDIATE; " + sqlStatement + "COMMIT;" (or similar) construct.

Even if this might seem like the right solution, going through the entire Chromium code base looking for SQL statements, and then wrapping each one of them into an explicit transaction seems unreasonable to us at this point.

Another point I want to bring up is that EVERY WebKit port that changes the default behavior of SQLite's BEGIN command will suffer from this problem. Chromium might be the only port that does it at the moment, but it's not unthinkable that another port might want to do the same thing in the future. Without this patch, they'll run into unexpected, hard-to-debug deadlocks as soon as they change their default BEGIN behavior. We (Michael and I) feel that a 1-word change to WebKit that explicitly states the expected default behavior is a much smaller price to pay, even if this change seems redundant and unnecessary (although we think that explicitly (re-)stating our expectations is not redundant and has value).

Any chance we could come to an agreement here? This is not just a nice-to-have or we-like-it-more-this-way change. Without this patch Chromium will not be able to support HTML5 DBs (unless we wrap all our current SQL statements into explicit transactions, which is probably not very likely to happen ever).

[Dimitri Glazkov (Google)]

Comment on attachment 40098 [details]
patch

> +        Starting all read-only transactions with an explicit BEGIN
> +        DIFERRED command instead of BEGIN, since some ports (chromium)

DEFERRED -- typo.

Otherwise, I think this is fine. I don't want to stir up a controversy here, but Dumi's arguments are pretty solid.

[Dumitru Daniliuc]

Created attachment 40315 [details]
patch

Fixed the typo and rearranged the comments in SQLiteTransaction a bit.

[Dimitri Glazkov (Google)]

Comment on attachment 40315 [details]
patch

r=me.

[WebKit Commit Bot]

Comment on attachment 40315 [details]
patch

Clearing flags on attachment: 40315

Committed r48894: <http://trac.webkit.org/changeset/48894>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

Why was this landed.  It doesn't seem like there was a consensus reached.

[Dimitri Glazkov (Google)]

I didn't think there was much left to discuss? Mark raised a valid point, Dumi addressed it pretty thoroughly. The patch doesn't seem very controversial -- doesn't change behavior, fixes a fairly pressing issue. Did I do the wrong thing?

[Sam Weinig]

(In reply to comment #17)
> I didn't think there was much left to discuss? Mark raised a valid point, Dumi
> addressed it pretty thoroughly. The patch doesn't seem very controversial --
> doesn't change behavior, fixes a fairly pressing issue. Did I do the wrong
> thing?

The last substantial comment ended with "Any chance we could come to an agreement here?", so yes, I think you acted prematurely here.  I also don't see how Dumi addressed Marks comment.  It seems the only issue here is that Chromium does not want to change on their end to match standard usage of SQLite.

[Jeremy Orlow]

(In reply to comment #18)
> (In reply to comment #17)
> > I didn't think there was much left to discuss? Mark raised a valid point, Dumi
> > addressed it pretty thoroughly. The patch doesn't seem very controversial --
> > doesn't change behavior, fixes a fairly pressing issue. Did I do the wrong
> > thing?
> 
> The last substantial comment ended with "Any chance we could come to an
> agreement here?", so yes, I think you acted prematurely here.  I also don't see
> how Dumi addressed Marks comment.  It seems the only issue here is that
> Chromium does not want to change on their end to match standard usage of
> SQLite.

What is the downside to Apple and/or any of the other ports?

The down side to Chromium changing how they do things is a substantial amount of changed code.  I'd even go so far as to predict a performance hit and some bloat in executable size due to a lot of extra SQL code being added.

If we tried to upstream a build option to SQLite to change the semantics, would that make everyone happy?  Based on Mark's reasoning in the beginning of the thread, it sounds like it would.

[Dimitri Glazkov (Google)]

Rolled out as http://trac.webkit.org/changeset/48897.

[Sam Weinig]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > I didn't think there was much left to discuss? Mark raised a valid point, Dumi
> > > addressed it pretty thoroughly. The patch doesn't seem very controversial --
> > > doesn't change behavior, fixes a fairly pressing issue. Did I do the wrong
> > > thing?
> > 
> > The last substantial comment ended with "Any chance we could come to an
> > agreement here?", so yes, I think you acted prematurely here.  I also don't see
> > how Dumi addressed Marks comment.  It seems the only issue here is that
> > Chromium does not want to change on their end to match standard usage of
> > SQLite.
> 
> What is the downside to Apple and/or any of the other ports?
> 

I think Marks objections are outlined well in comment #8.

[Jeremy Orlow]

(In reply to comment #21)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > > I didn't think there was much left to discuss? Mark raised a valid point, Dumi
> > > > addressed it pretty thoroughly. The patch doesn't seem very controversial --
> > > > doesn't change behavior, fixes a fairly pressing issue. Did I do the wrong
> > > > thing?
> > > 
> > > The last substantial comment ended with "Any chance we could come to an
> > > agreement here?", so yes, I think you acted prematurely here.  I also don't see
> > > how Dumi addressed Marks comment.  It seems the only issue here is that
> > > Chromium does not want to change on their end to match standard usage of
> > > SQLite.
> > 
> > What is the downside to Apple and/or any of the other ports?
> > 
> 
> I think Marks objections are outlined well in comment #8.

All of his objections only apply to the Chromium port.  We agree that they are valid concerns, but the Chromium port would rather assume the risk Mark outline in comment #8 rather than make the much bigger, riskier change he suggested.

This change will have no overhead on other WebKit ports.  We're not even asking WebKit developers to look out for this in the future.

[Michael Nordman]

> I think Marks objections are outlined well in comment #8.

And there is an unanswered response in comment #9. In the absence of a
response, four more eyes took a look, and that inspection resulted in things
getting committed. Not sure how this molehill became a mountain, but here we
are.

So... what is the plan to surmount this newly formed mountain?

(imho... this is not a productive use of time guys)

[Mark Rowe (bdash)]

A few random points:

* When an objection is raised to a patch it is common courtesy to ensure that the objection is cleared up before the patch is landed.

* Changing the semantics of a standard library such as SQLite is a bad idea.  You've already seen that it causes subtle bugs.

* Claiming that this affects "EVERY WebKit port that changes the
default behavior of SQLite's BEGIN command" may be literally true, but it's a ridiculous statement to make.  This affects Chrome, and only Chrome.

* Claims like "this change will have no overhead on other WebKit ports" are literally true, but miss the forest for this particular tree.  Chrome developers contributing to WebKit no longer see the same semantics from SQL code that every other port sees, which greatly increases the chances that they will contribute code that has subtle issues on other ports.


My suggestion is that if you're unwilling, for whatever reason, to change Chrome to use the standard SQLite semantics then you should make these semantics opt-in.  This could be accomplished by a PRAGMA statement executed immediately after a connection is opened.  This will allow WebCore to continue to remain blissfully unaware of the non-standard semantics that you wish Chromium's use of SQLite to have.

[Dimitri Glazkov (Google)]

My apologies. I had assumed -- incorrectly -- that the discussion had completed. I won't do this again. I'll let you guys hash out the specifics of this.

[Darin Fisher (:fishd, Google)]

> * Changing the semantics of a standard library such as SQLite is a bad idea. 
> You've already seen that it causes subtle bugs.

I agree in principle.  However, in this case modifying SQLite behavior proved to be the better solution.  It was not just a convenient hack.  This is something we put together back in 2007 for Gears to address issues related to multi-process access (which Chrome obviously shares).  In addition to this, we maintain several patches to SQLite.  Some are in the process of being contributed to SQLite.  Some have already (see for instance fts3).  Others are not for various reasons.  In all cases, we take on the burden of maintaining these patches because they provide us some substantial benefit.  That is Chromium team's burden.

When it comes to the proposed change, it is only adding specificity.  It doesn't change the meaning of the code.  So, in the short term at least, I don't see the harm in it.  It fixes a Chromium bug and leaves other ports unmodified.

I propose that we proceed with this patch, and then fork off the discussion about how to avoid it to another bug.  That will entail more work, but I think it is a worthwhile endeavor.  It is reasonable for us to circle back and review the decision from 2007 that brought us here.

Here's the sqlite thread from 2007:
http://www.mail-archive.com/[e-mail]/msg28271.html

[Mark Rowe (bdash)]

While this change itself is obviously small and doesn't change behavior, I feel that the deeper issues that it raises about using custom SQLite dialects should be addressed before we start modifying WebCore.  I suspect that Bugzilla isn't the best forum for that discussion.

[Jeremy Orlow]

(In reply to comment #27)
> While this change itself is obviously small and doesn't change behavior, I feel
> that the deeper issues that it raises about using custom SQLite dialects should
> be addressed before we start modifying WebCore.  I suspect that Bugzilla isn't
> the best forum for that discussion.

Sure.  We can do it on WebKit-dev then?

Either way, I think this patch should go in now.  It fixes a bug that is currently affecting Chromium and will not (itself) affect any other port in a negative way.

Darin acknowledged that there is a larger issue here that needs to be addressed, so there's no need to hold this fix hostage in order to force such a discussion to take place.

[Mark Rowe (bdash)]

(In reply to comment #28)
> (In reply to comment #27)
> > While this change itself is obviously small and doesn't change behavior, I feel
> > that the deeper issues that it raises about using custom SQLite dialects should
> > be addressed before we start modifying WebCore.  I suspect that Bugzilla isn't
> > the best forum for that discussion.
> 
> Sure.  We can do it on WebKit-dev then?

Sure.

> Either way, I think this patch should go in now.  It fixes a bug that is
> currently affecting Chromium and will not (itself) affect any other port in a
> negative way.
> 
> Darin acknowledged that there is a larger issue here that needs to be
> addressed, so there's no need to hold this fix hostage in order to force such a
> discussion to take place.

The comment to which you're replying was quite clear on my feelings about this.

[Mark Rowe (bdash)]

I'd like to add that I understand you're keen to see this problem fixed, but given the code has being using "BEGIN" for at least two years now I don't see why this is suddenly so urgent that we can't have a reasonable discussion to decide the appropriate course of action.

[Maciej Stachowiak]

(In reply to comment #10)
> Mark, I talked to dglazkov about this, and he brought up a good point. Every
> time we execute a SQL statement, SQLite implicitly creates a transaction. So
> even if we wanted to change all Chromium code that uses SQLite, but doesn't do
> it through WebCore, it wouldn't be as easy as grepping for BEGIN and changing
> all occurrences to BEGIN IMMEDIATE. We'd have to:
> 
> 1. Find every SQL statement that we execute.
> 2. Figure out if it goes through WebCore or not.
> 3. If it doesn't, wrap each statement into a "BEGIN IMMEDIATE; " + sqlStatement
> + "COMMIT;" (or similar) construct.
> 
> Even if this might seem like the right solution, going through the entire
> Chromium code base looking for SQL statements, and then wrapping each one of
> them into an explicit transaction seems unreasonable to us at this point.

If your change to SQLite affects every single SQL statement in Chromium, not just BEGINs, then isn't there a risk it affects every single SQL statement in WebKit, not just BEGINs? Or is there a reason that's not the case?

[Maciej Stachowiak]

(In reply to comment #30)
> I'd like to add that I understand you're keen to see this problem fixed, but
> given the code has being using "BEGIN" for at least two years now I don't see
> why this is suddenly so urgent that we can't have a reasonable discussion to
> decide the appropriate course of action.

Unless there is a reason this change is particularly time-sensitive, it's probably good to have the discussion first.

I wouldn't categorically rule out accepting the change, but to me it seems strange and worrisome for Chromium to modify SQLite's behavior and then modify WebKit to work around it. It seems like there will be a risk of this approach creating future bugs, because somebody didn't test on the other SQLite variant. So let's see if we can figure out a more sustainable solution.

[Maciej Stachowiak]

(In reply to comment #23)
> > I think Marks objections are outlined well in comment #8.
> 
> And there is an unanswered response in comment #9. In the absence of a
> response, four more eyes took a look, and that inspection resulted in things
> getting committed. Not sure how this molehill became a mountain, but here we
> are.
> 
> So... what is the plan to surmount this newly formed mountain?
> 
> (imho... this is not a productive use of time guys)

I agree this issue is the biggest in the world. We could live with this patch if there is truly no alternative, even though it creates a long-term testing risk. However, I don't think it's good to be so dismissive of Mark's concerns. He sees this small code change as potentially indicative of larger issues. That's worth thinking about, at least.

[Michael Nordman]

> I agree this issue is the biggest in the world. We could live with this patch
> if there is truly no alternative, even though it creates a long-term testing
> risk. However, I don't think it's good to be so dismissive of Mark's concerns.
> He sees this small code change as potentially indicative of larger issues.
> That's worth thinking about, at least.

Thnx for the courtesy of an actual reply to something i typed in here.

I certainly didn't mean to be dismissive of Mark's concerns. I replied and waited, dumi followed up, we waited some more. After no response, I escalated to some chrome webkit reviewers/committers, we exchanged thoughts on what to do and life went on... and then all hell broken loose.

It's been mentioned a couple of times in the course of this code review that the chrome team should probably revisit its decision to apply that SQLite patch. I buy that, several others of us do to. Any changes we make there has some ripple, all of our usages of SQLite will be affected. We could probably make the change w/o any incidence, but there is a chance of an unintended interaction somewhere. So we're not looking to make that change hastily. We will do so only with appropriate deliberation.

It's also been observed that the addition of the DEFERRED keyword in webcore code should be considered a small improvement to the webcore code base irrespective of default behavior of SQLite. Adding clarity and specificity is generally good thing. Several comments have added weight to that observation. And obviously it doesn't alter the behavior of webcore at all.

When i hold the two options up and look at what todo for the here and now, the answer is pretty obvious to me... a choice between a safe cosmetic change vs something that may actually carry risk.

[Dumitru Daniliuc]

Maciej, I don't think there's any testing risk here. With this patch, we do not want WebCore to do anything special for us. On the contrary, we want it to do exactly the same thing that it does for all other ports. And since our SQLite build uses different defaults, we just want to replace a SQLite standard command that depends on default values with another SQLite standard command that doesn't. That's all.

> If your change to SQLite affects every single SQL statement in Chromium, not
> just BEGINs, then isn't there a risk it affects every single SQL statement in
> WebKit, not just BEGINs? Or is there a reason that's not the case?

As far as I know, Chromium doesn't use any WebKit code that interacts with SQLite (other than the databases/storage code, but this is exactly the code that we're trying to fix in this patch).

[Maciej Stachowiak]

(In reply to comment #35)
> Maciej, I don't think there's any testing risk here. With this patch, we do not
> want WebCore to do anything special for us. On the contrary, we want it to do
> exactly the same thing that it does for all other ports. And since our SQLite
> build uses different defaults, we just want to replace a SQLite standard
> command that depends on default values with another SQLite standard command
> that doesn't. That's all.
> 
> > If your change to SQLite affects every single SQL statement in Chromium, not
> > just BEGINs, then isn't there a risk it affects every single SQL statement in
> > WebKit, not just BEGINs? Or is there a reason that's not the case?
> 
> As far as I know, Chromium doesn't use any WebKit code that interacts with
> SQLite (other than the databases/storage code, but this is exactly the code
> that we're trying to fix in this patch).


That doesn't really answer my question - if non-BEGIN statements in Chromium's code could be affected by a change to SQLite BEGIN behavior, why aren't non-BEGIN statements in WebKit code affected? Is the change asymmetric somehow? This patch only changes a BEGIN and not other statements. This isn't necessarily a showstopper, I'd just like to understand the issue.

[Michael Nordman]

> That doesn't really answer my question - if non-BEGIN statements in Chromium's
> code could be affected by a change to SQLite BEGIN behavior, why aren't
> non-BEGIN statements in WebKit code affected? Is the change asymmetric somehow?
> This patch only changes a BEGIN and not other statements. This isn't
> necessarily a showstopper, I'd just like to understand the issue.

Thats a good question. I'm not sure if our patch has any affect on the implicit transactions started around 'naked' statements. Our patch is to the parser, so there's a good chance that it doesn't affect non-BEGIN statements, but I don't know that for a fact. Scott probably know the answer to that question.

[Dimitri Glazkov (Google)]

As we spoke over IRC, I think the following plan materialized:

* dumi & shess to meet tomorrow and figure out the impact of rolling out BEGIN IMMEDIATE patch
* if the roll out is not possible, figure out the effort involved in adding a PRAGMA option (or even direct SQLite API) to change transaction mode run-time.
* If the effort is more than knee-high, we'll regroup and work w/Maciej & Mark until we have a consensus on how to proceed. This effort may involve alcohol.

[Maciej Stachowiak]

(In reply to comment #38)
> * If the effort is more than knee-high, we'll regroup and work w/Maciej & Mark
> until we have a consensus on how to proceed. This effort may involve alcohol.

Wait, no alcohol until step 3? :-)

[Michael Nordman]

Oh well, so much for avoiding the hoop jumping.

[Michael Nordman]

> Thats a good question. I'm not sure if our patch has any affect on the implicit
> transactions started around 'naked' statements. Our patch is to the parser, so
> there's a good chance that it doesn't affect non-BEGIN statements, but I don't
> know that for a fact. Scott probably know the answer to that question.

To complete the jump thru this particular hoop...

>> scott said
>> Re Michael's #37, I believe all implicit statements use the
>> appropriate transaction type from the get-go
>
> i said
> Clarification please... by 'appropiate trans type' do you mean...
> a. IMMEDIATE since we've whacked the parser in that way
> b. appropriate for the statement type, independent of our parser whackings
> I think you mean the latter.

scott said

I mean many different things, and all of them mean that it doesn't
matter.  Possibilities:

a. the BEGIN default is changed in the parser, so implicit statements
don't change their behavior at all.
b. the compiler knows enough to compile the statement to BEGIN
IMMEDIATE if it's an update, or BEGIN DEFERRED if not, again our
change wouldn't affect this.
c. regardless, if an implicit statement returns SQLITE_BUSY the
appropriate response is to execute it again.  This is correct and
works right regardless of whether it starts with an IMMEDIATE
transaction or upgrades it midway through.

Basically, I think implicit transactions are a red herring in this discussion.

[Dumitru Daniliuc]

We reverted our SQLite patch to make BEGIN default to BEGIN DEFERRED, thus making this patch unnecessary.