internal/integrations: db_test should drop test db instances when finished

[sreuland]

PR Checklist

PR Structure

• This PR has reasonably narrow scope (if not, break it down into smaller PRs).
• This PR avoids mixing refactoring changes with feature changes (split into two PRs
  otherwise).
• This PR's title starts with name of package that is most changed in the PR, ex.
  services/friendbot, or all or doc if the changes are broad or impact many
  packages.

Thoroughness

• This PR adds tests for the most critical parts of the new functionality or fixes.
• I've updated any docs (developer docs, .md
  files, etc... affected by this change). Take a look in the docs folder for a given service,
  like this one.

Release planning

• I've updated the relevant CHANGELOG (here for Horizon) if
  needed with deprecations, added features, breaking changes, and DB schema changes.
• I've decided if this PR requires a new major/minor version according to
  semver, or if it's mainly a patch change. The PR is targeted at the next
  release branch if it's not a patch change.

What

After integration test was ran, the postgres DB instances it created were not getting dropped/deleted at end of test run.

Why

It's best to clean up test db instances, especially for local development, as the db server can start to have hundreds of abandoned test db instances that are left over.

Closes: #3268

Known limitations

This effort focused on getting the test db instances purged after integrations/test_db.go, which did address some general changes to stop db session leakage such as on cmd invocations. It doesn't cover all cases of stranded db sessions though, and other integration tests and unit tests that create test db instances are still leaving instances behind after runtime, likely requires revisiting each tests and look for similar db conn patterns, as those tend to block being able to drop the db at end of test.

[bartekn]

Session.Close() is calling Session.DB.Close() and DB can be shared with other Session instances outside ingestion package. I think we shouldn't call it here. DB should be closed when all Horizon subsystems are closed and this is done in App.CloseDB().

[sreuland]

I stepped through this in debug, using PGAdmin to view open sessions on the test db instance as horizon app shuts down from a app.Close() invocation, and what I observed:

(1) waitForDone() https://github.com/sreuland/go/blob/3268_test_db_cleanup/services/horizon/internal/app.go#L170 runs first which then calls this ingest.Shutdown()
(2) App.CloseDB() https://github.com/sreuland/go/blob/3268_test_db_cleanup/services/horizon/internal/app.go#L157 runs second after waitForDone() finishes

horizon.waitForDone() and cmd.runDBReingestRange() are the only callers of Shutdown()

When horizon App shuts down, App.CloseDB() does not result in all the active sessions on the db instance being closed, there are 2 or 3 left open, when the historyQ.Close() is invoked from within ingest.Shutdown(), it results in those remaining sessions on db to be closed, maybe it's gc/memory thing since the DB session from horizon app is cloned and placed into a new history.Q struct instance which lives inside the ingest system struct scope, the sessions seem unique to their parent object references, which kind of contrasts the 'shared' aspect since, it appears as though each session instance has exclusive unique db sessions, and they won't be affected by the other, only when both of the sessions are closed, then the integration test cleanup routine that tries to drop the test db will work.

[sreuland]

@bartekn , did the session closing behavior i mentioned sound sensible? when ingest initiates shutdown it's from horizon waitForDone() which means overall app is going down anyway or from a db reingest command via runDBREingestRange(), the process is ending in both cases when ingest shutdown initiates the close on session which leads to DB. Without invoking the close on this ingest session instance, the underlying cloned session tends to lead to sessions left open on the db server, I haven't been able to remove them otherwise.

[sreuland]

@2opremio , wanted to get your opinion in this fix too, do you see value in taking this approach to delete test db instances, or is it just introducing unnecessary risk, etc. I was thinking to close this PR rather than lean on it anymore or adjust it if something more palpable is suggested. thanks for any insights!

[bartekn]

Sorry for a huge delay answering this. I missed the notification somehow. I prepared the code sample below that shows that closing a cloned session actually closes the underlying DB connection for both sessions:

package main

import (
	"context"

	"github.com/stellar/go/support/db"
)

func main() {
	session, err := db.Open("postgres", "postgres://bartek@localhost:5432/test?sslmode=disable")
	if err != nil {
		panic(err)
	}

	c1 := session.Clone()
	c2 := session.Clone()

	_, err = c1.ExecRaw(context.Background(), "SELECT 1;")
	if err != nil {
		panic(err)
	}

	err = c1.Close()
	if err != nil {
		panic(err)
	}

	_, err = c2.ExecRaw(context.Background(), "SELECT 1;")
	if err != nil {
		panic(err)
	}
}

Result:

panic: exec failed: sql: database is closed

goroutine 1 [running]:
main.main()
	/Users/bartek/go/src/github.com/stellar/go/shawn_test/main.go:30 +0x132
exit status 2

The reason why the code works in this PR is that in waitForDone the ingestion system is shut down after webserver (a.ingester.Shutdown()). But if we ever move it before a webserver or add another service that will send DB connections in the background and will be used after a.ingester.Shutdown() this will generate errors and bugs (like background worker will not be able to commit it's job).

[sreuland]

Thanks for verifying the go db/session close behavior, very helpful to rule that out!
yes, we don't want implicit coupling like that either, ok, will look into more. w/o this DB close in ingest shutdown, we strangely get the opposite effect, which is when App.CloseDB() is executed, the sessions related to ingestion don't get closed on the db server side as observed in PGAdmin, that is, until the go o/s process is terminated at which point they are gone.

[2opremio]

horizon.waitForDone() and cmd.runDBReingestRange() are the only callers of Shutdown()

All tests call it implitcitly on shutdown due to this call in integration.go:

	i.t.Cleanup(i.Shutdown)

[2opremio]

I think that the problem this PR is trying to solve is causing the Error starting ingestion read transaction: beginTx failed: pq: sorry, too many clients already at integration tests (such as https://github.com/stellar/go/runs/6233228705 and https://github.com/stellar/go/runs/6267795798)