Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PRAGMA synchronous=0; causes database disk image is malformed for IDBBatchAtomic VFS #111

Closed
tantaman opened this issue Aug 21, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@tantaman
Copy link
Contributor

When responding to #23 (comment) it made me wonder if setting PRAGMA schema.synchronous = 0 would have any impact on OPFS perf. IIUC, this would be similar behavior to relaxed durability that we have with IDB but with OPFS.

Running the wa-sqlite benchmarks with:

-- Pre-run setup
PRAGMA journal_mode=delete;
PRAGMA synchronous=0;

causes Test 6: Creating an index to always fail with

Error: database disk image is malformed
    at check (https://rhashimoto.github.io/wa-sqlite/src/sqlite-api.js:857:11)
    at Object.step (https://rhashimoto.github.io/wa-sqlite/src/sqlite-api.js:690:14)
    at async execute (https://rhashimoto.github.io/wa-sqlite/src/examples/tag.js:50:16)
@rhashimoto
Copy link
Owner

rhashimoto commented Oct 27, 2023

For some reason, SQLite disables batch atomic writes with PRAGMA synchronous=OFF. In spite of this, SQLite also isn't creating a journal file for most write transactions (Test 3 is an exception; it does create a journal file for that one). This almost guarantees that any mid-transaction crash will corrupt the database.

That doesn't explain this error, but it is curious and points away from batch atomic writes being the cause.

FYI setting the synchronous mode to 0 (off) is almost always a bad idea and is definitely not an alternative way to get relaxed durability. Relaxed durability won't corrupt your database - it just means that some of the most recent transactions may not be preserved. Turning synchronous off is an invitation to database corruption and should be avoided by all but the most expert or foolhardy (or both).

@rhashimoto
Copy link
Owner

Hmm. It's looking like this may be a bug in SQLite itself. I'm thinking that the lack of a journal file noted above is also a bug, perhaps a related bug.

I added some temporary code to IDBBatchAtomicVFS to shadow all the VFS writes into a memory buffer and then compare the VFS read results with the contents in the buffer. This successfully verified that whatever SQLite reads from the VFS is exactly what it wrote earlier, all the way to the reported error.

Next I just removed SQLITE_IOCAP_BATCH_ATOMIC from the capabilities that IDBBatchAtomicVFS claims, and then the benchmarks ran with no error (including with my shadowing). This provides support (though not proof) that the IndexedDB storage code is working fine. Interestingly, under these conditions SQLite now uses a journal file as expected.

So I suspect there are some broken interactions in SQLite between batch atomic and synchronous=off. Unfortunately, I doubt the SQLite folks are going to believe that without a test case written entirely in vanilla C or C++, and I don't think that's a great investment of my time for a mode that almost no one should really be using.

@rhashimoto rhashimoto added bug Something isn't working wontfix This will not be worked on labels Oct 27, 2023
@rhashimoto
Copy link
Owner

@rhashimoto
Copy link
Owner

When I figured out the cause of bug #118, I realized that this bug was really the same thing. It was confusing at first because the other bug does batch atomic writes and that doesn't happen here. But the underlying problem with incorrect file size metadata is the link. The fix works for both.

@rhashimoto rhashimoto removed the wontfix This will not be worked on label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants