Statistics plugin sync: fix bloated null id_books #10749
Conversation
plugins/statistics.koplugin/main.lua
Outdated
| @@ -428,6 +428,14 @@ Please wait… | |||
| end | |||
| end | |||
| end | |||
| if not self.settings.fixed_null_idbook then | |||
There was a problem hiding this comment.
You could probably just increment the db version thing (I don't recall the exact name, and am on mobile right now, sorry) instead of adding a setting key that'll stay around.
There was a problem hiding this comment.
It's just that the current migration function will backup the db, which might be 2GB! And I'm reluctant to give that workflow a special case :(
There was a problem hiding this comment.
Ah, good point, I'd forgotten about that.
Perhaps we can just move that whole bit of logic to a onetimemigration block instead of the plug-in itself, then?
There was a problem hiding this comment.
Done. Didn't really know about the onetime_migration choice.
| if last_migration_date < 20230727 then | ||
| logger.info("Performing one-time migration for 20230727") | ||
| local conn = SQ3.open(DataStorage:getSettingsDir() .. "/statistics.sqlite3") | ||
| local ok, value = pcall(conn.exec, conn, "PRAGMA table_info('page_stat_data')") |
There was a problem hiding this comment.
Is that OK if the statistics plugin is disabled (and the user has deleted the db) ?
There was a problem hiding this comment.
plugin is disabled
Still performs the deletion, should be fine and intended behavior.
the user has deleted the db
A warning will be shown in the log, seems an empty db will be created due to SQ3.open(). Should not be too big a deal though?
There was a problem hiding this comment.
You could pcall(SQ3.open, ...) just for consistency ?
(I guess it's more likely to happen than your db not compatible check :) )
There was a problem hiding this comment.
"Could not open blah" warnings are the norm for migration blocks, FWIW. We only pcall stuff that would otherwise crash ;).
There was a problem hiding this comment.
Added check for existence of file.
| local db_location = DataStorage:getSettingsDir() .. "/statistics.sqlite3" | ||
| if util.fileExists(db_location) then | ||
| local conn = SQ3.open(db_location) |
There was a problem hiding this comment.
Can it happen the file exists, but SQ3.open fails ? ie. db too big, file system full, db corrupted, or fs fuckup and chkdsk makde it empty, would SQ3.open throw an exception or just go on with it ?
There was a problem hiding this comment.
We actually guard the actual production code in the plugin far less defensively than this, so I don't think we have anything to worry about ;).
There was a problem hiding this comment.
I don't want to look how precisely we pcall() plugins when loading them, and if the init() is included - in which case a bad db would just disable the statistics plugin - while this happening here would prevent KOReader from launching ?
Ok, had a look:
function ReaderStatistics:init()
+ abc = def.ghi
07/28/23-18:59:59 ERROR Failed to initialize statistics plugin: plugins/statistics.koplugin/main.lua:105: attempt to index global 'def' (a nil value)
No statistics menu.
Corrupting my statistics.sqlite3 (dd bs=123 count=1), the plugin loads, in spite of:
07/28/23-19:01:51 ERROR Failed to initialize statistics plugin: common/lua-ljsqlite3/init.lua:60: ljsqlite3[corrupt] database disk image is malformed
its menu is shown, some functions does not work, some crash, but reading and changing books work.
Putting this at top of onetime_migration.lua:
local db_location = DataStorage:getSettingsDir() .. "/statistics.sqlite3"
if util.fileExists(db_location) then
local conn = SQ3.open(db_location)
logger.warn("DB CORRUPTED BUT I'M OK")
endhave us not crash:
07/28/23-19:07:45 WARN DB CORRUPTED BUT I'M OK
So, I guess it's ok if only what happens after is pcall()ed. (but who knows what happens with some other kind of corruption?)
|
Some conflict preventing the merge. (So, might as well update the date to the current day.) |
|
Updated date and resolved conflict. |
|
Oops, my bad, thought we had merged this already ;). |
|
Updated today to -54, works OK. There are now two new files along statistics.sqlite, one is the same name with -shm suffix, another with -wal suffix. Is this supposed to happen? |
|
As long as the database has an open handle, yep; that's SQLite's WAL journaling handling. |
Crap, we don't close that one ;p. |
|
I was going to say, should it really be around the entire time the program's open? |
|
Thanks! :D |
|
Yeah, we tend to close them explicitly ASAP instead of relying on the GC to do it for us. |
|
The fix worked OK! Can this debloat be also applied to book cache db? I feel like it is bloated too, 42M. |
It may have stalled stuff from books you have removed or when you rename a directory (the bookinfo cache from all the books would stay duplicated with the old full path and the new full path). You can do some cleaning yourself: |
|
Thanks! I’ll try prune. Not sure about Compact though, last time I used it (about a year ago), KOReader crashed :D |
|
Or I think I asked that question already, now I remember the difference that prune only empties the removed books places, but keeps allocation. So that new books will just replace these empty places. Compact OTOH is supposed to shrink db. |
This PR is an attempt to fix #10342 and #10656. According to #10656 (comment), the bug could be
null id_booksinpage_stat_data. I didn't really find out why that happened from the sync code but still added some "fail-proofs".Also it would be great if @libelle1995 and @xsdjt could try out this solution.
This change is