Summary: | Regression(r277571) Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() from ITP fails | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, darin, ggaren, katherine_cheney, sam, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=259284 | ||||||||
Attachments: |
|
Description
Chris Dumez
2021-09-03 12:40:16 PDT
Created attachment 437299 [details]
Patch
Comment on attachment 437299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437299&action=review > Source/WebCore/platform/sql/SQLiteDatabase.cpp:630 > int autoVacuumMode = 0; > - if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) > + int error = 0; > + if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) { > autoVacuumMode = statement->columnInt(0); > - int error = lastError(); > + error = lastError(); > + } Seems like we can straighten out the logic a little. If we can’t prepare the statement, we return false, but for some reason we implement that by setting error to "0" and then comparing it with SQLITE_ROW. I would write: auto statement = prepareStatement("PRAGMA auto_vacuum"_s); if (!statement) return false; // <Move here and/or improve long confusing comment about error codes that does not even mention SQLITE_ROW>? auto autoVacuumMode = statement->columnInt(0); if (lastError() != SQLITE_ROW) return false; I like that better than having a local variable named error. The code below also doesn’t need a local variable. return lastError() == SQLITE_OK; (In reply to Darin Adler from comment #3) > Comment on attachment 437299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=437299&action=review > > > Source/WebCore/platform/sql/SQLiteDatabase.cpp:630 > > int autoVacuumMode = 0; > > - if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) > > + int error = 0; > > + if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) { > > autoVacuumMode = statement->columnInt(0); > > - int error = lastError(); > > + error = lastError(); > > + } > > Seems like we can straighten out the logic a little. If we can’t prepare the > statement, we return false, but for some reason we implement that by setting > error to "0" and then comparing it with SQLITE_ROW. I would write: > > auto statement = prepareStatement("PRAGMA auto_vacuum"_s); > if (!statement) > return false; > > // <Move here and/or improve long confusing comment about error codes > that does not even mention SQLITE_ROW>? > auto autoVacuumMode = statement->columnInt(0); > if (lastError() != SQLITE_ROW) > return false; > > I like that better than having a local variable named error. The code below > also doesn’t need a local variable. > > return lastError() == SQLITE_OK; Your proposal is extending the lifetime of the statement. I was trying to keep the same lifetime as before r277571 (which is why I had added the scope in the first place). Maybe it doesn't matter though. Created attachment 437308 [details]
Patch
Comment on attachment 437308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437308&action=review > Source/WebCore/platform/sql/SQLiteDatabase.cpp:625 > + auto statement = prepareStatement("PRAGMA auto_vacuum"_s); I'm not sure if there is a consequence to having this statement call finalize after the statement inside executeCommand or runVacuumCommand, but I think this might change behavior more than we want it to. Comment on attachment 437308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437308&action=review >> Source/WebCore/platform/sql/SQLiteDatabase.cpp:625 >> + auto statement = prepareStatement("PRAGMA auto_vacuum"_s); > > I'm not sure if there is a consequence to having this statement call finalize after the statement inside executeCommand or runVacuumCommand, but I think this might change behavior more than we want it to. I thought about it and I personally don't see how it changes behavior. Yes, 2 statements will live at the same time but I don't see any issue with that (except that we're using a very tiny amount of extra memory for a little bit longer). Committed r282030 (241334@main): <https://commits.webkit.org/241334@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437308 [details]. |