Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9125018:
Skeleton of SQLite virtual-table module to recover data from corrupt databases. (Closed)
|
Created:
8 years, 11 months ago by Scott Hess - ex-Googler Modified:
8 years, 11 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSkeleton of SQLite virtual-table module to recover data from corrupt databases.
"recover" implements a virtual table which uses the SQLite pager layer
to read table pages and pull out the data which is structurally sound
(at least at the storage layer).
This CL implements the virtual-table interface, including schema, with
some mock data for purposes of landing some initial tests. This CL
should cause no changes to Chromium, as it does not get compiled.
BUG=109482
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117359
Patch Set 1 #
Total comments: 40
Patch Set 2 : Vandebo's comments. #Patch Set 3 : Mask constants to macros, upread README.chromium, copyright disclaimer on recover.c. #
Messages
Total messages: 11 (0 generated)
OK, here's the outer layer of things. Initially, I would prefer a pre-review, where you look it over and decide whether or not you're willing to commit to it. As mentioned in the thread, there is this much code again, and that code is less boilerplatey than this code. Of course, if someone says "zOMG, this is awesome, gimme more!", I'll be happy with that, too. [Basically, I'd rather have a rubber-stamp review where the reviewer only cares a little than get someone backed into a corner over it. Unfortunately, I'm going OOT for a few weeks on the 20th. No pressure.] For reference purposes only, I've put my entire change up at: http://codereview.chromium.org/9110047 As of this instant, that change is a proper git branch off this change. Everything above the custom SQLite functions in recover.c will definitely go in. Also some of the build-system stuff will go in, but I need to figure out the right way for that (SQLITE_RECOVER macro? amalgamation?). Anyhow, it can land on top of this change as: - leaf-reader code and tests. - interior-reader code and tests. - overflow code and tests. each of which is a few hundred relatively-isolated lines (I mean I can comment/mock out a small number of lines and land actual working code at each point). Thanks for speaking up, and no problem if you want to pass. I spent about a year passing on this problem.
On 2012/01/07 01:32:36, shess wrote: > For reference purposes only, I've put my entire change up at: > http://codereview.chromium.org/9110047 > As of this instant, that change is a proper git branch off this change. Here's a version of that change taken as a baseline from this change: http://codereview.chromium.org/9144001/ so the recover.c stuff shows what will be added there. Also the test files are likely to be keepers. But the build-related stuff is way subject to change.
On 2012/01/07 01:32:36, shess wrote: > OK, here's the outer layer of things. Initially, I would prefer a pre-review, > where you look it over and decide whether or not you're willing to commit to it. I am going to review this.
On Mon, Jan 9, 2012 at 5:21 PM, <vandebo@chromium.org> wrote: > On 2012/01/07 01:32:36, shess wrote: >> OK, here's the outer layer of things. Initially, I would prefer a >> pre-review, >> where you look it over and decide whether or not you're willing to commit >> to it. > > I am going to review this. > > http://codereview.chromium.org/9125018/ If there is anything at all I can do to expedite, let me know. I probably can't fly down for a couple days of driving a whiteboard. But if that would make the difference, I'll be darned if I wouldn't ask Mark about it :-). [It's not that it's that important, more that I want to make sure I don't lose momentum, as these kinds of changes tend to bitrot quickly when not checked in.] -scott
Aside from the potential memory leaks and nits, this change looks good.... In that it is internally consistent and jives with what I remember from SQLite and what I decided to look up. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... File third_party/sqlite/src/src/recover.c (right): http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:191: static const unsigned char kMaskNull = (1<<SQLITE_NULL); nit: put these in numerical order? http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:203: assert( iSerialType>=12 ); Are there constants for serial type that should be used here and below? http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:257: if( !strcmp(zTable, "sqlite_master") ){ nit: The dominate sqlite style seems to be strcmp(...)==0 http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:390: pCursor->iRowid = 0; init to 0 not necessary http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:410: } gMockData[5] = { gMockData[] http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:539: int r = zText>*pzWordStart; /* In case pzWordStart==pzWordEnd */ nit: no need to save r, just return the comparison. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:544: /* Return true if the next word is zText, also setting *pzContinue to nit: next word *in zText is zWord* http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:551: !strncasecmp(zWord, zWordStart, zWordEnd - zWordStart) ){ strncasecmp(...)==0 http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:582: struct { static? http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:587: } typeInfo[7] = { typeInfo[] http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:587: } typeInfo[7] = { kTypeInfo ? http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:602: if( !strncasecmp(typeInfo[i].zName, *pzTypeStart, nNameLen) ){ ==0 http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:655: static int ParseAndGenerateCreate(int nCols, const char *const *pCols, nit: ParseColumnsAndGenerateCreate ? http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:670: &iTypeMask); pass iTypes + i directly ? http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:683: zCreateSql = sqlite3_mprintf("%z%.*s %.*s%s, ", Looks like this leaks the old zCreateSql each time through the loop. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:693: /* Strip trailing ", ". */ Instead of stripping the trailing , and reallocating the string, the mprintf in the loop can have an extra %s with 'i-1<nCols ? "," : ""' as the value. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:697: zCreateSql = sqlite3_mprintf("%z)", zCreateSql); mem leak http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:728: if( strcasecmp(argv[1], "temp") ){ !=0 http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:747: if( dot && dot>argv[3] ){ for ".a" do you want zDb to be db->aDb[0].zName and zTable to be "a" or is a zTable of ".a" ok?
On 2012/01/11 00:41:55, vandebo wrote: > Aside from the potential memory leaks and nits, this change looks good.... In > that it is internally consistent and jives with what I remember from SQLite and > what I decided to look up. Thanks for the run-through! In addition to the changes in response to comments, when writing a test for the "db." and ".table" cases, they were already failing for un-related reasons, so I altered the error code in that case. Then went ahead and did a half-assed job of adding some other errors. I generally don't include user-provided data in error messages, due to experience dealing with logs from systems which include user-provided data in error messages. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... File third_party/sqlite/src/src/recover.c (right): http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:191: static const unsigned char kMaskNull = (1<<SQLITE_NULL); On 2012/01/11 00:41:55, vandebo wrote: > nit: put these in numerical order? Hmm. At one point the shifting was functional, but at this point re-using the SQLITE_ constants is really not worth anything. I'll just replace them with constants and be done with it. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:203: assert( iSerialType>=12 ); On 2012/01/11 00:41:55, vandebo wrote: > Are there constants for serial type that should be used here and below? AFAICT from vdbeaux.c, the sqlite3VdbeSerial*() functions are all coded using bare constants. Looking at it, I'm somewhat wondering if the SerialTypeIsCompatible() wouldn't be better written in the style of: case 0 : return (mask&kNullMask)!=0; case 1 : return (mask&kIntegerMask)!=0; ... or possibly dropping the !=0. It got the way it is because earlier I needed to allow integer or float encoding for float columns, because SQLite will encode things that way. I later changed the affinity handling to be up in the parse/create code, so now it could be cleaner. I'll try that. [Note that this switch statement will appear in other contexts in the future, they are more like series-of-case-statements.] http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:257: if( !strcmp(zTable, "sqlite_master") ){ On 2012/01/11 00:41:55, vandebo wrote: > nit: The dominate sqlite style seems to be strcmp(...)==0 Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:390: pCursor->iRowid = 0; On 2012/01/11 00:41:55, vandebo wrote: > init to 0 not necessary Call me pedantic. The iRowid is setting up my pre-condition, the memset() is because I'm paranoid. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:410: } gMockData[5] = { On 2012/01/11 00:41:55, vandebo wrote: > gMockData[] Done, and static'ed it, and gMockData will go away in the next CL :-). http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:539: int r = zText>*pzWordStart; /* In case pzWordStart==pzWordEnd */ On 2012/01/11 00:41:55, vandebo wrote: > nit: no need to save r, just return the comparison. Think carefully about the comment and the next line. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:544: /* Return true if the next word is zText, also setting *pzContinue to On 2012/01/11 00:41:55, vandebo wrote: > nit: next word *in zText is zWord* Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:551: !strncasecmp(zWord, zWordStart, zWordEnd - zWordStart) ){ On 2012/01/11 00:41:55, vandebo wrote: > strncasecmp(...)==0 Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:582: struct { On 2012/01/11 00:41:55, vandebo wrote: > static? Done. ... that was odd. Won't compile. "error: initializer element is not constant". Happens even as a static at file scope. I can't assign to the mask values. -O6 suppresses the errors for strictMask, but otherMask still complains. As you might expect, converting them to #define fixes it right up. That would mean kMaskRowid -> MASK_ROWID or something on that order. WDYT? kMaskRowid isn't really SQLite-style anyhow. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:587: } typeInfo[7] = { On 2012/01/11 00:41:55, vandebo wrote: > typeInfo[] Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:587: } typeInfo[7] = { On 2012/01/11 00:41:55, vandebo wrote: > kTypeInfo ? Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:602: if( !strncasecmp(typeInfo[i].zName, *pzTypeStart, nNameLen) ){ On 2012/01/11 00:41:55, vandebo wrote: > ==0 Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:655: static int ParseAndGenerateCreate(int nCols, const char *const *pCols, On 2012/01/11 00:41:55, vandebo wrote: > nit: ParseColumnsAndGenerateCreate ? Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:670: &iTypeMask); On 2012/01/11 00:41:55, vandebo wrote: > pass iTypes + i directly ? Hmm. I think earlier code had more local post-processing before setting the pTypes[i] value. Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:683: zCreateSql = sqlite3_mprintf("%z%.*s %.*s%s, ", On 2012/01/11 00:41:55, vandebo wrote: > Looks like this leaks the old zCreateSql each time through the loop. I think this is a somewhat recent addition for use cases like this. From http://www.sqlite.org/c3ref/mprintf.html : 'The "%z" formatting option works like "%s" but with the addition that after the string has been read and copied into the result, sqlite3_free() is called on the input string.' http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:693: /* Strip trailing ", ". */ On 2012/01/11 00:41:55, vandebo wrote: > Instead of stripping the trailing , and reallocating the string, the mprintf in > the loop can have an extra %s with 'i-1<nCols ? "," : ""' as the value. At various points this code has varied between use of a zSep variable and not. Why it ended up here was that once too often I got the %s messed up WRT parameters. Looking at it, I find myself wondering why I left the mprintf to add a ), rather than overwriting the ", ". I'll make a change there and see what you think. Aaaand, now that I think about it, I realize that your suggestion could be adapted to add the trailing ) on-the-fly, saving an entire sqlite3_mprintf()! http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:728: if( strcasecmp(argv[1], "temp") ){ On 2012/01/11 00:41:55, vandebo wrote: > !=0 Done. http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:747: if( dot && dot>argv[3] ){ On 2012/01/11 00:41:55, vandebo wrote: > for ".a" do you want zDb to be db->aDb[0].zName and zTable to be "a" or is a > zTable of ".a" ok? It should fail finding the root page in that case, but I think it's reasonable to be explicit. I've recoded things to disallow ".table" and "db.".
LGTM http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... File third_party/sqlite/src/src/recover.c (right): http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:539: int r = zText>*pzWordStart; /* In case pzWordStart==pzWordEnd */ On 2012/01/11 18:55:07, shess wrote: > On 2012/01/11 00:41:55, vandebo wrote: > > nit: no need to save r, just return the comparison. > > Think carefully about the comment and the next line. Oh, I see. I think I read that as findWord not finding anything instead of the intended meaning. (i.e. *pzWordStart==*pzWordEnd) http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:582: struct { On 2012/01/11 18:55:07, shess wrote: > On 2012/01/11 00:41:55, vandebo wrote: > > static? > > Done. > > ... that was odd. Won't compile. "error: initializer element is not constant". > Happens even as a static at file scope. I can't assign to the mask values. > -O6 suppresses the errors for strictMask, but otherMask still complains. > > As you might expect, converting them to #define fixes it right up. That would > mean kMaskRowid -> MASK_ROWID or something on that order. WDYT? kMaskRowid > isn't really SQLite-style anyhow. Strange... apparently C says that an initializer can't be a const qualified object. #define's seem fine - they seem at least twice as popular as static const in sqlite. I'm also ok not changing this, since it's O(columns) and not O(rows). http://codereview.chromium.org/9125018/diff/1/third_party/sqlite/src/src/reco... third_party/sqlite/src/src/recover.c:683: zCreateSql = sqlite3_mprintf("%z%.*s %.*s%s, ", On 2012/01/11 18:55:07, shess wrote: > On 2012/01/11 00:41:55, vandebo wrote: > > Looks like this leaks the old zCreateSql each time through the loop. > > I think this is a somewhat recent addition for use cases like this. From > http://www.sqlite.org/c3ref/mprintf.html : > > 'The "%z" formatting option works like "%s" but with the addition that after the > string has been read and copied into the result, sqlite3_free() is called on the > input string.' Oh, I see; I missed that. Certainly handy for cases like this.
(Sorry for the delay. If you are interested in me looking at this I hope to have time very soon, but don't feel like you have to wait.)
On 2012/01/11 21:46:00, vandebo wrote: > LGTM Awesome! Thanks for the expedient review, now I can cycle to the next bit. > > As you might expect, converting them to #define fixes it right up. That would > > mean kMaskRowid -> MASK_ROWID or something on that order. WDYT? kMaskRowid > > isn't really SQLite-style anyhow. > > Strange... apparently C says that an initializer can't be a const qualified > object. #define's seem fine - they seem at least twice as popular as static > const in sqlite. I'm also ok not changing this, since it's O(columns) and not > O(rows). I'm converting them to macros. It's the C way, after all. Also, I decided to shut up the presubmit check with a trivial edit to README.chromium, and I added the non-copyright header to recover.c. In case anyone is here wondering what the heck I was thinking, I used the header from fts3.c, based on having written and contributed that module to SQLite earlier in my Google employment.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shess@chromium.org/9125018/11001
Change committed as 117359 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
