testdb: Remove global advisory lock when creating Praefect database

Setting up the Praefect database for our tests works by populating a
shared template database with our migrations. This template database is
then used as the source of the actual database we're using for the test
and is shared across our tests. It is thus essentially a global shared
resource not only across tests of a single testrun, but even across
multiple runs of the testsuite. This brings multiple problems with it:

    - We need to acquire a global advisory lock so that no two tests try
      to update the template database at the same point in time. This
      sequentializes tests which want to create a test database.

    - There is a race between creating the template database and the
      real database as we release the lock before we create the real
      database. Consequentially, any concurrently running test with a
      different set of migrations could modify the state of the template
      database.

    - We need to have a bunch of logic to detect when the template
      database is not in a state that would be recognized by us.
      Overall, this solution is kind of complex.

Now the question is why we have decided to do it like this in the first
place. The logic has been introduced via c61cdadb5 (Replace in-memory
queue with Postgres implementation, 2021-07-14), which does not mention
why we use a shared template database instead of just creating the final
target database directly. There are two reasons I can think of though:

    1. It stresses the migration logic a bit more. This is in my opinion
       not a good argument though as the migration logic should ideally
       be tested as a standalone unit anyway.

    2. We want to avoid having to re-run database migrations on every
       test in order to be more efficient.

I doubt that (1) had been the reason, and assume it was (2). Benchmarks
show though that this optimization does not quite work in our favor:

    Benchmark 1: with advisory lock
      Time (mean ± σ):     12.042 s ±  0.487 s    [User: 42.657 s, System: 8.606 s]
      Range (min … max):   11.551 s … 12.643 s    5 runs

    Benchmark 2: without advisory lock
      Time (mean ± σ):     10.928 s ±  0.080 s    [User: 43.569 s, System: 9.402 s]
      Range (min … max):   10.816 s … 11.007 s    5 runs

    Summary
      'without advisory lock' ran
        1.10 ± 0.05 times faster than 'with advisory lock'

As you can see, the tests run about 10% _faster_ without the advisory
lock. This is because the tests can now be parallelized better as they
do not depend on a global lock anymore. That being said, both user and
system time have increased as we now spend more time initializing the
database. But that increase is seemingly getting absorbed by improved
parallelization.

Remove the global advisory lock. This simplifies the code, reduces the
amount of global state we have and speeds up our tests.

1 files changed, 16 insertions, 88 deletions

