Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2827673006:
[sql] Verify control of close-time WAL checkpoint. (Closed)
|
Created:
3 years, 8 months ago by Scott Hess - ex-Googler Modified:
3 years, 8 months ago Reviewers:
pwnall CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[sql] Verify control of close-time WAL checkpoint.
SQLite WAL mode by default checkpoints the WAL to the main database at
close time. Chromium closes databases at shutdown, which causes
shutdown slowdowns due to buffered data being flushed. WAL
checkpointing would effectively increase the amount of buffered data
being flushed.
[WAL mode is currently not in use by Chromium.]
BUG=675264
Review-Url: https://codereview.chromium.org/2827673006
Cr-Commit-Position: refs/heads/master@{#465772}
Committed: https://chromium.googlesource.com/chromium/src/+/f7fcc45c7cfb05f8fed045d38153792baba57e89
Patch Set 1 #Patch Set 2 : comment tweaks #
Total comments: 5
Messages
Total messages: 17 (8 generated)
comment tweaks
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: + pwnall@chromium.org
Found this CL lying around. Originally it was a patch backported from SQLite, but I've updated SQLite since then. The goal of the bug was to grind through the reasons we can't just turn on WAL mode, without actually turning it on, yet.
https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... File sql/sqlite_features_unittest.cc (right): https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... sql/sqlite_features_unittest.cc:464: TEST_F(SQLiteFeaturesTest, WALNoClose) { Um, this is the main test for whether the desired feature is present. The other changes above are mostly "Now that I've imported the ExecuteWithResult() helper, what else would I change?"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... File sql/sqlite_features_unittest.cc (right): https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... sql/sqlite_features_unittest.cc:460: #if !defined(USE_SYSTEM_SQLITE) Would tests fail anywhere if we removed this #if? I'd imagine we're OK with tests failing on people's custom builds if their system libraries don't implement everything we need.
https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... File sql/sqlite_features_unittest.cc (right): https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... sql/sqlite_features_unittest.cc:460: #if !defined(USE_SYSTEM_SQLITE) On 2017/04/19 20:02:23, pwnall wrote: > Would tests fail anywhere if we removed this #if? > > I'd imagine we're OK with tests failing on people's custom builds if their > system libraries don't implement everything we need. iOS uses the system SQLite. Also maybe Linux distros bundling Chromium, but I'm not sure that works any longer. Perhaps a different question is whether this feature is relevant for iOS, given that we generally don't actually shutdown in a way that makes this relevant. But here I'm trying to make assertions about features for Chromium proper to depend on, so it's not in the regular shutdown context, so that may also not really be relevant.
https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... File sql/sqlite_features_unittest.cc (right): https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... sql/sqlite_features_unittest.cc:460: #if !defined(USE_SYSTEM_SQLITE) On 2017/04/19 20:15:33, Scott Hess wrote: > On 2017/04/19 20:02:23, pwnall wrote: > > Would tests fail anywhere if we removed this #if? > > > > I'd imagine we're OK with tests failing on people's custom builds if their > > system libraries don't implement everything we need. > > iOS uses the system SQLite. Also maybe Linux distros bundling Chromium, but I'm > not sure that works any longer. > > Perhaps a different question is whether this feature is relevant for iOS, given > that we generally don't actually shutdown in a way that makes this relevant. > But here I'm trying to make assertions about features for Chromium proper to > depend on, so it's not in the regular shutdown context, so that may also not > really be relevant. TL;DR: If the test passes on bots, I'd leave it on. Our tests don't have to pass on Linux distros that replace our libraries with arbitrary versions. That was the case I was thinking of. In that case, I claim that it's better to have the test fail, so the distributor knows all bets are off. Is the system SQLite on our iOS bots old enough / built with a config that doesn't support the feature? If newer iOS has WAL, I'd leave the test in. If not, I'd be tempted to have the test skipped for iOS, with a comment explaining the situation.
The CQ bit was checked by shess@chromium.org
OK, I had thought that the Mojo case maybe needed testing (there is a subtle mojo case that I never remember how to build to test), but all the bots I could see gave exceptions. So I'm just going to press the button and see what happens. Sheriffs can feel free to revert this if it goes awry. https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... File sql/sqlite_features_unittest.cc (right): https://codereview.chromium.org/2827673006/diff/20001/sql/sqlite_features_uni... sql/sqlite_features_unittest.cc:460: #if !defined(USE_SYSTEM_SQLITE) On 2017/04/19 20:56:54, pwnall wrote: > Is the system SQLite on our iOS bots old enough / built with a config that > doesn't support the feature? If newer iOS has WAL, I'd leave the test in. If > not, I'd be tempted to have the test skipped for iOS, with a comment explaining > the situation. The SQLite change landed last fall, so it's pretty new. iOS has pretty good uptake when new OS versions come out, but I expect that older versions of iOS which are still supported by Chrome will not bump their SQLite version. I haven't done the research to figure out the exact SQLite version the change landed in, but based on https://github.com/yapstudios/YapDatabase/wiki/SQLite-version-(bundled-with-OS) I suspect no iOS versions of SQLite with the change being tested.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492639321819690,
"parent_rev": "08590c2373943322cbcaa8e06c445cfcc7f3acd8", "commit_rev":
"f7fcc45c7cfb05f8fed045d38153792baba57e89"}
Message was sent while issue was closed.
Description was changed from ========== [sql] Verify control of close-time WAL checkpoint. SQLite WAL mode by default checkpoints the WAL to the main database at close time. Chromium closes databases at shutdown, which causes shutdown slowdowns due to buffered data being flushed. WAL checkpointing would effectively increase the amount of buffered data being flushed. [WAL mode is currently not in use by Chromium.] BUG=675264 ========== to ========== [sql] Verify control of close-time WAL checkpoint. SQLite WAL mode by default checkpoints the WAL to the main database at close time. Chromium closes databases at shutdown, which causes shutdown slowdowns due to buffered data being flushed. WAL checkpointing would effectively increase the amount of buffered data being flushed. [WAL mode is currently not in use by Chromium.] BUG=675264 Review-Url: https://codereview.chromium.org/2827673006 Cr-Commit-Position: refs/heads/master@{#465772} Committed: https://chromium.googlesource.com/chromium/src/+/f7fcc45c7cfb05f8fed045d38153... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f7fcc45c7cfb05f8fed045d38153... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
