bug: only use one db connection at any point in time

I've fixed the implementation of `create()` to only use a single database connection at any point in time — previously, as @ebabani pointed out in #5, pgtestdb used 2 simultaneous connections. I was able to verify the behavior by adding some `time.Sleep` calls in the library and then connecting to the test database while running some tests: <details> <summary> Now, when you call `pgtestdb.New()` or `pgtestdb.Custom(0`, pgtestdb connects, ensures the template exists, creates the instance db, then cleans everything up like before, but only uses a single connection to the server at any point in time.</summary> * Here, you see a single active connection to the `postgres` database to start (that's me connected in `psql`.) ```psql postgres=# SELECT datname, sum(numbackends) FROM pg_stat_database WHERE datname is not null group by 1; datname | sum ---------------------------------------------+----- postgres | 1 template0 | 0 template1 | 0 ``` * I start running a test. `pgtestdb` connects to the `postgres` database as well, this is the `baseDB` in the code. It uses this connection to create the template database, run migrations, and then create the instance database for the specific test. ```psql postgres=# SELECT datname, sum(numbackends) FROM pg_stat_database WHERE datname is not null group by 1; datname | sum ---------------------------------------------+----- postgres | 2 template0 | 0 template1 | 0 ``` * `pgtestdb` connects to the instance database, then hands it to the test. this is the `db := pgtestdb.New(...)` connection. ```psql postgres=# SELECT datname, sum(numbackends) FROM pg_stat_database WHERE datname is not null group by 1; datname | sum -----------------------------------------------------------------+----- template1 | 0 testdb_tpl_ed8ae75db1176559951eadb85d6be0db_inst_8ed74b29d41d8c | 1 postgres | 1 testdb_tpl_ed8ae75db1176559951eadb85d6be0db | 0 template0 | 0 ``` * The test case finishes, and `pgtestdb` closes the connection to the instance, connects back to the `baseDB`, and deletes the instance. ```psql postgres=# SELECT datname, sum(numbackends) FROM pg_stat_database WHERE datname is not null group by 1; datname | sum ---------------------------------------------+----- postgres | 2 testdb_tpl_ed8ae75db1176559951eadb85d6be0db | 0 template0 | 0 template1 | 0 ``` * `pgtestdb` closes its connection, the test is entirely finished running. ```psql postgres=# SELECT datname, sum(numbackends) FROM pg_stat_database WHERE datname is not null group by 1; datname | sum ---------------------------------------------+----- postgres | 1 testdb_tpl_ed8ae75db1176559951eadb85d6be0db | 0 template0 | 0 template1 | 0 ``` </details> In my testing, re-connecting to the database adds some slight overhead per test, but it's fundamentally more correct, and the database connection time should be dominated by the time taken during each test so overall this is worth the performance impact because it allows you to double the amount of tests you are running at once! I also updated the FAQ to explain the problem, show some example error messages, and provide some ideas for how to work around it. Co-authored-by: Peter Downs <github@peterdowns.com>
peterldowns · Apr 4, 2024 · 15a8a8a · 15a8a8a
1 parent 7374056
commit 15a8a8a
Show file tree

Hide file tree

Showing 9 changed files with 80 additions and 30 deletions.
diff --git a/README.md b/README.md
@@ -1,6 +1,6 @@
 # 🧪 pgtestdb
 
