SQLiteNIO

Right, I think we're diverging quite far from this pitch now, right?

I think we're confusing async vs. sync APIs, async vs. sync I/O, etc with batching. IIRC, what Helge suggests is that there should be an API that allows you to batch multiple commands such that the DB connection isn't held by the client longer than absolutely necessary. And this applies irregardless of the API or the I/O being asynchronous or synchronous.

As mentioned I still think we are generally on topic wrt the proposal as the thread hopping issue is not addressed. To me such an API (for PG too, but specifically for SQLite) feels like encouraging the wrong thing.

I don't talk about batching, but actual, synchronous code execution. Like:

db.fetch()
do some Swift work, but dear nothing async
db.update()
do some Swift work, bdna
...

That's what (DB) backend code commonly looks like (unless you use PL/Swift).
It should proactively discourage (ideally forbid) async work and stay on a bg thread.

Also: "such that the DB connection isn't held by the client longer than absolutely necessary" implies that one shouldn't pass control back to the event loop while a DB connection is assigned (because that would interleave the batches w/ other queue work at the expense of either thread hopping or starving PG connections).

P.S.: Just wanted to raise my concern, feel free to ignore :slight_smile: (i.e. we don't have to discuss this to death).

It doesn't matter whether "do some Swift work" is sync or async, what matters is that it's guaranteed to be "fast enough". Or did you want to imply with "nothing async" that it's not meant to do any I/O because I/O can usually take an arbitrarily long time?

If you did

db.fetch()
synchronouslyParse100MBOfJSON()
db.update()
synchronouslyTypecheckASwiftProgram()

then we have synchronous code but still, we'd be holding onto the DB connection for far too long.

I assume that you're now talking about SQLite, right? In PostgresNIO's case, there are no background threads and no thread switches, the I/O happens straight from the EventLoop, using NIO.

In PostgresNIO's case, there is not necessarily thread hopping, if you want to avoid it you can run the connection to PostgreSQL on the same EL as you are (AFAIK Vapor always co-locates the Channel to Postgres with the Channel that delivers the HTTP request/response). But even if you have thread hopping, that likely won't be an issue because the sending, processing, and receiving the query/result is usually a lot slower than a few thread hops. Sure, not thread hopping is better than thread hopping but if we're waiting for the DB to reply to us, then the thread hop likely won't be the thing that kills us.

If we can batch queries, then that's even better because we can send them off at the same time rather than waiting for the result of the previous query.

Lastly (I don't think this matters for this debate at all), SwiftNIO executes future callbacks immediately, so you won't suffer any delays because of the EventLoop. So let's say you had this code:

let el = group.next()
el.execute {
    let promise = el.makePromise(of: Void.self)
    
    for i in 0..<1000 {
        el.execute { print("\(i)") }
    }
    
    promise.futureResult.whenComplete { _ in
        print("future callback")
    }
    
    promise.succeed(())
}

What you'll see printed is

future callback
0
1
2
3
4
[...]
999

I think providing a way to make SQLite and NIO play nice is valuable. However, I do not think the API in the package is the right abstraction.

Some examples:

  • The API doesn't make it possible to efficiently insert multiple records (reusing the same prepared statement, possibly not even changing all of the bound parameters after a step).
  • The Statement type queries a bunch of information from SQLite that might not be needed, and then copies it around with every Row.
  • There are no ways to conditionally read some columns from query results.
  • You can't customize flags the connection is opened with.
  • You can't customize the busy handler, which currently just waits forever (I think this can lead to deadlocks, but I'm not quite sure right now).

Now, don't get me wrong: I'm not worried about all that functionality not being available in the package right now. What I do worry about is that the architecture doesn't seem to be designed in a way that allows for functionality like that to be slotted in later. I think this stems from the fact that the package tries to be both a swifty SQLite interface as well as a NIO adapter at the same time.

One thing that sets SQLite apart from other SQL databases (in my experience at least; I haven't worked as much with others as I have with SQLite) is that there is a bunch of functionality in the C interface that can and should be employed to both improve performance and enable advanced use-cases.

For someone looking to do more than just fire off a few non-performance-sensitive queries, looking to use even one bit of the C interface, the package as it exists right now is, sadly, completely useless: Even if going down to the C interface would be acceptable (which is awful to use directly thanks to the pervasive use of opaque types), the package doesn't provide access to the underlying handles.


To address these shortcomings, I would propose an alternative architecture:

First, a target that is a very thin (and as little overhead as possible) wrapper to make the C API a tad more usable in Swift. This should really be not much more than an apinotes overlay could provide: Make functions into methods, replace OpaquePointer occurrences with proper types, and possibly convert result codes into thrown errors, if that can be done without too much overhead.

This target could then be used as base for all sorts of SQLite packages, related to NIO or not (crucially, this should work on all platforms that both SQLite and Swift support, even if NIO does not).

Second, a target that provides integration with NIO. From my experience using SQLite in concert with Dispatch and OperationQueue so far, it would probably for the moment suffice to have a database-queue type that has a single method func run<T>(_ block: (Connection) -> T) -> EventLoopFuture<T>, but I think it might be possible to extend and keep the existing interface around (although maybe with deprecations sprinkled generously over it) to avoid a breaking change as well.

Any convenience APIs not directly related to NIO can then be implemented (either in this or in other packages) on top of the wrapper, which would make them usable both with the NIO adapter as well as without, enabling more code reuse across the ecosystem.


Wrt. linking SQLite statically, I think that is an interesting enhancement, especially if in the future SPM gained a feature-flag system that would allow easy customization of which parts of the amalgamation are included. I do think keeping the ability to use the system version is important though, so as long as only one is possible (or is there a SPM feature that would allow the user to choose?) the system library is probably the way to go.

When it comes to renaming the symbols to avoid collisions, I do not think that is a good idea. For one, it would make using the system provided SQLite more difficult to impossible, and it would also mean that to use SQLite extensions written in C (both those provided by the SQLite project as well as third-party), one would have to fiddle around with the sources. Just 's/sqlite3_/foobar_sqlite3_/g' also won't cut it, because there are at least constants (and maybe macros?) to deal with, as well as internal functions, types and constants that do not follow the sqlite3_* naming scheme.

The SQLite C interface also doesn't change very often and (afaik) only in backwards-compatible ways, so I'm not too worried there are issues to be solved here.

6 Likes

Renaming symbols would only occur if we vend a copy of SQLite.c in the library. If we decide to use the system provided version we won't rename them.

With regards to adding SQLite extensions written in C, you'd need to fork the package and add them yourself. To be frank, the Vapor Core team have no desire to test and maintain any extensions outside of what we need. If you want to add extra stuff then that's up to you to do in a fork.

With regards to your alternative APIs on top of the C library - that is not something we can do unfortunately. The SQLiteNIO package is already released and tagged and in use. The pitch is purely about whether the community wants it accepted in to the SSWG incubation process and whether we need to add any potential non-breaking changes. Wrt building a package that doesn't rely on NIO, that goes against the best practices and requirements for SSWG packages. To be clear, this pitch is solely aimed at providing an SQLite package for the server, any additional platform support is a bonus and not a first class citizen. Adding Swift APIs for all the C functions available in SQLite would be a significant undertaking.

Finally got around to doing the actual formal proposal document for this: SQLiteNIO proposal by gwynne · Pull Request #82 · swift-server/sswg · GitHub

3 Likes