diff --git a/internal/testhelper/testdb/db.go b/internal/testhelper/testdb/db.go
index 222eb7303..c0c6b57a8 100644
--- a/internal/testhelper/testdb/db.go
+++ b/internal/testhelper/testdb/db.go
@@ -23,11 +23,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
-const (
-	advisoryLockIDDatabaseTemplate = 1627644550
-	praefectTemplateDatabase       = "praefect_template"
-)
-
 // TxWrapper is a simple wrapper around *sql.Tx.
 type TxWrapper struct {
 	*sql.Tx
@@ -209,73 +204,18 @@ func requireTerminateAllConnections(tb testing.TB, db *sql.DB, database string)
 func initPraefectDB(tb testing.TB, database string) *sql.DB {
 	tb.Helper()
 
-	dbCfg := GetConfig(tb, "postgres")
-	// We require a direct connection to the Postgres instance and not through the PgBouncer
-	// because we use transaction pool mood for it and it doesn't work well for system advisory locks.
-	postgresDB := requireSQLOpen(tb, dbCfg, true)
-	defer func() {
-		require.NoErrorf(tb, postgresDB.Close(), "release connection to the %q database", dbCfg.DBName)
-	}()
-
-	// Acquire exclusive advisory lock to prevent other concurrent test from doing the same.
-	_, err := postgresDB.Exec(`SELECT pg_advisory_lock($1)`, advisoryLockIDDatabaseTemplate)
-	require.NoError(tb, err, "not able to acquire lock for synchronisation")
-	var advisoryUnlock func()
-	advisoryUnlock = func() {
-		require.True(tb, scanSingleBool(tb, postgresDB, `SELECT pg_advisory_unlock($1)`, advisoryLockIDDatabaseTemplate), "release advisory lock")
-		advisoryUnlock = func() {}
-	}
-	defer func() { advisoryUnlock() }()
-
-	templateDBExists := databaseExist(tb, postgresDB, praefectTemplateDatabase)
-	if !templateDBExists {
-		_, err := postgresDB.Exec("CREATE DATABASE " + praefectTemplateDatabase + " WITH ENCODING 'UTF8'")
-		require.NoErrorf(tb, err, "failed to create %q database", praefectTemplateDatabase)
-	}
-
-	templateDBConf := GetConfig(tb, praefectTemplateDatabase)
-	templateDB := requireSQLOpen(tb, templateDBConf, true)
-	defer func() {
-		require.NoErrorf(tb, templateDB.Close(), "release connection to the %q database", templateDBConf.DBName)
-	}()
-
-	if _, err := glsql.Migrate(templateDB, false); err != nil {
-		// If database has unknown migration we try to re-create template database with
-		// current migration. It may be caused by other code changes done in another branch.
-		if pErr := (*migrate.PlanError)(nil); errors.As(err, &pErr) {
-			if strings.EqualFold(pErr.ErrorMessage, "unknown migration in database") {
-				require.NoErrorf(tb, templateDB.Close(), "release connection to the %q database", templateDBConf.DBName)
-
-				_, err = postgresDB.Exec("DROP DATABASE " + praefectTemplateDatabase)
-				require.NoErrorf(tb, err, "failed to drop %q database", praefectTemplateDatabase)
-				_, err = postgresDB.Exec("CREATE DATABASE " + praefectTemplateDatabase + " WITH ENCODING 'UTF8'")
-				require.NoErrorf(tb, err, "failed to create %q database", praefectTemplateDatabase)
-
-				remigrateTemplateDB := requireSQLOpen(tb, templateDBConf, true)
-				defer func() {
-					require.NoErrorf(tb, remigrateTemplateDB.Close(), "release connection to the %q database", templateDBConf.DBName)
-				}()
-				_, err = glsql.Migrate(remigrateTemplateDB, false)
-				require.NoErrorf(tb, err, "failed to run database migration on %q", praefectTemplateDatabase)
-			} else {
-				require.NoErrorf(tb, err, "failed to run database migration on %q", praefectTemplateDatabase)
-			}
-		} else {
-			require.NoErrorf(tb, err, "failed to run database migration on %q", praefectTemplateDatabase)
-		}
-	}
-
-	// Release advisory lock as soon as possible to unblock other tests from execution.
-	advisoryUnlock()
+	postgresCfg := GetConfig(tb, "postgres")
 
-	require.NoErrorf(tb, templateDB.Close(), "release connection to the %q database", templateDBConf.DBName)
-
-	_, err = postgresDB.Exec(`CREATE DATABASE ` + database + ` TEMPLATE ` + praefectTemplateDatabase)
-	require.NoErrorf(tb, err, "failed to create %q database", praefectTemplateDatabase)
+	postgresDB := requireSQLOpen(tb, postgresCfg, true)
+	tb.Cleanup(func() {
+		testhelper.MustClose(tb, postgresDB)
+	})
 
+	_, err := postgresDB.Exec("CREATE DATABASE " + database + " WITH ENCODING 'UTF8'")
+	require.NoErrorf(tb, err, "failed to create %q database", database)
 	tb.Cleanup(func() {
 		if _, ok := getDatabaseEnvironment(tb)["PGHOST_PGBOUNCER"]; ok {
-			pgbouncerCfg := dbCfg
+			pgbouncerCfg := postgresCfg
 			// This database name will connect us to the special admin console.
 			pgbouncerCfg.DBName = "pgbouncer"
 
@@ -294,39 +234,27 @@ func initPraefectDB(tb testing.TB, database string) *sql.DB {
 			require.NoError(tb, err, "killing PgBouncer connections")
 		}
 
-		dbCfg.DBName = "postgres"
-		postgresDB := requireSQLOpen(tb, dbCfg, true)
-		defer testhelper.MustClose(tb, postgresDB)
-
 		// We need to force-terminate open connections as for the tasks that use PgBouncer
 		// the actual client connected to the database is a PgBouncer and not a test that is
 		// running.
 		requireTerminateAllConnections(tb, postgresDB, database)
 
-		_, err = postgresDB.Exec("DROP DATABASE " + database)
+		_, err := postgresDB.Exec("DROP DATABASE " + database)
 		require.NoErrorf(tb, err, "failed to drop %q database", database)
 	})
 
-	// Connect to the testing database with optional PgBouncer
-	dbCfg.DBName = database
-	praefectTestDB := requireSQLOpen(tb, dbCfg, false)
+	praefectDBCfg := GetConfig(tb, database)
+	praefectDB := requireSQLOpen(tb, praefectDBCfg, false)
 	tb.Cleanup(func() {
-		if err := praefectTestDB.Close(); !errors.Is(err, net.ErrClosed) {
-			require.NoErrorf(tb, err, "release connection to the %q database", dbCfg.DBName)
+		if err := praefectDB.Close(); !errors.Is(err, net.ErrClosed) {
+			require.NoErrorf(tb, err, "release connection to the %q database", database)
 		}
 	})
-	return praefectTestDB
-}
 
-func databaseExist(tb testing.TB, db *sql.DB, database string) bool {
-	return scanSingleBool(tb, db, `SELECT EXISTS(SELECT * FROM pg_database WHERE datname = $1)`, database)
-}
+	_, err = glsql.Migrate(praefectDB, false)
+	require.NoErrorf(tb, err, "failed to run database migration on %q", database)
 
-func scanSingleBool(tb testing.TB, db *sql.DB, query string, args ...interface{}) bool {
-	var flag bool
-	row := db.QueryRow(query, args...)
-	require.NoError(tb, row.Scan(&flag))
-	return flag
+	return praefectDB
 }
 
 var (