Fix ATC mounting of SQLite DBs using WAL journaling #5633
Conversation
|
This is an amazing find! Thank you so much! Just one nit and a couple questions |
|
Does this cover all sqlite based tables, or just ATC ones? (perhaps a naive question, are there non-ATC ones) |
|
There are non ATC sqlite tables (especially in macOS) and this shouldn't affect those. There are also tables that do utilize this API though and will follow this modified code path. However, the effect should be a no-op because the table wouldn't have ever worked if those databases were WAL to start with. |
So this will fix things that use this code path. Things that don't use this code path will be unaffected (obviously), but perhaps we can eventually move everything to this code path? |
|
As a side note, I think we need to get CLA coverage before we can merge this. But I'm a big 👍 here |
|
@directionless I did sign the CLA thing a couple hours before making the PR. Maybe it's still syncing? |
Correct, my only point was that I don't think there could be any existing tables being fixed by this because the table never would have work before this. I think. Yes, that was part of the idea behind the ATC backend, to help move all SQLite code into a single place. If someone wants a lot of landed lines, moving all those macOS tables that still use their own file handling and reading would be an awesome diff. |
3bc33ec
to
5559f1f
Compare
|
@obelisk @theopolis Rebased on upstream master and squashed changes into a single commit. Testing all green now. |
This fix addresses issue #5225.
This idea came from reading the SQLite WAL documentation where it states:
It didn't clearly state that "shared-memory" requires the locking primitives to be enabled, but that's what I inferred since you usually don't want concurrent things to freely mess around with memory.
So I tried to load a WAL DB as ATC flipping the
respect_lockingparameter and it ended up working properly (hurray).I'm sure the original reason why it was chosen to use the
*-none(no-op locking primitives) VFSes by default is to avoid getting rejections from locked databases, but that's not the case with WAL since (again from the SQLite docs):That said, the logic of this fix is to simply test the journal mode with a PRAGMA statement before loading the table. The possible outcomes are:
*-noneVFS (as it happened before);*-noneVFS (as it happened before);I tried to add the code in a place which looked more or less appropriate and to use the routines that I found to be available at a quick glance through the codebase. If there is a better way to write this, please point it out to me and I'll try address such suggestions.
Tested on MacOS 10.14.5 against Mozilla Firefox (WAL) and Google Chrome (non WAL) history databases.