search

riverqueue / river

Fast and reliable background jobs in Go
https://riverqueue.com
Mozilla Public License 2.0
3.22k stars 86 forks source link

Fully functional `database/sql` driver #351

Closed brandur closed 1 month ago

brandur commented 3 months ago

Here, implement the rest of driver functionality on riverdatabasesql, the existing driver for Go's built-in database/sql package. Previously it only supported a minimal interface allowing it to run migrations, but nothing more sophisticated like inserting jobs.

The benefit of a fully functional driver is that it will allow River to be integrated with with other Go database packages that aren't built around Pgx like Bun (requested in #302) and GORM (requested in #58). I'll need to write up some documentation, but this change should make both of those integrations possible immediately.

It also lays the groundwork for future non-Postgres drivers. It's going to be a little more still, but I want to take a stab at SQLite, and this change will get us a lot of the way there.

There's no way with database/sql to support listen/notify, so here we introduce the idea of a poll only driver. River's client checks whether a driver can support listen/notify on initialization, and if not, it enters poll only mode the same way as if configured with PollOnly.

An intuitive idiosyncrasy of this set up is that even when using the database/sql driver bundled here, regardless of whether they're working with Bun, GORM, or whatever, users will generally still be using Pgx under the hood since it's the only maintained and fully functional Postgres driver in the Go ecosystem. With that said, the driver still has to bundle in lib/pq for various constructs like pq.Array because we're using sqlc, and sqlc's database/sql driver always uses lib/pq. I tried to find a way around this, but came out fairly convinced that there is none. To rid ourselves of lib/pq completely we'd need sqlc to ship an alternative Pgx driver that used Pgx internally, but exposed a database/sql interface using *sql.Tx instead of pgx.Tx.

bgentry commented 3 months ago

There's no way with database/sql to support listen/notify, so here we introduce the idea of a poll only driver. River's client checks whether a driver can support listen/notify on initialization, and if not, it enters poll only mode the same way as if configured with PollOnly.

I think we've discussed this possibility before, but wanted to raise it again as it's relevant here. If users are actually using pgx under the hood of database/sql or Bun, it shouldn't be too difficult for us to use a raw pgx connection for the listener—provided the driver's constructor gives us a way to create one.

We can't rely on a single conn never failing, so we'd need to at least be able to recreate a provided conn from its ConnConfig, OR we could accept a listener-specific pgxpool.Pool as part of the constructor so that we can steal conns from it as needed. We also talked about having different constructors within a single driver that give access to different functionality, so even if we don't add this now it doesn't preclude us doing so in the future. I think a change/addition like this would bring this database/sql driver pretty much to feature parity though!

With that said, the driver still has to bundle in lib/pq for various constructs like pq.Array because we're using sqlc, and sqlc's database/sql driver always uses lib/pq. I tried to find a way around this, but came out fairly convinced that there is none. To rid ourselves of lib/pq completely we'd need sqlc to ship an alternative Pgx driver that used Pgx internally, but exposed a database/sql interface using *sql.Tx instead of pgx.Tx.

Dang, I wonder how tough of a lift this would be? Especially since iirc pgx has complete support for most of its types within database/sql. I tried searching for issues related to this and only found https://github.com/sqlc-dev/sqlc/issues/2960 that was relevant.

Regarding the rest of this, I'm not gonna get through a full review this evening but should be able to get through it in the morning.

brandur commented 3 months ago

I think we've discussed this possibility before, but wanted to raise it again as it's relevant here. If users are actually using pgx under the hood of database/sql or Bun, it shouldn't be too difficult for us to use a raw pgx connection for the listener—provided the driver's constructor gives us a way to create one.

I don't love this — IMO, this sort of internal magic would be quite surprising and there'd be no intuitive way to predict that it might happen. It also puts a Pgx dependency back into riverdatabasesql — not the worst thing, but not great.

There's a much better way to do this which is that if you're using Pgx but still need database/sql in some places (say you're using Bun): (1) initialize one work client with riverpgxv5, and (2) initialize a second insert client that you use with Bun. Check out the latest blog post teed up in for the home page — there's a code sample in there of what it'd look like.

Dang, I wonder how tough of a lift this would be? Especially since iirc pgx has complete support for most of its types within database/sql. I tried searching for issues related to this and only found https://github.com/sqlc-dev/sqlc/issues/2960 that was relevant.

I don't think it'd be that hard as far as adding drivers to Sqlc goes (overall, it doesn't seem to use that much lib/pq, just a few functions here and there), but any new Sqlc driver looks like kind of a lot of work :/ This is one that I'm kind of hoping that if we wait long enough someone will eventually do it, but certainly more than I want to commit to right now.

Regarding the rest of this, I'm not gonna get through a full review this evening but should be able to get through it in the morning.

Awesome, thx.

brandur commented 1 month ago

@bgentry I need to base another PR on this one so gonna bring it for now, although won't cut a release right away.