462173 - We treat transaction errors as fatal when we should not

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

For the async statements (note the plural), we open a transaction, and then commit it when we are done.  However, it is possible that this returns an error, in which case we can try to handle it (if it is SQLITE_BUSY), or we should rollback.  Right now we do neither.  We'll need bug 451851 to fix this properly.

Requesting blocking because this breaks places pretty badly when it comes up.

Comment 1

[(not currently active) Ted Mielczarek]

Shouldn't ship async statements with a bug like this.

Comment 2

[Shawn Wilsher :sdwilsh]

For reviewer reference:
http://www.sqlite.org/lang_transaction.html

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Does this really fix places' problem?  I would expect that the places badness would be that places has an outstanding SELECT that is still active when the async thread goes to commit its transaction.  That transaction commit will fail with SQLITE_ERROR, so no retry attempt will be made and you are hosed for good unless your code starts manually trying to issue a commit/rollback on its own (without a new BEGIN).  Change mozilla's sqlite3.c:48348 to SQLITE_BUSY and these changes come into play.

Comment 5

[Shawn Wilsher :sdwilsh]

So, as of right now it's safe to retry this if we get SQLITE_ERROR.  They may end up changing it so that it will return SQLITE_BUSY.  They will investigate this and change (to make sure it doesn't have implications elsewhere) and get back with us.

For now, I can change this patch to retry on SQLITE_ERROR

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #5)
> For now, I can change this patch to retry on SQLITE_ERROR

Change it to use the logic for rollback too, since rollback can have the same problem happen to it.

Comment 7

[Shawn Wilsher :sdwilsh]

I don't think we can fix this without getting a fix for SQLite.  There is no way to know what SQLITE_ERROR is recoverable and which are not.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #7)
> I don't think we can fix this without getting a fix for SQLite.  There is no
> way to know what SQLITE_ERROR is recoverable and which are not.

Well, we could have mozStorageConnection effectively maintain its own activeVdbeCnt; as statements transition to and from executing, they notify the connection.  This has the upside that instead of busy-sleep-waiting, the transaction commit could actually block on a proper notification primitive.  On the other hand, that's a lot of additional book-keeping overhead.

Is there a problem with just changing the one line I called out in mozilla's copy of sqlite until we get the official fix from sqlite command?  Granted, we are changing an error return from fatal to non-fatal, but... 1) it's still an error, 2) people could keep retrying regardless of the error code, 3) the error code was as-is is content-free and accordingly not actionable.

Comment 9

[Shawn Wilsher :sdwilsh]

We have a strict policy of not making any changes to the sqlite files and only accept official copies of sqlite.  This may mean that we end up asking them for a special branch with this fix in it, but it has their qa stamp of approval then.

Comment 10

[Shawn Wilsher :sdwilsh]

SQLite 3.6.5 has a much better behavior - commits always succeed (they are already on disk anyway), but rollbacks may end up being busy.  We can busy wait on rollback since it's a rare condition anyway.  That change makes our lives so much easier :)

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

When we upgrade to a newer SQLite (not 3.6.5 since it crashes in FTS3), I think this is what we'll want.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 347847 [details] [diff] [review]
v2.0

Yes, this all turns out much better/simpler with the new sqlite.

Comment 13

[Shawn Wilsher :sdwilsh]

Per blocking rankings, this needs to be fixed by release, but doesn't need to block the proposed beta 3.

Comment 14

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/5384f38dc5e5

Comment 16

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b6c78d8df75