OPFSAnyContextVFS: publish writes where SQLite ends them

[lalexdotcom]

OPFSAnyContextVFS keeps writes and truncations in a FileSystemWritableFileStream, and nothing it holds is visible to another context until it is closed. Only xSync and the context's own next read close it today, and SQLite does not always sync after its last change.

What happens today

• VACUUM. The truncation that ends a VACUUM comes after the transaction's last xSync, while the lock is still held. A context that takes the lock next gets the file at its old size. On Firefox its File then fails with AbortError once the writer closes its stream, which xRead returns as SQLITE_IOERR_READ.
• locking_mode=EXCLUSIVE with synchronous=OFF. There is no xSync and no unlock, so a commit stays in the open stream and is lost if the context goes away without closing the database.
• I/O errors. When a cache spill write fails, the pager gives up the transaction without syncing what it had written, and releases the lock. The context's next read closes the stream with no lock held. Closing replaces the whole file, so it discards whatever another context committed in between.

The change

• jFileControl closes the stream on SQLITE_FCNTL_SYNC, which SQLite sends at every commit whatever PRAGMA synchronous says, and on SQLITE_FCNTL_COMMIT_PHASETWO, which follows the commit's truncation. After SQLITE_FCNTL_OVERWRITE, i.e. during a VACUUM, it waits for the latter, as IDBMirrorVFS does. jSync skips its close in that case and still closes for every other file.
• jUnlock closes any stream still open before releasing the lock, as a backstop for the error paths. A failed close is reported as SQLITE_IOERR_UNLOCK, and the lock is released either way.
• jFileSize answers from the open stream instead of closing it. pager_truncate() asks for the size just before truncating, so a VACUUM now copies the database once instead of twice.

Test

test/vfs_publication.js has a worker of its own whose VFS can make database writes fail, and is wired into OPFSAnyContextVFS.test.js:

• context A's cache spill fails; B rolls the hot journal back and commits a row; A reads; the row must survive;
• with locking_mode=EXCLUSIVE and synchronous=OFF, A commits a row and its worker is terminated; B must see the row;
• a VACUUM opens one stream on the database, and the file has its truncated size right after.

test/vfs_xUnlock.js calls the VFS directly: A writes, syncs, truncates with no xSync after it and unlocks, and B must see the truncated size.

yarn web-test-runner test/OPFSAnyContextVFS.test.js

With master's VFS, all four fail on asyncify and jspi (Expected 200 to be 201, Expected 2 to be 1, Expected 110592 to be 61440, Expected 8192 to equal 4096). With this change, the file's 120 tests pass, 3 runs of 3, and also on Firefox outside the suite. The whole suite: 6142 passed, 0 failed.

Checklist

• I grant to recipients of this Project distribution a perpetual,
  non-exclusive, royalty-free, irrevocable copyright license to reproduce, prepare
  derivative works of, publicly display, sublicense, and distribute this
  Contribution and such derivative works.
• I certify that I am legally entitled to grant this license, and that this
  Contribution contains no content requiring a license from any third party.

[rhashimoto]

That is fine as long as SQLite syncs after its last change, but it does not always: the truncation at the end of a VACUUM comes after the last xSync of the transaction.

I think this should not happen. I asked the SQLite folks.

[lalexdotcom]

Thanks for checking it with the CLI — your trace shows the same sequence. It looks deliberate: in SQLite 3.53.0, the version wa-sqlite builds, pager_end_transaction() shrinks the file after the journal is finalized, while EXCLUSIVE is still held, and nothing syncs after it (pager.c L2145-2152):

This branch is taken when committing a transaction in rollback-journal mode if the database file on disk is larger than the database image. At this point the journal has been finalized and the transaction successfully committed, but the EXCLUSIVE lock is still held on the file. So it is safe to truncate the database file to its minimum required size.

Growing the file, by contrast, happens in sqlite3PagerCommitPhaseOne() before the sync (L6658-6671).

