Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1685073003:
[sqlite] Replace sqlite3Btree* calls with info from header. (Closed)
|
Created:
4 years, 10 months ago by Scott Hess - ex-Googler Modified:
4 years, 10 months ago Reviewers:
Ryan Hamilton CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@zzsql_recover_handle_review1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sqlite] Replace sqlite3Btree* calls with info from header.
Replace sqlite3BtreeGetOptimalReserve() with reading directly from the SQLite
header. Also use the header to replace sqlite3BtreeGetPageSize(), though it
could also be replaced with "PRAGMA page_size". Also read the encoding from the
header rather than using a PRAGMA.
BUG=584407
Committed: https://crrev.com/01e4fcc8143e031d29b94ab412eeb902a90191a3
Cr-Commit-Position: refs/heads/master@{#375083}
Patch Set 1 #
Total comments: 7
Patch Set 2 : No static const as array size. Silly Visual Studio. #
Total comments: 1
Patch Set 3 : And apparently clang is less pedantic than Visual Studio about unused variables. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (10 generated)
shess@chromium.org changed reviewers: + rch@chromium.org
Getting to the end of the line, here. https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... File third_party/sqlite/src/src/recover.c (left): https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... third_party/sqlite/src/src/recover.c:658: static int getEncoding(sqlite3 *db, const char *zDb, int* piEncoding){ Now calculated by GetPager(). https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... File third_party/sqlite/src/src/recover.c (right): https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... third_party/sqlite/src/src/recover.c:240: /* From section 1.2. */ This refers to https://www.sqlite.org/fileformat2.html#database_header https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... third_party/sqlite/src/src/recover.c:522: rc = pFile->pMethods->xRead(pFile, header, sizeof(header), 0); This is basically pread(). https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... third_party/sqlite/src/src/recover.c:526: return SQLITE_CORRUPT; This case cannot be reached in the current code, I believe. When I attempt to attach a database busted in this way, things fail at the attach, so this code can't be called. But SQLITE_IOERR_SHORT_READ could easily be confusing if returned to the calling code, because it's not really an I/O error. https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... third_party/sqlite/src/src/recover.c:532: nPageSize = decodeUnsigned16(header + kiHeaderPageSizeOffset); Offset 16. decodeUnsigned16() decodes a 16-bit big-endian value in a manner consistent with how SQLite stores things everywhere else. Section 1.2.2 references support for 64k pages. I actually didn't realize this, but IMHO it's hinky enough that I'm going to ignore it at this time. I don't want to have to audit the code again. Chromium only supports 32k pages. https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... third_party/sqlite/src/src/recover.c:539: nReservedSize = header[kiHeaderReservedSizeOffset]; This guy is somewhat confusing. Section 1.2.4 indicates that it should be capped at 32 for 512-byte pages, but I could find no evidence that the SQLite code does this. To test this code I have to manually inject changes into SQLite, because I've never found a standalone system to test it with (AFAICT, there are a few closed-source users out there). Chromium will never have this non-zero, unless someone is contriving to link against a modified SQLite or something. https://codereview.chromium.org/1685073003/diff/1/third_party/sqlite/src/src/... third_party/sqlite/src/src/recover.c:542: iEncoding = decodeUnsigned32(header + kiHeaderEncodingOffset); As long as we're here, this was just a lot easier to decode this way than having the complicated PRAGMA (deleted later). I believe the values assigned to *piEncoding are exactly the values being tested with literals, but I couldn't tell if section 1.2.13 was making a guarantee or not.
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/patch-status/1685073003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685073003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm Nice improvements. Thanks for the annotations in the CL.
No static const as array size. Silly Visual Studio.
Thanks for the review. Minor fix posted based on compile failures in previous dry run, because "static const" is not const enough for Visual Studio to use as an array size (presumably because of the possibility of ODR violations). https://codereview.chromium.org/1685073003/diff/20001/third_party/sqlite/src/... File third_party/sqlite/src/src/recover.c (right): https://codereview.chromium.org/1685073003/diff/20001/third_party/sqlite/src/... third_party/sqlite/src/src/recover.c:247: enum { knHeaderSize = 100}; My immediate next thought was "Why not put all the constants in the enum?" But rabbit holes are deep, so I'm avoiding thinking like that. And the enum will probably fail on a random other platform, leaving me with a #define.
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/patch-status/1685073003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685073003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
And apparently clang is less pedantic than Visual Studio about unused variables.
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/patch-status/1685073003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685073003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shess@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/1685073003/#ps40001 (title: "And apparently clang is less pedantic than Visual Studio about unused variables.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1685073003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685073003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [sqlite] Replace sqlite3Btree* calls with info from header. Replace sqlite3BtreeGetOptimalReserve() with reading directly from the SQLite header. Also use the header to replace sqlite3BtreeGetPageSize(), though it could also be replaced with "PRAGMA page_size". Also read the encoding from the header rather than using a PRAGMA. BUG=584407 ========== to ========== [sqlite] Replace sqlite3Btree* calls with info from header. Replace sqlite3BtreeGetOptimalReserve() with reading directly from the SQLite header. Also use the header to replace sqlite3BtreeGetPageSize(), though it could also be replaced with "PRAGMA page_size". Also read the encoding from the header rather than using a PRAGMA. BUG=584407 Committed: https://crrev.com/01e4fcc8143e031d29b94ab412eeb902a90191a3 Cr-Commit-Position: refs/heads/master@{#375083} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/01e4fcc8143e031d29b94ab412eeb902a90191a3 Cr-Commit-Position: refs/heads/master@{#375083} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
