Instead it is relying on SQLITE_FCNTL_SYNC, which is only applied on database files according to the doc (emphasis added):

Thank you for pointing that out. That VFS was my first ever and it's very possible that "mistakes were made." Its approach to syncing was based off of one of the demo VFSes and may well not be appropriate for the journal files. i'll look into that immediately.

Instead it is relying on SQLITE_FCNTL_SYNC, which is only applied on database files according to the doc (emphasis added):

That's now fixed in src:a371374148a2874b. That "sync dichotomy" was a topic in our dev chat channel last summer, in the context of the OPFS VFS, but the details of that discussion escape me.

All of the existing tests continue to work with this change, and a small amount of debug output shows that it's now syncing multiple file handles per test.

The demo/testing site and prerelease snapshot have been updated.

Thanks again for the feedback!

If I'm reading that patch correctly, all you did was implement xSync. I think you went from not syncing enough to syncing too much - if you still sync on SQLITE_FCNTL_SYNC then that will sync database files twice in succession. Too much is a whole lot better than too little, but it will cost performance (a noticeable amount, I would guess).

If I'm reading that patch correctly, all you did was implement xSync.

That's correct.

... if you still sync on SQLITE_FCNTL_SYNC then that will sync database files twice in succession.

That's my instinct as well, but when this topic came up last summer internally we decided (for reasons lost to memory and likely above my proverbial pay grade) that the SQLITE_FCNTL_SYNC approach was needed for this case. i do recall that syncing of the db did not happen when i expected it to via xSync(), which prompted me to ask Richard or Dan about it and i was directed to the SQLITE_FCNTL_SYNC approach.

Too much is a whole lot better than too little, but it will cost performance (a noticeable amount, I would guess).

FWIW, that's also my instinct, but our I/O-heavy benchmarks aren't unduly affected by this change. Specifically, we test the overall OPFS performance using a WASM build of the project's "speedtest1" binary:

https://wasm-testing.sqlite.org/speedtest1-worker.html?vfs=opfs&size=25

The benchmarks before and after this change look close enough that i can't say for certain that there's been any impact, but my comparison was admitted cursory. i will do a more careful before/after analysis/comparison of this later this evening and/or tomorrow and add some debugging traces to find out whether duplicate syncing is a genuine issue.

i will do a more careful before/after analysis/comparison of this later...

The performance hit for duplicate syncing (once via xFileControl() and once via xSync()) is very roughly 10% in the speedtest1 benchmarks (noting that they specifically stress I/O over computation). However, the FCNLT_SYNC approach is apparently still required because of this note in the docs:

Or, if the xSync method is not invoked because the user has configured SQLite with PRAGMA synchronous=OFF it [xFileControl] is invoked in place of the xSync method.

The VFS doesn't have ready access to the state of that pragma, so can only guess it based on the way xFileControl/xSync are called (or not). i will get some brains added to it to avoid the superfluous syncs.

The performance hit for duplicate syncing (once via xFileControl() and once via xSync()) is very roughly 10% in the speedtest1 benchmarks (noting that they specifically stress I/O over computation).

Is that with or without --big-transactions? If it's only with then I think you're avoiding the workloads where it matters most. That's not necessarily unreasonable - 140K write transactions might take over an hour - but only if you're not misleading yourself on what things cost.

However, the FCNTL_SYNC approach is apparently still required because of this note in the docs:

Obviously the SQLite team defines what PRAGMA synchronous=off requires, but I would argue that if I turn it off I don't expect (or want) any syncing to be done. The docs seem to agree with me, so perhaps this is a documentation bug:

With synchronous OFF (0), SQLite continues without syncing as soon as it has handed data off to the operating system...

The VFS can track the state of this pragma via SQLITE_FCNTL_PRAGMA so you could make the behavior conditional on that, but I would argue that ignoring SQLITE_FCNTL_SYNC is more compatible with the current documentation.

Is that with or without --big-transactions?

It's always been unusably slow without that flag, so we always use that for OPFS tests. That flag was in fact added specifically for OPFS, but predates our own VFS: the same slowdowns were evident when using OPFS over Emscripten's WASMFS, which we leaned on heavily until we had our VFS in place.

That's not necessarily unreasonable - 140K write transactions might take over an hour - but only if you're not misleading yourself on what things cost.

Misleading myself may well be part of the problem! i'm currently working on logic to suppress any unnecessary syncs. Perhaps we'll see a boost after that.

The VFS can track the state of this pragma via SQLITE_FCNTL_PRAGMA

How did i not know about that?

Some preliminary new timings should be out later tonight (CET) or tomorrow at the latest.

Thank you again for your continued feedback.

i'm currently working on logic to suppress any unnecessary syncs.

My argument is that all syncs are unnecessary if synchronous=off so you don't need to handle SQLITE_FCNTL_SYNC at all. I think if you disagree then you need to change all the VFS implementations in the amalgamation as well as OPFS. And the documentation.

My argument is that all syncs are unnecessary if synchronous=off so you don't need to handle SQLITE_FCNTL_SYNC at all.

The culprit has been located...

Looking back through the source history from that time, i pulled in the FCNTL_SYNC handling as-is from the kvvfs, where that handling is (per the associated checkin comments) necessary so that kvvfs will also work properly even when synchronous=off. The OPFS VFS code was started shortly after that, and the kvvfs code was one of my starting points. So the use of FCNTL_SYNC in the OPFS VFS boils down to a copy/paste error on my part.

A version which removes the xFileControl() handling entirely is currently under test. Assuming no weird errors show up in testing, it will land in trunk in the next hour or three. (Edit: src:f809de7f232c8c2731a8)

Thank you again!