Whatever the answer on the forum, I don't think the VFS can count on an xSync after the last change: with PRAGMA synchronous=OFF, SQLite never calls xSync at all (L3617). I checked it on Chrome with a two-context test: context A sets synchronous, creates a table, inserts 3 rows and stays open; context B then counts them. On master, B counts 0 with synchronous=OFF, on both asyncify and jspi, and 3 with NORMAL and FULL. With this PR it counts 3 in every case. Closing the writable in xUnlock covers both: whatever the lock covered is published before another context can take it.

I've also merged master into the branch. It predated the JSPI detection fix, so the PR's test now runs on jspi too, and fails there on master the same way (Expected 8192 to equal 4096).

Happy to add the synchronous=OFF test to the PR if you'd like.

[rhashimoto]

It looks deliberate: in SQLite 3.53.0

You're right, and a response on the forum by someone knowledgeable concurs. This appears to be intentional.

Happy to add the synchronous=OFF test to the PR if you'd like.

No, not necessary. The only reason to use it is to get better write performance, but (1) this is not the VFS to use for better write performance, and (2) it's not going to get any faster if we just make the same calls in unlock anyway.

[lalexdotcom]

Fair enough on the test. One nuance on (1), though: OPFSAnyContextVFS is not the VFS for write performance, but it is the only OPFS VFS here that serves concurrent reads without readwrite-unsafe, so on Firefox and Safari it is the one to pick when that is the main need. An application that picks it for that can still reach for synchronous=OFF to win back some write speed, and on master that silently hides its commits from other contexts. With this PR it does not, whatever it gains or not in speed, so a test would guard correctness rather than performance. I'll leave it out unless you'd like it in.

[rhashimoto]

I have apparently encountered this issue before but I don't remember it at all! It is handled in IDBMirrorVFS here.

The best place to close the writable for VACUUM truncate is in jFileControl() for opcode SQLITE_FCNTL_COMMIT_PHASETWO, which is sent after truncation. But that doesn't handle database changes that don't end with a commit (e.g. ROLLBACK) so we also need to close for opcode SQLITE_FCNTL_SYNC (which arrives regardless of any PRAGMA synchronous setting). Every commit will send both opcodes, which would write the entire database twice in the VACUUM truncate scenario, so skip the SQLITE_FCNTL_SYNC close if we have seen SQLITE_FCNTL_OVERWRITE. That's what IDBMirrorVFS does.

Using jUnlock() mostly works, but affects durability with PRAGMA locking_mode=EXCLUSIVE because the database is not unlocked when a transaction ends. One of the main lessons I had to learn in writing VFSs (more than once!) is not to use locking to infer transaction boundaries.

IDBMirrorVFS only needs to sync main database files to storage, but OPFSAnyContextVFS has to handle writables for all its files. SQLITE_FCNTL_SYNC is only delivered for database files, so closing is still needed injSync() for other files.

Summary:

• Add a File boolean to track overwrite state.
• Implement jFileControl():
  • SQLITE_FCNTL_OVERWRITE - Set overwrite state.
  • SQLITE_FCNTL_SYNC - Close writable if not overwriting. Clear overwrite state.
  • SQLITE_FCNTL_COMMIT_PHASETWO - Close writable.
• Modify jSync() to close writable only if not a database file (or not overwriting).

There are a couple painful cases, but both have workarounds:

1. With PRAGMA synchronous=FULL, which is the default, SQLite syncs the journal file, modifies the journal header, and syncs the journal file again. This double sync in this VFS will write the entire journal file twice. We could detect this and ignore the first sync, but users can easily work around it with PRAGMA synchronous=NORMALwhich is believed to be safe on modern filesystems.

2. Using PRAGMA auto_vacuum=FULL will overwrite the entire database twice on any transaction that reduces the database page count, once for the transaction data and once for the truncation, because this doesn't send the overwrite opcode. The workaround here is don't do that.

[lalexdotcom]

Thanks for pointing me to IDBMirrorVFS, and for the lesson about locks and transaction boundaries. Before reworking the PR I tried your design next to the current one. I ran both in the test runner with two workers, on Chromium and Firefox and on both builds. I'd like your opinion on one point first.

You're right about locking_mode=EXCLUSIVE. With synchronous=OFF, a context inserts rows and then goes away without closing. This PR loses those rows, and your design keeps them.

