Support for `database/sql` in migrations + framework for multi-driver River #98

brandur · 2023-12-03T21:17:35Z

Here, add a new minimal driver called riverdriver/riversql that
supports Go's built-in database/sql package, but only for purposes of
migrations. The idea here is to fully complete #57 by providing a way of
making rivermigrate interoperable with Go migration frameworks that
support Go-based migrations like Goose, which provides hooks for
*sql.Tx [1] rather than pgx.

riverdriver/riversql is not a full driver and is only meant to be used
with rivermigrate. We document this clearly in a number of places.

To make a multi-driver world possible with River, we have to start the
work of building a platform that does more than riverpgxv5's "cheat"
workaround. This works by having each driver implement specific database
operations like MigrationGetAll, which target their wrapped database
package of choice.

This is accomplished by having each driver bundle in its own sqlc that
targets its package. So riverpgxv5 has an sqlc.yaml that targets
pgx/v5, while riversql has one that targets database/sql. There's
some sqlc.yaml duplication involved here, but luckily both drivers can
share a river_migration.sql file that contains all queries involved,
so you only need to change one place. river_migration.sql also migrates
entirely out of the main ./internal/dbsqlc.

The idea here is that eventually ./internal/dbsqlc will disappear
completely, usurped entirely by driver-specific versions. As this is
done, all references to pgx will disappear from the top-level package.
There are some complications here to figure out like LISTEN/NOTIFY
though, and I'm not clear whether database/sql could ever become a
fully functional driver as it might be missing some needed functionality
(e.g. subtransactions are still not supported after talking about them
for ten f*ing years [2]. However, even if it's not, the system would let
us support other fully functional packages or future major versions of
pgx (or even past ones like pgx/v4 if there's demand).

river/riverdriver becomes a package as it now has types in it that
need to be referenced by driver implementations, and this would
otherwise not be possible without introducing a circular dependency.

Notably, this development branch has to use some go.mod replace
directives to demonstrate that it works correctly. If we go this
direction, we'll need to break it into chunks to release it without
them:

~~Break out changes to river/riverdriver. Tag and release it.~~
Break out changes to riverdriver/river* drivers. Have them target
the release in (1), comment out replaces, then tag and release them.
Target the remaining River changes to the releases in (1) and (2),
comment out replaces, then tag and release the top-level driver.

Unfortunately future deep incisions to drivers will require similar
gymnastics, but I don't think there's a way around it (we already have
this process except it's currently two steps instead of three). The hope
is that these will change relatively rarely, so it won't be too painful.

[1] https://github.com/pressly/goose#go-migrations
[2] golang/go#7898

brandur · 2023-12-03T21:22:24Z

go.mod

+
+replace github.com/riverqueue/river/riverdriver/riverpgxv5 => ./riverdriver/riverpgxv5
+
+replace github.com/riverqueue/river/riverdriver/riversql => ./riverdriver/riversql


See note in PR description, but these replaces are needed for now to show that the PR works. If we bring it in, the change will be broken up into three chunks and all the replaces removed before final release.

Also note: lint's going to fail for this PR because of these, but the other checks are all working, so safe to ignore for now.

brandur · 2023-12-03T21:23:41Z

riverdriver/river_driver_interface.go

+
+// Executor provides River operations against a database. It may be a database
+// pool or transaction.
+type Executor interface {


The executor will eventually contain all possible operations against a River schema. Notably, it looks quite similar to an adapter, and long term, will probably replace it.

brandur · 2023-12-03T21:25:26Z

riverdriver/river_driver_interface_test.go

-)
-
-// Verify interface compliance.
-var _ Driver[pgx.Tx] = &riverpgxv5.Driver{}


This check moves into each driver as they verify their own interface compliance.

brandur · 2023-12-03T21:32:32Z

@bgentry Arg, sorry this one's kind of big, but I figured it was better to build a bit more of a solid platform for the drivers rather than introduce some new backdoor to make them usable with database/sql. The tl;dr is this will give us a way to support Goose through rivermigrate and hopefully provide a driver platform that'll allow us to move operations into one by one until we converge on full non-hack support over time.

bgentry

Fantastic work on this, it's a nice step toward broader driver support 🎉 🚀

There are some complications here to figure out like LISTEN/NOTIFY
though, and I'm not clear whether database/sql could ever become a
fully functional driver as it might be missing some needed functionality
(e.g. subtransactions are still not supported after talking about them
for ten f*ing years [2]. However, even if it's not, the system would let
us support other fully functional packages or future major versions of
pgx (or even past ones like pgx/v4 if there's demand).

Yeah, the listener implementation is deeply tied to pgxv5 and might either be very difficult or impossible to implement in other drivers. My current thinking is that all the drivers will need to have a dependency on pgxv5 and provide a way to retrieve a *pgx.ConnConfig that the notifier can use to connect to the DB with pgxv5. That or we pull the notifier into the driver interface, and they'll all still be implemented with pgxv5 under the covers even if another driver is also being used for other queries.

bgentry · 2023-12-04T15:05:58Z

riverdriver/riversql/go.mod

@@ -0,0 +1,25 @@
+module github.com/riverqueue/river/riverdriver/riversql


I have some qualms with this package name. Primarily worried that it could be confusing or take away a name that we might want to use for other purposes. However the best alternative I can come up with is riverdbsql which I don't love either 😕

@bgentry

I'm -1 on riverdbsql just because it doesn't really reflect the actual naming of database/sql very well (i.e. abbreviation in one but not the other).

I just pushed a change that renames it to riverdatabasesql — it's a little bit of a mouthful (one reason I originally avoided it), but I figure that this package will be used so rarely that it probably doesn't matter. What do you think?

That works.

riverdriver/river_driver_interface.go

bgentry · 2023-12-04T15:17:00Z

riverdriver/riversql/internal/dbsqlc/sqlc.yaml

+    queries:
+      - ../../../riverpgxv5/internal/dbsqlc/river_migration.sql
+    schema:
+      - ../../../riverpgxv5/internal/dbsqlc/river_migration.sql


Is the thought that in general we should be able to share queries between drivers and let sqlc abstract away the differences? More generated code to execute or compile, but less code to actually maintain?

After reading your comment about how each driver would need to implement the full set of query methods I had some concerns about the amount of duplication we'd end up with, but if we can share the underlying schema and query definitions then that should help considerably.

Yeah, that's the idea — more compiled code, but the SQL would be entirely shared. So far I haven't seen anything to suggest that sharing the sqlc SQL sources won't be possible, so I think it should work reasonably well.

bgentry · 2023-12-04T15:20:06Z

riverdriver/riverpgxv5/river_pgx_v5_driver.go

+// mapSlice manipulates a slice and transforms it to a slice of another type.
+func mapSlice[T any, R any](collection []T, mapFunc func(T) R) []R {
+	result := make([]R, len(collection))
+
+	for i, item := range collection {
+		result[i] = mapFunc(item)
+	}
+
+	return result
+}


I assume this is included in the pgxv5 driver to avoid a circular dependency on internal/util/sliceutil?

Yeah :/ We could add another package that both top-level river and the drivers could both use, but doesn't feel worth it. We could put it in riverdriver to share, but that feels pretty weird. I suspect this is the main function that'll be useful so there won't be too much more duplication.

bgentry · 2023-12-04T15:37:31Z

docs/development.md

+git tag riverdriver/VERSION -m "release riverdriver/VERSION"
 git tag riverdriver/riverpgxv5/$VERSION -m "release riverdriver/riverpgxv5/$VERSION"
+git tag riverdriver/riversql/$VERSION -m "release riverdriver/riversql/$VERSION"


Getting closer to the point where this might warrant a little bit of effort to automate.

Yeah, major +1.

Background: While trying to figure out how to safely release [1] (which includes a new `riverdriver` submodule), I threw myself through a loop and realized that it'd be practically impossible to do without a whole bunch of manual finnagling on the release tags/lint settings in the repo, or breaking the master build, or most likely, both. This got me thinking: maybe we should, maybe we _should_ be using `replace` directives to make this less painful/brittle, which were originally removed in #35 because using `replace` makes it impossible to use `go install ...@latest`. A change that we made recently was the addition of a new Go migration API in #65 (`rivermigrate`), and a side effect of that change is that the API being used by the River CLI became entirely public (previously, it'd depended on packages in River's `internal`). By extension, that means it's now possible to make the River CLI its own Go module, something that was infeasible before because of the use of `internal`. So my thinking currently: maybe we should go back to trying to see if we can make `replace` in use by most of River's core packages, and keep the River CLI as its own module without `replace` so that it can still be installed with `go install ...@latest`. I'm not entirely sure we won't run into another problem, but it might be the easiest thing in the meantime. As the River CLI expands, we'll need to make sure to only use public APIs, but that might not be the worst thing anyway -- we could adopt the philosophy that any function the CLI accesses should also be accessible by the Go API. Here, notably we still use a `replace`, which I'm finding that I need to have a passing build for now, and which I think will have to temporarily stay until we cut a new release. Trying to build the new submodule without results in this error that I was unable to otherwise find a way around: $ go test . ambiguous import: found package github.com/riverqueue/river/cmd/river in multiple modules: github.com/riverqueue/river v0.0.12 (/Users/brandur/Documents/go/pkg/mod/github.com/riverqueue/[email protected]/cmd/river) github.com/riverqueue/river/cmd/river (/Users/brandur/Documents/projects/river/cmd/river) [1] #98

brandur · 2023-12-09T22:52:33Z

@bgentry Alright, it seems like #102 is also bad, so back to the original plan. What I'd do for release is:

Merge this. Tag riverdriver submodule and release it.
Remove replace in riverdatabasesql and riverpgxv5 and point them to release from (1) instead. Commit, tag and release both of them.
Remove replaces in top-level River package, pointing them to (2) and (3) instead. Commit, tag and release it.

I also had to bring this back in .golanci.yml so as not to break the master build as this comes in:

  gomoddirectives:
    replace-local: true

We'd remove that again as part of step (3).

This seems pretty painful for sure, but I don't think there's another way? We could automate most of this, but we'd have to unprotect the master branch because it'd require creating multiple commits in succession.

Does this sound right you?

Background: While trying to figure out how to safely release [1] (which includes a new `riverdriver` submodule), I threw myself through a loop and realized that it'd be practically impossible to do without a whole bunch of manual finnagling on the release tags/lint settings in the repo, or breaking the master build, or most likely, both. This got me thinking: maybe we should, maybe we _should_ be using `replace` directives to make this less painful/brittle, which were originally removed in #35 because using `replace` makes it impossible to use `go install ...@latest`. A change that we made recently was the addition of a new Go migration API in #65 (`rivermigrate`), and a side effect of that change is that the API being used by the River CLI became entirely public (previously, it'd depended on packages in River's `internal`). By extension, that means it's now possible to make the River CLI its own Go module, something that was infeasible before because of the use of `internal`. So my thinking currently: maybe we should go back to trying to see if we can make `replace` in use by most of River's core packages, and keep the River CLI as its own module without `replace` so that it can still be installed with `go install ...@latest`. I'm not entirely sure we won't run into another problem, but it might be the easiest thing in the meantime. As the River CLI expands, we'll need to make sure to only use public APIs, but that might not be the worst thing anyway -- we could adopt the philosophy that any function the CLI accesses should also be accessible by the Go API. Here, notably we still use a `replace`, which I'm finding that I need to have a passing build for now, and which I think will have to temporarily stay until we cut a new release. Trying to build the new submodule without results in this error that I was unable to otherwise find a way around: $ go test . ambiguous import: found package github.com/riverqueue/river/cmd/river in multiple modules: github.com/riverqueue/river v0.0.12 (/Users/brandur/Documents/go/pkg/mod/github.com/riverqueue/[email protected]/cmd/river) github.com/riverqueue/river/cmd/river (/Users/brandur/Documents/projects/river/cmd/river) [1] #98

… River Here, add a new minimal driver called `riverdriver/riversql` that supports Go's built-in `database/sql` package, but only for purposes of migrations. The idea here is to fully complete #57 by providing a way of making `rivermigrate` interoperable with Go migration frameworks that support Go-based migrations like Goose, which provides hooks for `*sql.Tx` [1] rather than pgx. `riverdriver/riversql` is not a full driver and is only meant to be used with `rivermigrate`. We document this clearly in a number of places. To make a multi-driver world possible with River, we have to start the work of building a platform that does more than `riverpgxv5`'s "cheat" workaround. This works by having each driver implement specific database operations like `MigrationGetAll`, which target their wrapped database package of choice. This is accomplished by having each driver bundle in its own sqlc that targets its package. So `riverpgxv5` has an `sqlc.yaml` that targets `pgx/v5`, while `riversql` has one that targets `database/sql`. There's some `sqlc.yaml` duplication involved here, but luckily both drivers can share a `river_migration.sql` file that contains all queries involved, so you only need to change one place. `river_migration.sql` also migrates entirely out of the main `./internal/dbsqlc`. The idea here is that eventually `./internal/dbsqlc` will disappear completely, usurped entirely by driver-specific versions. As this is done, all references to `pgx` will disappear from the top-level package. There are some complications here to figure out like `LISTEN`/`NOTIFY` though, and I'm not clear whether `database/sql` could ever become a fully functional driver as it might be missing some needed functionality (e.g. subtransactions are still not supported after talking about them for ten f*ing years [2]. However, even if it's not, the system would let us support other fully functional packages or future major versions of pgx (or even past ones like `pgx/v4` if there's demand). `river/riverdriver` becomes a package as it now has types in it that need to be referenced by driver implementations, and this would otherwise not be possible without introducing a circular dependency. [1] https://github.com/pressly/goose#go-migrations [2] golang/go#7898

Background: While trying to figure out how to safely release [1] (which includes a new `riverdriver` submodule), I threw myself through a loop and realized that it'd be practically impossible to do without a whole bunch of manual finnagling on the release tags/lint settings in the repo, or breaking the master build, or most likely, both. This got me thinking: maybe we should, maybe we _should_ be using `replace` directives to make this less painful/brittle, which were originally removed in #35 because using `replace` makes it impossible to use `go install ...@latest`. A change that we made recently was the addition of a new Go migration API in #65 (`rivermigrate`), and a side effect of that change is that the API being used by the River CLI became entirely public (previously, it'd depended on packages in River's `internal`). By extension, that means it's now possible to make the River CLI its own Go module, something that was infeasible before because of the use of `internal`. So my thinking currently: maybe we should go back to trying to see if we can make `replace` in use by most of River's core packages, and keep the River CLI as its own module without `replace` so that it can still be installed with `go install ...@latest`. I'm not entirely sure we won't run into another problem, but it might be the easiest thing in the meantime. As the River CLI expands, we'll need to make sure to only use public APIs, but that might not be the worst thing anyway -- we could adopt the philosophy that any function the CLI accesses should also be accessible by the Go API. Here, notably we still use a `replace`, which I'm finding that I need to have a passing build for now, and which I think will have to temporarily stay until we cut a new release. Trying to build the new submodule without results in this error that I was unable to otherwise find a way around: $ go test . ambiguous import: found package github.com/riverqueue/river/cmd/river in multiple modules: github.com/riverqueue/river v0.0.12 (/Users/brandur/Documents/go/pkg/mod/github.com/riverqueue/[email protected]/cmd/river) github.com/riverqueue/river/cmd/river (/Users/brandur/Documents/projects/river/cmd/river) [1] #98

brandur force-pushed the brandur-sql-db branch from 5367858 to 78ceba1 Compare December 3, 2023 21:21

brandur commented Dec 3, 2023

View reviewed changes

brandur force-pushed the brandur-sql-db branch from 78ceba1 to 1c0d3f5 Compare December 3, 2023 21:25

brandur commented Dec 3, 2023

View reviewed changes

brandur force-pushed the brandur-sql-db branch from 1c0d3f5 to 2e4c2d5 Compare December 3, 2023 21:28

brandur requested a review from bgentry December 3, 2023 21:30

brandur mentioned this pull request Dec 3, 2023

Make SQL Migrations accessible as a method #57

Closed

bgentry approved these changes Dec 4, 2023

View reviewed changes

brandur force-pushed the brandur-sql-db branch 3 times, most recently from 9b78294 to 5c299aa Compare December 7, 2023 05:32

brandur mentioned this pull request Dec 9, 2023

Put River CLI into its own submodule #102

Closed

brandur force-pushed the brandur-sql-db branch 4 times, most recently from 4cf1e83 to 7dd9d6f Compare December 9, 2023 22:45

brandur force-pushed the brandur-sql-db branch 2 times, most recently from d3cd47b to a27cf5d Compare December 13, 2023 02:04

brandur force-pushed the brandur-sql-db branch from a27cf5d to 1933931 Compare December 13, 2023 02:09

brandur mentioned this pull request Dec 13, 2023

Put River CLI into its own submodule #107

Merged

brandur force-pushed the brandur-sql-db branch from 1933931 to e868602 Compare December 13, 2023 02:17

brandur force-pushed the brandur-sql-db branch from e868602 to d9f4904 Compare December 13, 2023 02:22

brandur merged commit 9852b52 into master Dec 13, 2023
7 checks passed

brandur deleted the brandur-sql-db branch December 13, 2023 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for `database/sql` in migrations + framework for multi-driver River #98

Support for `database/sql` in migrations + framework for multi-driver River #98

brandur commented Dec 3, 2023 •

edited

Loading

brandur Dec 3, 2023

brandur Dec 3, 2023

brandur Dec 3, 2023

brandur Dec 3, 2023

brandur commented Dec 3, 2023

bgentry left a comment

bgentry Dec 4, 2023

brandur Dec 7, 2023

bgentry Dec 8, 2023

bgentry Dec 4, 2023

brandur Dec 7, 2023

bgentry Dec 4, 2023

brandur Dec 7, 2023

bgentry Dec 4, 2023

brandur Dec 7, 2023

brandur commented Dec 9, 2023


		replace github.com/riverqueue/river/riverdriver/riverpgxv5 => ./riverdriver/riverpgxv5

		replace github.com/riverqueue/river/riverdriver/riversql => ./riverdriver/riversql

		@@ -0,0 +1,25 @@
		module github.com/riverqueue/river/riverdriver/riversql

Support for database/sql in migrations + framework for multi-driver River #98

Support for database/sql in migrations + framework for multi-driver River #98

Conversation

brandur commented Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur commented Dec 3, 2023

bgentry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur commented Dec 9, 2023

Support for `database/sql` in migrations + framework for multi-driver River #98

Support for `database/sql` in migrations + framework for multi-driver River #98

brandur commented Dec 3, 2023 •

edited

Loading