Experiment: replacing modernc.org/sqlite with github.com/ncruces/go-sqlite3 in gotosocial

[NyaaaWhatsUpDoc]

Just starting an open discussion for this so I can document things as I experience them, and serves as a useful space for any related discussion to this Go sqlite implementation. I'm not 100% sure this is something we'd look at PR'ing to main, but there's definitely a very spirited discussion between our core devs on matrix :D.

So far there's a working build replacing modernc.org/sqlite with github.com/ncruces/go-sqlite3 here: https://github.com/NyaaaWhatsUpDoc/gotosocial/tree/experiment/use-wasm-sqlite3

I might look at upstreaming the funky little driver wrapper I made (see ./internal/db/sqlite) as it makes wrapping the driver yourself easier if you need to, and i've kept our standard retryOnBusy() handler in place over using the built-in busy-timeout handler both due to it not being context aware (though change should be easy enough, i have a change for y'all ready if you want it upstream) and as i'm not sure if they 100% replace one-another.

This working build runs gotosocial as expected, with decent performance, minus a memory leak (which doesn't occur on current main) which causes the application to get oom killed every few hours. With the fact we're using github.com/uptrace/bun (which in turn wraps database/sql, which then uses our busy / error handling wrapper...) it's hard to narrow down exactly where it's being introduced, but i shall keep looking!

Anyways, i hope at least someone here in this discussion board finds this interesting! Either way i'll probably continue to ramble with my progress as i make it :)

[ncruces]

Not sure this is the best forum for this discussion (rather than something under github.com/superseriousbusiness/gotosocial), where it might have more visibility of interested parties? I don't mind hosting it here either, so that's fine.

Any specific questions/concerns you might have for me, please make them more explicit, 'cause at first glance I don't think I can contribute much to the discussion.

I'll still open a few threads for things that come up.

[NyaaaWhatsUpDoc]

I'm one of the main gotosocial developers, just we don't really use our discussion board and I figured this project's discussion board might be more useful for others looking to make the switch and seeing this thread, whether it provides info or just encouragement. Though ultimatley I'm not hugely fussed where it ends up so I'm happy if you'd rather move it :)

[ncruces]

Busy handler not being context aware

The philosophy of this driver is that the busy handler is the maximum you'll ever want to wait. Queries should still bail out early if a context is cancelled. If this doesn't happen, I consider it a bug.

Acting on context done isn't immediate (it's hard to interrupt file IO, and I don't even try). Also, the default timeout handler sleeps increments of 100ms, so you can always miss cancellation by at least that much. But I don't think you should ever miss context cancellation by more than (e.g.) 250ms.

[NyaaaWhatsUpDoc]

Does the busy handler with a timeout set handle retries? Having looked at the sqlite docs multiple times for this I'm not 100% sure whether it does or not. If it does then it seems with it enabled it should render our retryOnBusy() wrapping code useless, which would be great since less wrapping code :p

[ncruces]

Yes, that's the entire point of it.

