Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() from ITP fails since r277571 and logs: 0x104de9900 - ResourceLoadStatisticsDatabaseStore::turnOnIncrementalAutoVacuum failed, error message: not an error
<rdar://82581507>
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].