Fix ATC mounting of SQLite DBs using WAL journaling

[piax93]

This fix addresses issue #5225.
This idea came from reading the SQLite WAL documentation where it states:

WAL normally requires that the VFS support shared-memory primitives...

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_locking parameter 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):

WAL provides more concurrency as readers do not block writers and a writer does not block readers. Reading and writing can proceed concurrently.

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:

• the DB uses WAL -> PRAGMA statement returns "wal" and the table is loaded with default VFS (locking enabled);
• the DB uses something else and it's not locked -> PRAGMA statement returns "something else" and the table is loaded with a *-none VFS (as it happened before);
• the DB uses something else and it's locked -> PRAGMA statement fails and the table is loaded with a *-none VFS (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.

[obelisk]

This is an amazing find! Thank you so much! Just one nit and a couple questions

[directionless]

Does this cover all sqlite based tables, or just ATC ones? (perhaps a naive question, are there non-ATC ones)

[obelisk]

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.

[directionless]

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?

[directionless]

As a side note, I think we need to get CLA coverage before we can merge this.

But I'm a big 👍 here

[piax93]

@directionless I did sign the CLA thing a couple hours before making the PR. Maybe it's still syncing?

[obelisk]

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?

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.

[theopolis]

Hey @obelisk, do you approve the changes here? If so can you approve in the review tool?

@piax93, if you don't mind rebasing onto the current master we should see the tests pass (aiming for this before merging).

[piax93]

@obelisk @theopolis Rebased on upstream master and squashed changes into a single commit. Testing all green now.

[directionless]

Prior to rebase, this had been approved by @obelisk

I think it's okay. Tests are green

CLA tool isn't hooked up, but @piax93 says they signed it.