Full schema insertion + schema testing (#848)

* Full schema insertion + schema testing

Here, introduce a full version of schema injection as initially started
in #798, and exposing a new `Schema` option to make it configurable even
outside the test suite.

This involved fixing a lot of bugs from #798, and the only way to make
it possible to root them all out was to make full use of schemas across
the entire test suite. The original "test DB manager" system has been
replaced with a new `riverschematest.TestSchema` helper that generates a
schema for use with a test case or prefers an existing idle one that was
already generated for the same test run.

`TestSchema` runs migrations for generated schemas which also means we
don't need to use `testdbman` anymore, with tests capable of
bootstrapping themselves at run time. We reuse schemas to avoid this
extra work when possible, but migrating a new schema is also
surprisingly fast, taking up to 50ms, but getting more down to a stable
~10ms once things are warmed up. Here's a series of sample timings:

    $ go test ./rivermigrate -run TestMigrator/MigrateUpDefault -test.v -count 50 | grep 'migrations ran in'
        river_migrate_test.go:492: migrations ran in 45.571209ms
        river_migrate_test.go:492: migrations ran in 24.642458ms
        river_migrate_test.go:492: migrations ran in 16.749708ms
        river_migrate_test.go:492: migrations ran in 22.970375ms
        river_migrate_test.go:492: migrations ran in 16.201375ms
        river_migrate_test.go:492: migrations ran in 15.727625ms
        river_migrate_test.go:492: migrations ran in 13.291333ms
        river_migrate_test.go:492: migrations ran in 13.680708ms
        river_migrate_test.go:492: migrations ran in 14.867416ms
        river_migrate_test.go:492: migrations ran in 15.631916ms
        river_migrate_test.go:492: migrations ran in 13.873791ms
        river_migrate_test.go:492: migrations ran in 14.8645ms
        river_migrate_test.go:492: migrations ran in 14.92575ms
        river_migrate_test.go:492: migrations ran in 12.541834ms
        river_migrate_test.go:492: migrations ran in 14.753875ms
        river_migrate_test.go:492: migrations ran in 12.694334ms
        river_migrate_test.go:492: migrations ran in 13.955917ms
        river_migrate_test.go:492: migrations ran in 12.126458ms
        river_migrate_test.go:492: migrations ran in 14.095958ms
        river_migrate_test.go:492: migrations ran in 13.273375ms
        river_migrate_test.go:492: migrations ran in 13.988917ms
        river_migrate_test.go:492: migrations ran in 13.141459ms
        river_migrate_test.go:492: migrations ran in 12.394417ms
        river_migrate_test.go:492: migrations ran in 11.539208ms
        river_migrate_test.go:492: migrations ran in 11.577834ms
        river_migrate_test.go:492: migrations ran in 10.883375ms
        river_migrate_test.go:492: migrations ran in 10.547417ms
        river_migrate_test.go:492: migrations ran in 12.330375ms
        river_migrate_test.go:492: migrations ran in 11.54575ms
        river_migrate_test.go:492: migrations ran in 11.437458ms
        river_migrate_test.go:492: migrations ran in 10.957ms
        river_migrate_test.go:492: migrations ran in 10.589083ms
        river_migrate_test.go:492: migrations ran in 9.758583ms

Removal of the "test DB manager" system also means that we can ungate
test from `-p 1` because they're all able to run in parallel now. The
limiting factor I ran in is that we need to keep max pool connections
within each package's tests to a relatively modest number (I found 15
seemed to maximum success) so parallel packages don't exceed the default
Postgres configuration of 100 connections.