There is one path your design doesn't cover: I/O errors. If a cache spill write fails, the pager goes to PAGER_ERROR (pager.c L4656). sqlite3PagerRollback() then returns without playback (L6777), and the lock is released with the spilled pages still in the writable. The same happens when a commit write fails and the rollback playback fails too (L2977, L6811). Neither SQLITE_FCNTL_SYNC, COMMIT_PHASETWO nor xSync comes after.

The writable is then closed by that context's next read, with no lock held. Closing it replaces the whole file, so it discards anything another context committed in between. I tested this with a write failure injected in the VFS. Context A fails. Context B recovers the hot journal and commits a row. Then A reads, and B's row is gone. That happens on master and with your design. Also closing in jUnlock keeps the row.

So I'd propose your design as you laid it out, plus the close in jUnlock as a backstop. It wouldn't be used to find transaction boundaries, only to make sure the lock is never handed over with a writable still open. That combination was the only one that kept every commit in those tests.

Two smaller points:

• VACUUM still copies the database twice here, with either design. pager_truncate() calls xFileSize just before truncating (L2669), and jFileSize closes the writable. Getting a single copy would need jFileSize to answer without closing, for example by tracking the size through writes and truncations. Do you want that in this PR?
• Your summary clears the overwrite flag on SQLITE_FCNTL_SYNC, but the xSync that follows would then close the writable anyway. I'd clear it on COMMIT_PHASETWO, as IDBMirrorVFS does.

Looking at IDBMirrorVFS for the same kind of problem, it seems to have one of its own, when the IndexedDB transaction in #commitTx aborts (on a quota error, for instance). With synchronous=full, the commit fails, but the failed rows come back with the context's next commit. With normal, the abort goes unnoticed, and the database stored in IndexedDB ends up failing integrity_check. I could send you a separate PR for that.

If that sounds right to you, I'll rework this PR along those lines and add a test for the error path.

[rhashimoto]

So I'd propose your design as you laid it out, plus the close in jUnlock as a backstop. It wouldn't be used to find transaction boundaries, only to make sure the lock is never handed over with a writable still open.

I was considering leaving that in jUnlock() as a backstop but I couldn't think of a way to get there. You demonstrated a way, so I'm all for it.

Do you want [file size tracking to prevent VACUUM double sync] in this PR?

That's a good observation. As you can probably guess, optimizing write performance for this VFS isn't a priority for me so that isn't something I particularly want. However, it isn't going to add a lot of complexity so it isn't something I object to either. If you want it, I would be happy to review and include it.

Your summary clears the overwrite flag on SQLITE_FCNTL_SYNC, but the xSync that follows would then close the writable anyway. I'd clear it on COMMIT_PHASETWO, as IDBMirrorVFS does.

We can keep it in SQLITE_FCNTL_COMMIT_PHASETWO like IDBMirrorVFS. My concern was that if VACUUM failed then SQLITE_FCNTL_COMMIT_PHASETWO won't be called and the overwrite state won't be reset. But my argument to change it is weak because (1) SQLITE_FCNTL_SYNC won't necessarily be called either, and (2) I haven't yet been able to come up with a scenario where anything bad actually happens after that.

I could send you a separate PR for [IDBMirrorVFS].

Yes, please, if you're up for that. Thanks!

[lalexdotcom]

Reworked along those lines: your design, with the close in jUnlock kept as a backstop, and jFileSize answering from the open writable. A VACUUM now opens one writable on the database instead of two. I went ahead with the size tracking after all; it stays small. The overwrite flag is cleared on SQLITE_FCNTL_COMMIT_PHASETWO, as agreed.

The new test/vfs_publication.js covers the failed cache spill, locking_mode=EXCLUSIVE with synchronous=OFF, and the single copy. Each fails with master's VFS. I also checked them against the previous head and against your design without the backstop: each of those misses its own case. The file passes 3 runs of 3, also on Firefox, and the whole suite 6142 passed, 0 failed. I merged master and rewrote the description.

I'll open the IDBMirrorVFS PR separately.

[rhashimoto]

Thanks for helping to figure all this out! LGTM.