-![Latest Version](https://badgers.space/badge/latest%20version/v0.0.13/blueviolet?corner_radius=m)
+![Latest Version](https://badgers.space/badge/latest%20version/v0.0.14/blueviolet?corner_radius=m)
 ![Golang](https://badgers.space/badge/golang/1.18+/blue?corner_radius=m)
 
 pgtestdb makes it cheap and easy to create ephemeral Postgres databases for your
@@ -535,6 +535,43 @@ For ramdisk/CI in particular, you may get some ideas from [this blog post](https
 -c 'client_min_messages=warning'
 ```
 
+## Why are my tests failing because they can't connect to Postgres?
+
+First, make sure the server is running and you can connect to it. But assuming
+you're seeing some kind of failure while running a larger test suite, the most likely
+problem is that you're exceeding the maximum number of connections your Postgres server is
+configured to accept. This may show up as a few different types of error messages:
+
+```
+--- FAIL: TestParallel2/subtest_34 (0.01s)
+    testdb_test.go:134: failed to create instance: failed to create instance from template testdb_tpl_ed8ae75db1176559951eadb85d6be0db: failed to connect to `host=localhost user=postgres database=`: server error (FATAL: sorry, too many clients already (SQLSTATE 53300))
+```
+
+or
+
+```
+--- FAIL: TestParallel2/subtest_25 (0.04s)
+    testdb_test.go:134: could not create pgtestdb user: sessionlock(testdb-user) failed to open conn: failed to connect to `host=localhost user=postgres database=`: dial error (dial tcp 127.0.0.1:5433: connect: connection refused
+  ```
+
+or
+
+```
+--- FAIL: TestNew (0.07s)
+    testdb_test.go:42: failed to migrator.Prepare template testdb_tpl_ed8ae75db1176559951eadb85d6be0db: sessionlock(test-sql-migrator) failed to open conn: failed to connect to `host=localhost user=pgtdbuser database=testdb_tpl_ed8ae75db1176559951eadb85d6be0db`: server error (FATAL: remaining connection slots are reserved for non-replication superuser connections (SQLSTATE 53300))
+```
+
+The fundamental way to fix this error is to make sure that you do not attempt
+more simultaneous connections to the server than it is ready to accept. Here are
+the best ways to do that, from easiest to most complicated:
+
+* Just don't run so many tests at once. If you're developing locally, and have a fast CI, you may not need to run the full test suite at once.
+* Set a higher value for your server's [`max_connections` parameter](https://www.postgresql.org/docs/current/runtime-config-connection.html), which defaults to 100. If you look at the [`docker-compose.yaml`](./docker-compose.yml) in this repo you'll see we set it to 1000. Assuming you're using a dockerized and tmpfs-backed test server on a beefy machine, you should be able to safely crank this really high.
+* Run fewer tests at the same time. You'll want to read the [go docs for the `-parallel` test flag and the `-p` build flag](https://pkg.go.dev/cmd/go) very carefully. You can also find these docs by running `go help build` and `go help tesflags`. Basically, `-p N` means go will run tests for up to `N` packages at the same time, and `-parallel Y` means it will run up to `Y` tests in parallel within each package. Both `N` and `Y` default to `GOMAXPROCS`, which is the number of logical CPUs in your machine. So by default, `go test ./...` can run up to `GOMAXPROCS * GOMAXPROCS` tests at the same time. You can try tuning `parallel Y` and `-p N` to set a hard limit of `N * Y` simultaneous tests, but that doesn't mean that you're using at most `N * Y` database connections --- depending on your test and application logic, you may actually use multiple connections at the same time. Sorry, it's confusing.
+* Consider putting a connection pooler in front of your test database. This is maybe easiest to do in CI but it's possible locally with a containerized instance of pgbouncer, for example. Your tests will still contend for your database's resources, but it may turn "tests failing" into "tests getting slower".
+
+It's worth noting that when you call `pgtestdb.New()` or `pgtestdb.Custom()`, the library will only use one active connection at any point in time. So if you have 100 tests running in parallel, pgtestdb will at most consume 100 simultaneous connections. Your own code in your tests may run multiple queries in parallel and consume more connections at once, though.
+
 ## How do I connect to the test databases through `pgx` / `pgxpool` instead of using the sql.DB interface?
 
 You can use `pgtestdb.Custom` to connect via `pgx`, `pgxpool`, or any other

diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-v0.0.13
+v0.0.14
diff --git a/migrators/atlasmigrator/go.mod b/migrators/atlasmigrator/go.mod
@@ -6,7 +6,7 @@ replace github.com/peterldowns/pgtestdb => ../../
 
 require (
 	github.com/jackc/pgx/v5 v5.5.1
-	github.com/peterldowns/pgtestdb v0.0.13
+	github.com/peterldowns/pgtestdb v0.0.14
 	github.com/peterldowns/testy v0.0.1
 )
 

diff --git a/migrators/dbmatemigrator/go.mod b/migrators/dbmatemigrator/go.mod
@@ -7,7 +7,7 @@ replace github.com/peterldowns/pgtestdb => ../../
 require (
 	github.com/amacneil/dbmate/v2 v2.4.0
 	github.com/jackc/pgx/v5 v5.5.1
-	github.com/peterldowns/pgtestdb v0.0.13
+	github.com/peterldowns/pgtestdb v0.0.14
 	github.com/peterldowns/testy v0.0.1
 )
 

diff --git a/migrators/golangmigrator/go.mod b/migrators/golangmigrator/go.mod
@@ -7,7 +7,7 @@ replace github.com/peterldowns/pgtestdb => ../../
 require (
 	github.com/golang-migrate/migrate/v4 v4.16.0
 	github.com/jackc/pgx/v5 v5.5.1
-	github.com/peterldowns/pgtestdb v0.0.13
+	github.com/peterldowns/pgtestdb v0.0.14
 	github.com/peterldowns/testy v0.0.1
 )
 

diff --git a/migrators/goosemigrator/go.mod b/migrators/goosemigrator/go.mod
@@ -6,7 +6,7 @@ replace github.com/peterldowns/pgtestdb => ../../
 
 require (
 	github.com/jackc/pgx/v5 v5.5.1
-	github.com/peterldowns/pgtestdb v0.0.13
+	github.com/peterldowns/pgtestdb v0.0.14
 	github.com/peterldowns/testy v0.0.1
 	github.com/pressly/goose/v3 v3.11.2
 )

diff --git a/migrators/pgmigrator/go.mod b/migrators/pgmigrator/go.mod
@@ -7,7 +7,7 @@ replace github.com/peterldowns/pgtestdb => ../../
 require (
 	github.com/jackc/pgx/v5 v5.5.1
 	github.com/peterldowns/pgmigrate v0.0.5
-	github.com/peterldowns/pgtestdb v0.0.13
+	github.com/peterldowns/pgtestdb v0.0.14
 	github.com/peterldowns/testy v0.0.1
 )
 

diff --git a/migrators/sqlmigrator/go.mod b/migrators/sqlmigrator/go.mod
@@ -6,7 +6,7 @@ replace github.com/peterldowns/pgtestdb => ../../
 
 require (
 	github.com/jackc/pgx/v5 v5.5.1
-	github.com/peterldowns/pgtestdb v0.0.13
+	github.com/peterldowns/pgtestdb v0.0.14
 	github.com/peterldowns/testy v0.0.1
 	github.com/rubenv/sql-migrate v1.4.0
 )

diff --git a/testdb.go b/testdb.go
@@ -122,6 +122,18 @@ type Migrator interface {
 	Verify(context.Context, *sql.DB, Config) error
 }
 
+// TB is a subset of the `testing.TB` testing interface implemented by
+// `*testing.T`, `*testing.B`, and `*testing.F`, so you can use pgtestdb to get
+// a database for tests, benchmarks, and fuzzes. It contains only the methods
+// actually needed by pgtestdb, defined so that we can more easily mock it.
+type TB interface {
+	Cleanup(func())
+	Failed() bool
+	Fatalf(format string, args ...any)
+	Helper()
+	Logf(format string, args ...any)
+}
+
 // New connects to a postgres server and creates and connects to a fresh
 // database instance. This database is prepared and migrated by the given
 // migrator, by get-or-creating a template database and then cloning it. This is
@@ -144,18 +156,6 @@ func New(t TB, conf Config, migrator Migrator) *sql.DB {
 	return db
 }
 
-// TB is a subset of the `testing.TB` testing interface implemented by
-// `*testing.T`, `*testing.B`, and `*testing.F`, so you can use pgtestdb to get
-// a database for tests, benchmarks, and fuzzes. It contains only the methods
-// actually needed by pgtestdb, defined so that we can more easily mock it.
-type TB interface {
-	Cleanup(func())
-	Failed() bool
-	Fatalf(format string, args ...any)
-	Helper()
-	Logf(format string, args ...any)
-}
-
 // Custom is like [New] but after creating the new database instance, it closes
 // any connections and returns the configuration details of that database so
 // that you can connect to it explicitly, potentially via a different SQL
@@ -168,18 +168,16 @@ func Custom(t TB, conf Config, migrator Migrator) *Config {
 	// choosing without interference from this existing connection.
 	if err := db.Close(); err != nil {
 		t.Fatalf("could not close test database: '%s': %s", config.Database, err)
-		return nil // uncreachable
+		return nil // unreachable
 	}
 	return config
 }
 
-// Helper
-// Fatalf
-// Fatal
-// Logf
-// Cleanup
-// Failed
-
+// create contains the implementation of [New] and [Custom], and is responsible
+// for actually creating the instance database to be used by a testcase.
+//
+// create will use at most one connection to the underlying database at any
+// given time.
 func create(t TB, conf Config, migrator Migrator) (*Config, *sql.DB) {
 	t.Helper()
 	ctx := context.Background()
@@ -219,22 +217,37 @@ func create(t TB, conf Config, migrator Migrator) (*Config, *sql.DB) {
 		return nil, nil // unreachable
 	}
 
+	if err := baseDB.Close(); err != nil {
+		t.Fatalf("could not close base database: '%s': %s", conf.Database, err)
+		return nil, nil // unreachable
+	}
+
 	t.Cleanup(func() {
 		// Close the testDB
 		if err := db.Close(); err != nil {
 			t.Fatalf("could not close test database: '%s': %s", instance.Database, err)
-			return // uncreachable
+			return // unreachable
 		}
 		// If the test failed, leave the instance around for further investigation
 		if t.Failed() {
 			return
 		}
-		// Otherwise, remove the instance from the server
+
+		// Otherwise, reconnect to the basedb and remove the instance from the server
+		baseDB, err := conf.Connect()
+		if err != nil {
+			t.Fatalf("could not connect to database: '%s': %s", conf.Database, err)
+			return
+		}
 		query := fmt.Sprintf(`DROP DATABASE IF EXISTS "%s"`, instance.Database)
 		if _, err := baseDB.ExecContext(ctx, query); err != nil {
 			t.Fatalf("could not drop test database '%s': %s", instance.Database, err)
 			return // unreachable
 		}
+		if err := baseDB.Close(); err != nil {
+			t.Fatalf("could not close base database: '%s': %s", conf.Database, err)
+			return // unreachable
+		}
 	})
 
 	// Even if the template previously existed, verify that it was created