[Sync] Add test to verify sync DB corruption handling during SaveChanges

[Sync] Add test to verify sync DB corruption handling during SaveChanges

BUG=470993
Committed: https://crrev.com/9f5d1bfcc8d3f856c921c8910f005f305df128c3
Cr-Commit-Position: refs/heads/master@{#326802}

[maniscalco, 2015-04-22 21:59:13]

The CQ bit was checked by maniscalco@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-22 22:01:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1090413005/20001

[maniscalco, 2015-04-23 21:08:13]

Patchset #6 (id:100001) has been deleted

[maniscalco, 2015-04-23 21:09:06]

maniscalco@chromium.org changed reviewers:
+ zea@chromium.org

[maniscalco, 2015-04-23 21:09:07]

Nicolas, would you please review this change?

[Nicolas Zea, 2015-04-23 21:53:27]

LGTM with one question/concern

https://codereview.chromium.org/1090413005/diff/120001/sync/syncable/director...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/1090413005/diff/120001/sync/syncable/director...
sync/syncable/directory_backing_store_unittest.cc:4100: // won't keep it all in
memory. If this test fails, you may need to increase
Hmm, do we control how many entries SQLite will keep in memory, or is that
something it does on its own?

[maniscalco, 2015-04-23 22:57:48]

Thanks for the speedy review!  I've updated the comments based on your feedback.

https://codereview.chromium.org/1090413005/diff/120001/sync/syncable/director...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/1090413005/diff/120001/sync/syncable/director...
sync/syncable/directory_backing_store_unittest.cc:4100: // won't keep it all in
memory. If this test fails, you may need to increase
On 2015/04/23 21:53:27, Nicolas Zea wrote:
> Hmm, do we control how many entries SQLite will keep in memory, or is that
> something it does on its own?

It's complicated and I don't fully understand it.  Here's what I know.

With 1000 entries, I found that sqlite didn't read the file before writing to it
and so it didn't notice the corruption.  With 2000 entries it did read the file
before writing.  I set the num_entries value to 4000 to be safe.  There is some
threshold here and it's not entirely clear to me on which factors it depends.

I suspect the threshold depends on the DB page size and the cache size
(controlled by a PRAGMA).  Possibly more.

http://www.sqlite.org/sessions/pragma.html#pragma_cache_size

My guess is that with larger cache and larger page size, we'd need a larger
number of entries.

One thing I can do is to add a comment at the ASSERT that would fail if the
current value of num_entries becomes in sufficient.  That might at least help a
future maintainer understand that num_entries may need to be increased.

[Nicolas Zea, 2015-04-24 00:53:39]

lgtm

[maniscalco, 2015-04-24 15:24:15]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2015-04-24 15:24:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1090413005/140001

[commit-bot: I haz the power, 2015-04-24 15:31:07]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-04-24 15:32:01]

Patchset 7 (id:??) landed as
https://crrev.com/9f5d1bfcc8d3f856c921c8910f005f305df128c3
Cr-Commit-Position: refs/heads/master@{#326802}