If the database is locked (returning SQLITE_BUSY, SQLITE_LOCKED mostly applies to shared cache which we don't use), the default busy handler will sleep in increments of at most 100ms, and retry up to the busy timeout. If you set the busy timeout high enough (I recommend 1min), you can then use contexts to cancel early instead (the error in this case will be SQLITE_INTERRUPT).

The only situations where this doesn't happen are those that would lead to dead lock: two connections waiting "forever" on each other until one gives up. Then, one of the connections returns SQLITE_BUSY immediately so the other can proceed.

If this is returned by our driver before the timeout expires, you're in the middle of a transaction where you read data that might be/become stale, and you need to abort the entire transaction to retry. This is not something that can be done transparently.

However, the above situation can be avoided in all cases.

To avoid SQLITE_BUSY ever being returned, you need to follow one simple rule: don't upgrade read transactions into write transactions. If one transaction is going to write to any attached database, use BEGIN IMMEDIATE. With the database/sql, driver this can be achieved by using _txlock=immediate (making write transactions the default), then using read-only transactions for the ones that don't write (you'll get SQLITE_READONLY if you try).

To avoid SQLITE_LOCKED, you need avoid dropping tables/indices from the same connection and in the middle of stepping queries on those tables/indices. This is a hard mistake to make with database/sql, so I wouldn't worry about it.

PS: if you get lots of timeouts and busy errors, do remember to Close your sql.Rows, sql.Stmt, and sql.Conn, and either Commit or Rollback your sql.Tx. No amount of waiting or retrying will fix these mistakes.

[NyaaaWhatsUpDoc]

This is very informative thank you! I tried removing our retryOnBusy() wrapping code now, and with the busy-timeout all still seems to be working as expected. We were aware of setting the busy timeout previously, but I'm not entirely certain the modernc.org/sqlite implementation behaves entirely as expected. (or maybe we were just using it incorrectly, i'm not sure, i don't want to point all blame at them)

[ncruces]

ExecContext is weird

we hide ExecerContext as it follows a weird logic path in sql.DB, being only a short-path for stmts with no args.

Exec can execute multiple statements (e.g. PRAGMAS or DDL/DML) in one go. Having no parameters and no results makes this saner, and reduces subtle bugs.

Everything else (including Query) goes through Prepare , which does the opposite and errors out if there are multiple statements.

This is my attempt at enforcing some correctness properties.

[NyaaaWhatsUpDoc]

useful to know, thank you!

[ncruces]

Error translation

In general I'd advising going with idiomatic Go and using this pattern for error translation:
https://github.com/ncruces/go-sqlite3/blob/main/gormlite/error_translator.go

Obviously, that's just a matter of style and you may prefer to keep it how you did it. This is just to say that this driver tries to, as much as possible, respect Go idioms in error handling.

[NyaaaWhatsUpDoc]

Ah interesting! This is a much nicer way of doing things, and more idiomatic I'll agree, though in ours since it's not a publicly exported library I wasn't as worried about that and instead was striving for performance, however small the difference. (since errors.Is() relies on reflection)

[NyaaaWhatsUpDoc]

Update on the situation: memory leak resolved. It was me adding QueryContext() and ExecContext() support to our SQLiteConn{} wrapper causing it. The statements being prepared must not have ended up closed for whatever reason (though i did hook rows.Close() to do this, i was probably missing something somewhere). So entirely a "me" problem 😅

All (bar two things) seems to be working as expected now, with less code than we had previously for the modernc.org/sqlite implementation, since the busy timeout handler can now successfully be relied on!

The only 2 things remaining are a particularly slow running query (again this could be an issue of our own making, no fingers being pointed here), which we don't experience under modernc.org/sqlite, and our tests seem to fail using "addr=$randomUUID?mode=memory&cache=shared" connection string which previously we never had issues with.

[ncruces]

Slow queries

A slow query could easily be "our fault."

Our compiler is different ("Wasm+wazero" vs. "c2go+go") so CPU bottlenecks could differ.
Our storage engine is different (we reimplement the OS layer) so IO bottlenecks could differ.

One way we may be different (though I'm not sure, because I'm unsure of modernc internals) is if the query does large sorts.

SQLite can do sorts in parallel, if configured to do so. We don't support this, as Wasm is single threaded (and I don't plan to change this even with threads support: single threading is a choice here).

[daenney]

I don't believe we currently set/configure worker threads in GoToSocial for SQLite, so I suspect that at least is not that.

[ncruces]

If you can capture a pprof of the running query, I can maybe look into it.
Although, it'll probably be a not very instructive "running native code" which is SQLite, basically.

[NyaaaWhatsUpDoc]

why the choice for single threading, even if wasm adds thread support, out of interest? (not trying to push your answer on this, just curious!).

later i'll try capture a CPU profile via pprof, take a look and share the results. again thank you for all the input here!

[ncruces]

By assuming C/Wasm is single threaded, we avoid a lot of complexity.

SQLite can be compiled without threads, without mutexes; malloc will also run without mutexes.
Each connection is sandboxed in its own address space, and shares nothing with other connections.

Basically, each SQLite instance acts like a single-threaded process isolated from the world, and needs Go to act as an OS for it to interact with outside world.

Every time I tried to change this (for shared cache, for shared memory), I got into nothing but pain.

[ncruces]

So, question: do you see this getting some traction? Any blockers remaining?

[ncruces]

Oh that makes sense! SQLITE_ENABLE_STAT4 is enabled in modernc.

I'll review these compile options (modernc and mattn) and tweak the binary after SQLite 3.46. I've already tested the beta, and will test any flag changes, so there should be no surprises.

That said. You can always use your own custom build, if you have specific needs. My intent is that this is explicitly supported. But I also want to provide a good default experience.

[NyaaaWhatsUpDoc]

Exactly that was my thoughts. Tweaking the opts to our needs wouldn't be too difficult as a fork to keep up to date (if that's even the best way of doing it), there may be easier ways. The only part of SQLITE_ENABLE_STAT4 I wasn't sure if you'd want to make as a default opt was the fact they mention that "the downside of this compile-time option is that it violates the query planner stability guarantee making it more difficult to ensure consistent performance in mass-produced applications.".

[daenney]

I think by default query stability guarantee isn't enabled even in SQLite, you have to set SQLITE_ENABLE_QPSG. So unless this build already does that, enabling STAT4 shouldn't have any negative consequences.

[ncruces]

The options modernc includes and I don't (mattn doesn't either, for the most part) are:

• SQLITE_ENABLE_STAT4
• SQLITE_ENABLE_COLUMN_METADATA
• SQLITE_ENABLE_DBSTAT_VTAB
• SQLITE_ENABLE_MEMORY_MANAGEMENT
• SQLITE_ENABLE_OFFSET_SQL_FUNC
• SQLITE_ENABLE_SNAPSHOT
• SQLITE_ENABLE_SESSION
• SQLITE_ENABLE_PREUPDATE_HOOK
• SQLITE_ENABLE_RBU

For now I'm inclined to include SQLITE_ENABLE_STAT4. The others I may include upon request. Most require APIs for wrapping, so if you find this via search, open an issue. Some are not useful with a Wasm build. And usefulness will be weighed against Wasm code size increase.

[ncruces]

Exactly that was my thoughts. Tweaking the opts to our needs wouldn't be too difficult as a fork to keep up to date (if that's even the best way of doing it), there may be easier ways.

You don't need to fork. You need to create your own version of package embed, and maybe pin a version in go.mod for stability. Or ask me to provide (e.g.) embed/stat4, and support it.

I think by default query stability guarantee isn't enabled even in SQLite, you have to set SQLITE_ENABLE_QPSG. So unless this build already does that, enabling STAT4 shouldn't have any negative consequences.

I don't. And you can DBCONFIG_ENABLE_QPSG if you want query stability. The only question is how much SQLITE_ENABLE_STAT4 bloats the Wasm (I don't expect very much). But every little feature does, a bit.

And the only big downside of bloating the Wasm, is that opening the first DB connection becomes slower, as the wazero compiler is a little slow.

[ncruces]

So, this got merged in superseriousbusiness/gotosocial#2863!

If you're ever serious about releasing a FreeBSD binary with this enabled, something needs to be done about #85.

Otherwise, what you have should be good for both Linux and macOS.

[daenney]

Right, GtS is packaged on the BSDs 😅. Thanks for the shout, we need to keep that in mind.