|
|
Created:
4 years, 4 months ago by Scott Hess - ex-Googler Modified:
4 years, 4 months ago Reviewers:
afakhry CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sql] Retry post-poison open as soon as possible.
sql::Connection::Open() retries the open if the handle is poisoned by
the error callback. The assumption is that the error callback likely
razed or recovered the database, and since there can be no outstanding
statements the open can be safely retried. This change adds this
handling to call which attempts to probe the database for basic
validity.
BUG=none
Committed: https://crrev.com/bccd300e8c0aa5f5cd255b24b5b709012c0f4355
Cr-Commit-Position: refs/heads/master@{#413292}
Patch Set 1 #
Total comments: 8
Patch Set 2 : nullptr #Messages
Total messages: 18 (8 generated)
The CQ bit was checked by shess@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shess@chromium.org changed reviewers: + afakhry@chromium.org
Turns out this was already being tested by SQLConnectionTest.RazeCallbackReopen. It's just moving the call forward from the secure_delete section in that case. I don't think it's worthwhile to test more precisely.
On 2016/08/18 21:21:19, Scott Hess wrote: > Turns out this was already being tested by SQLConnectionTest.RazeCallbackReopen. > It's just moving the call forward from the secure_delete section in that case. > I don't think it's worthwhile to test more precisely. Umm, I _think_ this is basically the thing you did, only after the code to set the journal_mode. I pulled it up here because I think earlier I suggested _not_ making a change in this area on the basis of "Don't change that until you understand why it's written that way", and AFAICT it was written as a probe to see if the database is totally invalid, so it's reasonable to layer that in here. So I think this is generally useful.
https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc#newcode1703 sql/connection.cc:1703: err = ExecuteAndReturnErrorCode("PRAGMA auto_vacuum"); Shouldn't we use Execute() instead so that the error callback is invoked, in order to give a chance to clients to Raze() the DB on a catastrophic error? https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc#newcode1747 sql/connection.cc:1747: ignore_result(Execute("PRAGMA journal_mode = TRUNCATE")); Why retrying up there and not here?
https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc#newcode1703 sql/connection.cc:1703: err = ExecuteAndReturnErrorCode("PRAGMA auto_vacuum"); On 2016/08/18 21:32:29, afakhry wrote: > Shouldn't we use Execute() instead so that the error callback is invoked, in > order to give a chance to clients to Raze() the DB on a catastrophic error? Sqlite.OpenProbeFailure tracks the specific SQLite error. Execute() followed by GetErrorCode() could work, but if the error callback does RazeAndClose() or calls recovery code, GetErrorCode() will return the last error from _that_ (kind of like how errno needs to be pinned in error-handling code if you want to log it). OnSqliteError() will call into the regular error-handling, including callback. https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc#newcode1747 sql/connection.cc:1747: ignore_result(Execute("PRAGMA journal_mode = TRUNCATE")); On 2016/08/18 21:32:29, afakhry wrote: > Why retrying up there and not here? Any Execute() calls in between will cause poisoned_ to be set, and then the retry under secure_delete should handle it. I haven't been able to formulate a case where any of these would fail catastrophically but secure_delete would succeed. That case would also apply to the auto_vacuum case I'm changing, here, but pushing it as early as possible removes all the busy-work of firing errors over and over again. I _believe_ that the majority of the poison-type errors in Open() will happen either in sqlite3_open() or probing the first page (which the auto_vacuum check does). I think the next-most-likely case will be in loading the schema, which I don't think anything in Open() does, though I'm semi-seriously considering adding a call to force that. Put another way, I don't want to litter this code with retries, I'd rather refactor to make a helper and then put a retry around it. But I don't think your CL will profit from pending on that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc#newcode1703 sql/connection.cc:1703: err = ExecuteAndReturnErrorCode("PRAGMA auto_vacuum"); On 2016/08/18 22:06:53, Scott Hess wrote: > On 2016/08/18 21:32:29, afakhry wrote: > > Shouldn't we use Execute() instead so that the error callback is invoked, in > > order to give a chance to clients to Raze() the DB on a catastrophic error? > > Sqlite.OpenProbeFailure tracks the specific SQLite error. Execute() followed by > GetErrorCode() could work, but if the error callback does RazeAndClose() or > calls recovery code, GetErrorCode() will return the last error from _that_ (kind > of like how errno needs to be pinned in error-handling code if you want to log > it). OnSqliteError() will call into the regular error-handling, including > callback. Oh yes, sorry, I didn't notice the OnSqliteError() even though I wrote something similar in one of the patches in my other CL. :D https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc#newcode1706 sql/connection.cc:1706: OnSqliteError(err, NULL, "PRAGMA auto_vacuum"); Nit: NULL --> nullptr. https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc#newcode1747 sql/connection.cc:1747: ignore_result(Execute("PRAGMA journal_mode = TRUNCATE")); On 2016/08/18 22:06:52, Scott Hess wrote: > On 2016/08/18 21:32:29, afakhry wrote: > > Why retrying up there and not here? > > Any Execute() calls in between will cause poisoned_ to be set, and then the > retry under secure_delete should handle it. I haven't been able to formulate a > case where any of these would fail catastrophically but secure_delete would > succeed. That case would also apply to the auto_vacuum case I'm changing, here, > but pushing it as early as possible removes all the busy-work of firing errors > over and over again. > > I _believe_ that the majority of the poison-type errors in Open() will happen > either in sqlite3_open() or probing the first page (which the auto_vacuum check > does). I think the next-most-likely case will be in loading the schema, which I > don't think anything in Open() does, though I'm semi-seriously considering > adding a call to force that. > > Put another way, I don't want to litter this code with retries, I'd rather > refactor to make a helper and then put a retry around it. But I don't think > your CL will profit from pending on that. Makes sense. Thanks. Acknowledged.
https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2258703004/diff/1/sql/connection.cc#newcode1706 sql/connection.cc:1706: OnSqliteError(err, NULL, "PRAGMA auto_vacuum"); On 2016/08/19 15:25:09, afakhry wrote: > Nit: NULL --> nullptr. Done.
nullptr
The CQ bit was checked by shess@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2258703004/#ps20001 (title: "nullptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [sql] Retry post-poison open as soon as possible. sql::Connection::Open() retries the open if the handle is poisoned by the error callback. The assumption is that the error callback likely razed or recovered the database, and since there can be no outstanding statements the open can be safely retried. This change adds this handling to call which attempts to probe the database for basic validity. BUG=none ========== to ========== [sql] Retry post-poison open as soon as possible. sql::Connection::Open() retries the open if the handle is poisoned by the error callback. The assumption is that the error callback likely razed or recovered the database, and since there can be no outstanding statements the open can be safely retried. This change adds this handling to call which attempts to probe the database for basic validity. BUG=none Committed: https://crrev.com/bccd300e8c0aa5f5cd255b24b5b709012c0f4355 Cr-Commit-Position: refs/heads/master@{#413292} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bccd300e8c0aa5f5cd255b24b5b709012c0f4355 Cr-Commit-Position: refs/heads/master@{#413292} |