Something that can be kind of annoying is that in case a schema isn't
used properly somewhere in a test case (i.e. `TestSchema` is run, but
then not used), inserts/operations will go the default schema, which
will leave debris there, and that will interfere with test cases using
`TestTx` (with test DB manager, all debris would go to a different
database so you wouldn't notice). To remediate this, I've added a
cleanup hook to `TestSchema` that looks for leftovers that may have been
added to the default schema. This isn't perfect because those leftovers
may have come from another test case running in parallel or which ran
previously, but it helps to zero in on the original source of the issue.

* Use schema testing for `TestTx`, sharing one schema per set of migration lines

Here, start using a new isolated schema for test transaction invocations
rather than falling back on whatever happens to be in `river_test`. A
major benefit of this approach is that it makes migration inconsistencies
impossible so that when checking out a branch with a new migration, your
tests will always get the new migration, but then drop it when moving
back to the `master` branch.

It has the additional advantage that when alternatively running River
and River Pro tests locally pointing to the same `river_test` database,
we get the right migration lines raised for `TestTx` without having to
worry about whether Pro migrations have been made available yet or not.

We attempt to keep test transactions very efficient by only raising one
schema per package being tested (and per set of migration lines,
although this is rarely used) so we don't need to create a schema and
run migrations once per test, but rather once for _all_ tests in the
package.

Similar to `TestSchema`, `TestTx` picks up a driver so that it can use
`GetMigrationDefaultLines` to determine what migration lines a test case
is expecting to exist before running. Once again, this is mainly useful
for River Pro.

`TestTx` gets moved to the higher level `riverschematest` package
because it needs access to high level constructs like migrations, and I
rename `riverschematest` to `riverdbtest` because with test transactions
in it, its use becomes more general. I would've also moved functions
in `riversharedtest` like `DBPool` and `TestDatabaseURL` over, but
unfortunately we still need these at a lower level so that the tests in
`rivermigrate` can use them.

I removed the `riverdbtest` functionality that checks for extraneous
rows. The `public` schema no longer even exists in CI, and the check is
fairly unnecessary now that zeroing `search_path` makes it very
difficult to accidentally insert records into anything but the current
test schema.

* Inject schema to completer instead of each job being completed

Per code review feedback, instead of sending schema into the executor
and then through into each completion being made, inject it once into
the completer, then have the completer add the schema as it's completing
batches. In practice, this ends up removing a lot of schema noise, and
the job executor no longer needs to know about schema at all.

Author: brandur

.github/workflows/ci.yaml

@@ -8,7 +8,7 @@ env:
   DATABASE_URL: postgres://postgres:postgres@127.0.0.1:5432/river_dev?sslmode=disable
 
   # Test database.
-  TEST_DATABASE_URL: postgres://postgres:postgres@127.0.0.1:5432/river_test?sslmode=disable
+  TEST_DATABASE_URL: postgres://postgres:postgres@127.0.0.1:5432/river_test?pool_max_conns=15&sslmode=disable
 
 on:
   push:
@@ -42,6 +42,13 @@ jobs:
       postgres:
         image: postgres:${{ matrix.postgres-version }}
         env:
+          # Left as a reminder that it might not be a bad idea to increase max
+          # connections and then increase the maximum allowed in the databaes
+          # pools for each package under tests. This config is only supported on
+          # Postgres 16+ though, and changing Postgres configuration on any
+          # version before that is absurdly difficult through Docker, so it
+          # might be worth just waiting until <16 have rolled off.
+          # POSTGRES_INITDB_ARGS: "-c max_connections=1500"
           POSTGRES_PASSWORD: postgres
         options: >-
           --health-cmd pg_isready
@@ -62,42 +69,11 @@ jobs:
       - name: Display Go version
         run: go version
 
-      - name: Set up test DBs
-        run: go run ./internal/cmd/testdbman create
-        env:
-          PGHOST: 127.0.0.1
-          PGPORT: 5432
-          PGUSER: postgres
-          PGPASSWORD: postgres
-          PGSSLMODE: disable
+      - name: Set up database
+        run: psql -c "CREATE DATABASE river_test" $ADMIN_DATABASE_URL
 
       - name: Test
-        working-directory: .
-        run: go test -p 1 -race ./... -timeout 2m
-
-      - name: Test cmd/river
-        working-directory: ./cmd/river
-        run: go test -race ./... -timeout 2m
-
-      - name: Test riverdriver
-        working-directory: ./riverdriver
-        run: go test -race ./... -timeout 2m
-
-      - name: Test riverdriver/riverdatabasesql
-        working-directory: ./riverdriver/riverdatabasesql
-        run: go test -race ./... -timeout 2m
-
-      - name: Test riverdriver/riverpgxv5
-        working-directory: ./riverdriver/riverpgxv5
-        run: go test -race ./... -timeout 2m
-
-      - name: Test rivershared
-        working-directory: ./rivershared
-        run: go test -race ./... -timeout 2m
-
-      - name: Test rivertype
-        working-directory: ./rivertype
-        run: go test -race ./... -timeout 2m
+        run: make test/race
 
   cli:
     strategy:
@@ -157,21 +133,16 @@ jobs:
 
       - run: river migrate-get --all --exclude-version 1 --up
 
-      - name: river migrate-up
-        run: river migrate-up --database-url $DATABASE_URL
-        shell: bash
+      - run: river migrate-up --database-url $DATABASE_URL
+        shell: bash # needed for windows to interpret env var
 
-      - name: river migrate-list
-        run: river migrate-list --database-url $DATABASE_URL
+      - run: river migrate-list --database-url $DATABASE_URL
         shell: bash
 
-      - name: river validate
-        run: river validate --database-url $DATABASE_URL
+      - run: river validate --database-url $DATABASE_URL
         shell: bash
 
-      - name: river version
-        run: river version
-        shell: bash
+      - run: river version
 
       - name: river bench
         run: |
@@ -196,8 +167,7 @@ jobs:
           fi
         shell: bash
 
-      - name: river migrate-down
-        run: river migrate-down --database-url $DATABASE_URL --max-steps 100
+      - run: river migrate-down --database-url $DATABASE_URL --max-steps 100
         shell: bash
 
       - name: river validate (expect failure)
@@ -214,6 +184,32 @@ jobs:
           fi
         shell: bash
 
+      #
+      # Run the whole migration loop again, this time with a custom schema.
+      #
+
+      - run: echo "CUSTOM_SCHEMA=custom_schema" >> $GITHUB_ENV
+        shell: bash
+
+      - run: psql -c "CREATE SCHEMA $CUSTOM_SCHEMA" $DATABASE_URL
+        shell: bash
+
+      - run: river migrate-up --database-url $DATABASE_URL --schema $CUSTOM_SCHEMA
+        shell: bash
+
+      - run: river validate --database-url $DATABASE_URL --schema $CUSTOM_SCHEMA
+        shell: bash
+
+      - run: river migrate-down --database-url $DATABASE_URL --max-steps 100 --schema $CUSTOM_SCHEMA
+        shell: bash
+
+      - name: river validate (expect failure)
+        run: |
+          if river validate --database-url $DATABASE_URL --schema $CUSTOM_SCHEMA; then
+            echo "expected non-zero exit code" && exit 1
+          fi
+        shell: bash
+
   golangci:
     name: lint
     runs-on: ubuntu-latest

.golangci.yaml

@@ -88,6 +88,7 @@ linters:
         - r
         - sb # common convention for string builder
         - t
+        - tb
         - tt # common convention for table tests
         - tx
         - w

CHANGELOG.md

@@ -7,10 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+⚠️ Internal APIs used for communication between River and River Pro have changed. If using River Pro, make sure to update River and River Pro to latest at the same time to get compatible versions.
+
 ### Added
 
 - Added `river/riverlog` containing middleware that injects a context logger to workers that collates log output and persists it with job metadata. This is paired with a River UI enhancement that shows logs in the UI. [PR #844](https://github.com/riverqueue/river/pull/844).
 - Added `JobInsertMiddlewareFunc` and `WorkerMiddlewareFunc` to easily implement middleware with a function instead of a struct. [PR #844](https://github.com/riverqueue/river/pull/844).
+- Added `Config.Schema` which lets a non-default schema be injected explicitly into a River client that'll be used for all database operations. This may be particularly useful for proxies like PgBouncer that may not respect a schema configured in `search_path`. [PR #848](https://github.com/riverqueue/river/pull/848).
 
 ### Changed
 

Makefile

@@ -53,14 +53,14 @@ $(foreach mod,$(submodules),$(eval $(call lint-target,$(mod))))
 .PHONY: test
 test:: ## Run test suite for all submodules
 define test-target
-    test:: ; cd $1 && go test ./... -p 1
+    test:: ; cd $1 && go test ./... -timeout 2m
 endef
 $(foreach mod,$(submodules),$(eval $(call test-target,$(mod))))
 
 .PHONY: test/race
 test/race:: ## Run test suite for all submodules with race detector
 define test-race-target
-    test/race:: ; cd $1 && go test ./... -p 1 -race
+    test/race:: ; cd $1 && go test ./... -race -timeout 2m
 endef
 $(foreach mod,$(submodules),$(eval $(call test-race-target,$(mod))))