mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 14:30:13 +00:00
perf(store): clone Postgres test databases from a migrated template (TASK-2900) (#1260)
`storetest.NewPostgres` and its in-package twin `testStorePostgres` ran the
full 87-file migration chain for EVERY test that asked for a store. The schema
is identical for every one of them, so building it per test bought nothing.
Measured on this box, at 8910ad06, before any change:
per store construction CREATE DATABASE 21ms
migrate chain 385ms <- 92% of the toll
DROP DATABASE 13ms
Postgres store leg wall 495.8s / test-binary CPU 93.7s
18% of that leg's wall clock was CPU inside the test binary. The other 82% was
the binary waiting on postgres, and the container's own cgroup accounting put
407.2s of CPU on the other side of the socket — 4.3x what the test process
burned. The per-test distribution is flat (slowest test 3.9%, top twenty 19%),
so there was no fat test to find: ~850 tests were each paying a fixed toll.
`CREATE DATABASE ... TEMPLATE <migrated>` is a file-level copy, and it is the
Postgres analogue of the SQLite template-and-copy this suite has had since
IDEA-1914. Both helpers now build one migrated template per test binary
(sync.Once, RWMutex-guarded against Cleanup) and clone per test.
Results, three repeats per driver on a quiet box:
Postgres wall 495.8s -> 224.4s -54.7%
Postgres container CPU 407.2s -> 167.6s -58.8%
SQLite wall 185.6s -> 186.0s unchanged (the control)
The change DELETES work rather than rearranging it — container CPU is the
figure that says so, and the flat SQLite row is what lets the Postgres delta be
attributed to the template rather than to the environment.
Predicted before building, and recorded on the trail before the measurement
existed: container CPU between 134s and 208s. Actual 167.6s. Rearranged, that
yields 850 store constructions per leg, against a static lower bound of 708 and
a dynamic ceiling of 970 — two instruments sharing no mechanism agreeing on a
number the bracket could only bound.
THE REGRESSION THIS INVITES, and the test that catches it:
A template that silently stopped being used would pass every other test in the
repository. A clone and a per-test migration produce byte-identical databases,
so no assertion about schema or behaviour distinguishes them, and the
build-once counter reads 1 either way — it proves the template was BUILT once,
not that anything was CLONED from it. Drop the TEMPLATE clause and the whole
cost returns silently.
So the template is stamped after migrating with a marker only a file-level copy
can carry, and both helpers assert it on the database they hand out. A table
comment carries it because it lives in the database's own pg_description; a
DATABASE comment would not, since those live in the shared pg_shdescription
keyed by an oid the clone does not inherit. schema_migrations.applied_at was
rejected as the carrier: RFC3339 is second-resolution and a fresh 385ms
migration can land inside the same second as the template's.
Mutation matrix — the class has exactly two members, because the import cycle
forces the helper to be duplicated:
storetest twin, TEMPLATE removed exactly ONE test fails (the marker test);
the other five pass
in-package helper, TEMPLATE removed exactly ONE test fails (its counterpart),
and the run takes 21.2s against 0.9s
Both controls were run only after committing, so restoring from HEAD could not
delete uncommitted work.
No test weakened: per-test outcome sets compared before and after on both
drivers. Zero tests disappeared, zero changed outcome. The only deltas are the
three new mechanism tests, which pass on Postgres and skip on SQLite.
Cleanup() gains the Postgres half; its three existing callers need no change,
because the invariant they rely on — "tears down whatever templates this binary
built" — is what widened. A binary killed mid-run leaks its pad_tmpl_* database,
bounded by the container's lifetime, since the test stack is per-worktree
(TASK-2708) and `make test-pg` tears it down with -v.
Codex round 2 (enumeration-shaped, aimed at the two helpers' divergence) found
one real P2: the in-package helper's FAILURE path dropped its half-built clone
without WITH (FORCE), while the twin's used it. NewPostgres can fail with its
pool already open, since a migration error arrives after the connection does,
and a plain DROP is refused while a session is attached — so the un-forced form
leaked a pad_test_* database on exactly the path that most needs cleaning up.
Pre-existing and asymmetric; corrected here because this change makes the two
helpers a matched pair. Class swept: all three remaining DROP DATABASE sites in
the repository now use WITH (FORCE), which is the whole population.
Round 2 also noted the helpers differ in admin-connection plumbing — the
in-package one goes through newPostgresConn (guarded driver, Ping, pool limits)
and the twin through a raw sql.Open. Accepted rather than unified: it predates
this change, the SQL either side is fixed and parameter-free, and the only
difference is connection validation, which the twin's t.Fatalf surfaces anyway.
Deliverable 3 (t.Parallel on the SQLite leg) follows as a separate package under
the same task, per the lead's split ruling.
Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR
This commit is contained in:
@@ -11,11 +11,15 @@ import (
|
||||
// and internal/server/main_test.go for context — the same reasoning
|
||||
// applies to internal/store tests that call CreateUser directly.
|
||||
//
|
||||
// It also tears down testStoreSQLite's process-wide template DB
|
||||
// (IDEA-1914, store_test.go) after the suite finishes.
|
||||
// It also tears down the process-wide template databases after the suite
|
||||
// finishes: testStoreSQLite's template FILE (IDEA-1914, store_test.go) and
|
||||
// testStorePostgres's template DATABASE (TASK-2900, same file). The Postgres
|
||||
// teardown is a no-op when the suite ran on SQLite, since no template was
|
||||
// built.
|
||||
func TestMain(m *testing.M) {
|
||||
SetBcryptCostForTesting(bcrypt.MinCost)
|
||||
code := m.Run()
|
||||
removeSQLiteTemplate()
|
||||
removePostgresTemplate()
|
||||
os.Exit(code)
|
||||
}
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
package store
|
||||
|
||||
import (
|
||||
"os"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TASK-2900. The in-package mirror of storetest/postgres_test.go's mechanism
|
||||
// tests, and the reason there are two is the reason there are two helpers:
|
||||
// internal/store's white-box tests cannot import storetest without an import
|
||||
// cycle, so the template machinery is duplicated, and a mechanism test that
|
||||
// covered only one copy would leave the copy 700 tests actually use unguarded.
|
||||
//
|
||||
// This file is the one that matters most by volume. `testStore(t)` routes here
|
||||
// under PAD_TEST_POSTGRES_URL, and it is called from 699 sites in this package.
|
||||
|
||||
// TestTestStorePostgres_DatabasesAreClonedNotMigrated pins the mechanism.
|
||||
//
|
||||
// Nothing else can. A clone and a per-test migration produce byte-identical
|
||||
// databases, so no assertion about schema or behaviour distinguishes them:
|
||||
// revert `CREATE DATABASE ... TEMPLATE` to a plain `CREATE DATABASE` and all
|
||||
// 902 other tests in this package still pass while the whole per-test
|
||||
// migration cost silently returns. Verified by mutation against the storetest
|
||||
// twin, where dropping the TEMPLATE clause failed exactly one test — this one's
|
||||
// counterpart — and left the other five green.
|
||||
//
|
||||
// The discriminator is a marker stamped on the template after migrating, which
|
||||
// a file-level clone carries and a fresh migration cannot produce.
|
||||
func TestTestStorePostgres_DatabasesAreClonedNotMigrated(t *testing.T) {
|
||||
baseURL := os.Getenv("PAD_TEST_POSTGRES_URL")
|
||||
if baseURL == "" {
|
||||
t.Skip("PAD_TEST_POSTGRES_URL not set; this test is about the Postgres fixture")
|
||||
}
|
||||
s := testStore(t)
|
||||
|
||||
pgTemplateMu.RLock()
|
||||
want := pgTemplateMarker(pgTemplateName)
|
||||
pgTemplateMu.RUnlock()
|
||||
|
||||
var got *string
|
||||
if err := s.db.QueryRow(`SELECT obj_description('schema_migrations'::regclass, 'pg_class')`).Scan(&got); err != nil {
|
||||
t.Fatalf("read template marker from the handed-out database: %v", err)
|
||||
}
|
||||
if got == nil {
|
||||
t.Fatal("the database handed out carries NO template marker — it was migrated from scratch " +
|
||||
"rather than cloned; the per-test migration cost this package's 699 testStore(t) call sites " +
|
||||
"used to pay is back")
|
||||
}
|
||||
if *got != want {
|
||||
t.Fatalf("template marker mismatch: got %q, want %q", *got, want)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTestStorePostgres_TemplateBuildsOnce asserts the migration chain runs at
|
||||
// most once per test binary. It is NOT a substitute for the test above —
|
||||
// deliberately noted, because the counter reads 1 whether or not anything was
|
||||
// cloned from the template, which is exactly the trap the mutation exposed.
|
||||
func TestTestStorePostgres_TemplateBuildsOnce(t *testing.T) {
|
||||
if os.Getenv("PAD_TEST_POSTGRES_URL") == "" {
|
||||
t.Skip("PAD_TEST_POSTGRES_URL not set")
|
||||
}
|
||||
_ = testStore(t)
|
||||
_ = testStore(t)
|
||||
|
||||
if got := atomic.LoadInt32(&pgTemplateBuilds); got != 1 {
|
||||
t.Fatalf("buildPostgresTemplate ran %d times, want exactly 1 (sync.Once)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTestStorePostgres_ClonesAreIsolated proves each caller gets its own
|
||||
// writable database rather than a shared one — the failure a template
|
||||
// optimisation invites is handing out the template itself.
|
||||
func TestTestStorePostgres_ClonesAreIsolated(t *testing.T) {
|
||||
if os.Getenv("PAD_TEST_POSTGRES_URL") == "" {
|
||||
t.Skip("PAD_TEST_POSTGRES_URL not set")
|
||||
}
|
||||
sA := testStore(t)
|
||||
ws := createTestWorkspace(t, sA, "Clone Isolation A")
|
||||
|
||||
sB := testStore(t)
|
||||
leaked, err := sB.GetWorkspaceBySlug(ws.Slug)
|
||||
if err != nil {
|
||||
t.Fatalf("query store B: %v", err)
|
||||
}
|
||||
if leaked != nil {
|
||||
t.Fatalf("workspace %q created in store A is visible in store B; clones are not isolated", ws.Slug)
|
||||
}
|
||||
}
|
||||
+142
-13
@@ -7,6 +7,7 @@ import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
@@ -153,38 +154,87 @@ func removeSQLiteTemplate() {
|
||||
}
|
||||
}
|
||||
|
||||
// testStorePostgres creates an isolated test database on the PostgreSQL server.
|
||||
// It connects to the base URL, creates a randomly-named database, runs
|
||||
// migrations, and drops the database when the test finishes.
|
||||
// pgTemplate* mirror storetest/postgres.go's template machinery, duplicated
|
||||
// here for the same import-cycle reason testStoreSQLite is. KEEP IN SYNC.
|
||||
var (
|
||||
pgTemplateMu sync.RWMutex
|
||||
pgTemplateOnce sync.Once
|
||||
pgTemplateName string
|
||||
pgTemplateErr error
|
||||
|
||||
// pgCloneMu serialises CREATE DATABASE ... TEMPLATE within this binary:
|
||||
// Postgres refuses to clone a database another session is connected to,
|
||||
// and a clone briefly connects to the source. Uncontended while tests are
|
||||
// serial; here because TASK-2900 may make some of them parallel, and a
|
||||
// free lock costs less than a flake that only appears under concurrency.
|
||||
pgCloneMu sync.Mutex
|
||||
|
||||
// pgTemplateBuilds is test-only instrumentation mirroring
|
||||
// storetest's pgBuildCount. Note what it does NOT prove: it reads 1
|
||||
// whether or not anything was cloned from the template. The clone itself
|
||||
// is pinned by the marker, not by this counter.
|
||||
pgTemplateBuilds int32
|
||||
)
|
||||
|
||||
// testStorePostgres creates an isolated test database on the PostgreSQL server
|
||||
// by CLONING a per-binary template that has already been migrated.
|
||||
//
|
||||
// TASK-2900: this used to run the full migration chain per TEST — 385ms of a
|
||||
// 420ms per-construction toll, 386ms of it in the container's own CPU
|
||||
// accounting, against ~700 constructions in this package and 407s of container
|
||||
// CPU for the whole Postgres leg. The schema is identical for every test, so
|
||||
// building it per test bought nothing. This is the Postgres analogue of the
|
||||
// SQLite template-and-copy above (IDEA-1914).
|
||||
//
|
||||
// KEEP IN SYNC with internal/store/storetest/postgres.go, which carries the
|
||||
// same machinery for tests outside this package.
|
||||
func testStorePostgres(t *testing.T, baseURL string) *Store {
|
||||
t.Helper()
|
||||
|
||||
// Generate a unique database name for this test.
|
||||
dbName := "pad_test_" + uuid.New().String()[:8]
|
||||
|
||||
pgTemplateMu.RLock()
|
||||
defer pgTemplateMu.RUnlock()
|
||||
|
||||
pgTemplateOnce.Do(func() {
|
||||
pgTemplateName, pgTemplateErr = buildPostgresTemplate(baseURL)
|
||||
})
|
||||
if pgTemplateErr != nil {
|
||||
t.Fatalf("build postgres template: %v", pgTemplateErr)
|
||||
}
|
||||
|
||||
// Connect to the default "pad" database to issue CREATE/DROP DATABASE.
|
||||
adminStore, err := newPostgresConn(baseURL)
|
||||
if err != nil {
|
||||
t.Fatalf("connect to postgres for admin: %v", err)
|
||||
}
|
||||
|
||||
// CREATE DATABASE cannot run inside a transaction.
|
||||
if _, err := adminStore.db.Exec(fmt.Sprintf("CREATE DATABASE %s", dbName)); err != nil {
|
||||
adminStore.Close()
|
||||
t.Fatalf("create test database %s: %v", dbName, err)
|
||||
}
|
||||
pgCloneMu.Lock()
|
||||
_, err = adminStore.db.Exec(fmt.Sprintf("CREATE DATABASE %s TEMPLATE %s", dbName, pgTemplateName))
|
||||
pgCloneMu.Unlock()
|
||||
adminStore.Close()
|
||||
if err != nil {
|
||||
t.Fatalf("clone test database %s from template %s: %v", dbName, pgTemplateName, err)
|
||||
}
|
||||
|
||||
// Build the connection string for the new database.
|
||||
testURL := replaceDBName(baseURL, dbName)
|
||||
|
||||
// The clone is already migrated, so this constructor's own migrate() is a
|
||||
// fast no-op — schema_migrations arrived with the copy. Deliberately not
|
||||
// skipped: the fixture goes through the same constructor as production.
|
||||
s, err := NewPostgres(testURL)
|
||||
if err != nil {
|
||||
// Clean up the database we just created.
|
||||
if admin2, err2 := newPostgresConn(baseURL); err2 == nil {
|
||||
admin2.db.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %s", dbName))
|
||||
admin2.Close()
|
||||
}
|
||||
// Clean up the database we just created. WITH (FORCE), matching the
|
||||
// success path below and storetest's twin: NewPostgres can fail with
|
||||
// its pool already open — a migration error arrives after the
|
||||
// connection does — and a plain DROP is refused while any session is
|
||||
// attached, so the un-forced form left an orphaned pad_test_* database
|
||||
// behind on exactly the path that most needs cleaning up. Pre-existing
|
||||
// and asymmetric with the twin; corrected here because this change
|
||||
// makes the two helpers a matched pair (codex round 2, P2).
|
||||
dropPostgresTemplateDB(baseURL, dbName)
|
||||
t.Fatalf("open test postgres store: %v", err)
|
||||
}
|
||||
|
||||
@@ -200,6 +250,85 @@ func testStorePostgres(t *testing.T, baseURL string) *Store {
|
||||
return s
|
||||
}
|
||||
|
||||
// buildPostgresTemplate creates ONE fully-migrated database per test binary.
|
||||
// Callers hold pgTemplateMu for read.
|
||||
//
|
||||
// Closing the migrating connection is load-bearing, not hygiene: CREATE
|
||||
// DATABASE ... TEMPLATE fails while any session is connected to the source.
|
||||
//
|
||||
// LEAK POSTURE: a binary killed mid-run leaves its pad_tmpl_* database behind,
|
||||
// bounded by the container's lifetime — the test stack is per-worktree
|
||||
// (TASK-2708) and `make test-pg` tears it down with -v.
|
||||
func buildPostgresTemplate(baseURL string) (string, error) {
|
||||
atomic.AddInt32(&pgTemplateBuilds, 1)
|
||||
name := "pad_tmpl_" + uuid.New().String()[:8]
|
||||
|
||||
admin, err := newPostgresConn(baseURL)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("connect to postgres for admin: %w", err)
|
||||
}
|
||||
if _, err := admin.db.Exec(fmt.Sprintf("CREATE DATABASE %s", name)); err != nil {
|
||||
admin.Close()
|
||||
return "", fmt.Errorf("create template database %s: %w", name, err)
|
||||
}
|
||||
admin.Close()
|
||||
|
||||
// The one time this binary pays for the migration chain.
|
||||
s, err := NewPostgres(replaceDBName(baseURL, name))
|
||||
if err != nil {
|
||||
dropPostgresTemplateDB(baseURL, name)
|
||||
return "", fmt.Errorf("migrate template database %s: %w", name, err)
|
||||
}
|
||||
// Stamp the template with a marker a freshly-migrated database cannot have.
|
||||
// Without it this mechanism is untestable: a clone and a per-test migration
|
||||
// produce otherwise identical databases, so reverting the TEMPLATE clause
|
||||
// would restore the entire cost while leaving every test in the repository
|
||||
// green. A table comment carries it because it lives in this database's own
|
||||
// pg_description and travels with a file-level clone, while being invisible
|
||||
// to product queries and to the NUL-scan table walkers, which read DATA.
|
||||
// schema_migrations.applied_at is NOT sufficient: RFC3339 is
|
||||
// second-resolution and a fresh 385ms migration can land in the same second.
|
||||
// See storetest/postgres.go, which carries the same marker for the same
|
||||
// reason. KEEP IN SYNC.
|
||||
if _, err := s.db.Exec(fmt.Sprintf("COMMENT ON TABLE schema_migrations IS '%s'", pgTemplateMarker(name))); err != nil {
|
||||
s.Close()
|
||||
dropPostgresTemplateDB(baseURL, name)
|
||||
return "", fmt.Errorf("stamp template %s: %w", name, err)
|
||||
}
|
||||
if err := s.Close(); err != nil {
|
||||
dropPostgresTemplateDB(baseURL, name)
|
||||
return "", fmt.Errorf("close template connection %s: %w", name, err)
|
||||
}
|
||||
return name, nil
|
||||
}
|
||||
|
||||
// pgTemplateMarker is the sentinel stamped on the template and expected on
|
||||
// every database handed out. See buildPostgresTemplate.
|
||||
func pgTemplateMarker(templateName string) string {
|
||||
return "storetest-pg-template:" + templateName
|
||||
}
|
||||
|
||||
// removePostgresTemplate drops the per-binary template. Called from TestMain
|
||||
// after m.Run(), alongside removeSQLiteTemplate.
|
||||
func removePostgresTemplate() {
|
||||
pgTemplateMu.Lock()
|
||||
defer pgTemplateMu.Unlock()
|
||||
if pgTemplateName == "" {
|
||||
return
|
||||
}
|
||||
if baseURL := os.Getenv("PAD_TEST_POSTGRES_URL"); baseURL != "" {
|
||||
dropPostgresTemplateDB(baseURL, pgTemplateName)
|
||||
}
|
||||
pgTemplateName = ""
|
||||
}
|
||||
|
||||
func dropPostgresTemplateDB(baseURL, name string) {
|
||||
if admin, err := newPostgresConn(baseURL); err == nil {
|
||||
admin.db.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %s WITH (FORCE)", name))
|
||||
admin.Close()
|
||||
}
|
||||
}
|
||||
|
||||
// newPostgresConn opens a raw postgres connection (no migrations).
|
||||
func newPostgresConn(connStr string) (*Store, error) {
|
||||
db, err := openPostgresDB(connStr)
|
||||
|
||||
@@ -2,8 +2,11 @@ package storetest
|
||||
|
||||
import (
|
||||
"database/sql"
|
||||
"fmt"
|
||||
"os"
|
||||
"strings"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
@@ -11,17 +14,59 @@ import (
|
||||
"github.com/PerpetualSoftware/pad/internal/store"
|
||||
)
|
||||
|
||||
var (
|
||||
// pgTemplateMu guards pgTemplateName against Cleanup racing an in-flight
|
||||
// build or clone — the same race as templateMu, and the same remedy:
|
||||
// NewPostgres holds it for read across build + clone, Cleanup takes the
|
||||
// write lock before dropping.
|
||||
pgTemplateMu sync.RWMutex
|
||||
pgTemplateOnce sync.Once
|
||||
pgTemplateName string
|
||||
pgTemplateErr error
|
||||
|
||||
// pgCloneMu serialises CREATE DATABASE ... TEMPLATE within this binary.
|
||||
//
|
||||
// Postgres refuses to clone a database that any other session is connected
|
||||
// to ("source database is being accessed by other users"), and a clone
|
||||
// briefly connects to the source. Serial tests would not need this; the
|
||||
// lock is here because TASK-2900 may add t.Parallel() to tests this helper
|
||||
// backs, and a lock that is uncontended today costs less than a flake that
|
||||
// only appears once tests run concurrently. The serialised section is the
|
||||
// clone itself, measured at ~36ms.
|
||||
pgCloneMu sync.Mutex
|
||||
|
||||
// pgBuildCount is test-only instrumentation, mirroring buildCount for the
|
||||
// SQLite template. It exists because a template that silently stopped
|
||||
// being used would still pass every test in the suite — the clone and the
|
||||
// per-test migration produce identical databases, so correctness cannot
|
||||
// distinguish them and only a count can. See postgres_test.go.
|
||||
pgBuildCount int32
|
||||
)
|
||||
|
||||
// NewPostgres returns a *store.Store backed by an ISOLATED PostgreSQL database
|
||||
// when PAD_TEST_POSTGRES_URL is set, and t.Skip()s the test otherwise. It lets
|
||||
// tests OUTSIDE the store package (e.g. internal/server) exercise Postgres-gated
|
||||
// code paths under `make test-pg`, where the store-package white-box helper
|
||||
// (internal/store/store_test.go::testStorePostgres) isn't importable.
|
||||
//
|
||||
// It mirrors that helper: a uniquely-named database is CREATEd off the base URL,
|
||||
// opened via store.NewPostgres (which runs the full migration chain), and DROPped
|
||||
// on cleanup. The pgx driver is already registered transitively via the store
|
||||
// import. KEEP IN SYNC with store_test.go's testStorePostgres (duplicated for the
|
||||
// same import-cycle reason as NewSQLite — see the package doc).
|
||||
// The database is CLONED from a per-binary template that has already been
|
||||
// migrated, not migrated from scratch (TASK-2900). The pgx driver is already
|
||||
// registered transitively via the store import. KEEP IN SYNC with
|
||||
// store_test.go's testStorePostgres (duplicated for the same import-cycle
|
||||
// reason as NewSQLite — see the package doc), which carries the same template
|
||||
// machinery.
|
||||
//
|
||||
// Why the template: the migration chain used to run once per TEST. Measured at
|
||||
// 385ms of a 420ms per-construction toll, against ~700 constructions in
|
||||
// internal/store alone — which is why that leg spent four fifths of its wall
|
||||
// clock waiting on postgres rather than working. In the container's own CPU
|
||||
// accounting the toll was 386ms per construction against 407s for the whole
|
||||
// leg. Cloning an already-migrated template is a file-level copy: 104ms of
|
||||
// container CPU per clone plus 346ms once per binary.
|
||||
//
|
||||
// This is the Postgres analogue of the SQLite template-and-copy this package
|
||||
// has had since IDEA-1914, and it is here for the same reason: the schema is
|
||||
// identical for every test, so building it per test buys nothing.
|
||||
func NewPostgres(t *testing.T) *store.Store {
|
||||
t.Helper()
|
||||
|
||||
@@ -32,21 +77,37 @@ func NewPostgres(t *testing.T) *store.Store {
|
||||
|
||||
dbName := "pad_test_" + strings.ReplaceAll(uuid.New().String()[:8], "-", "")
|
||||
|
||||
pgTemplateMu.RLock()
|
||||
defer pgTemplateMu.RUnlock()
|
||||
|
||||
pgTemplateOnce.Do(func() {
|
||||
pgTemplateName, pgTemplateErr = buildPGTemplate(baseURL)
|
||||
})
|
||||
if pgTemplateErr != nil {
|
||||
t.Fatalf("storetest: build pg template: %v", pgTemplateErr)
|
||||
}
|
||||
|
||||
admin, err := sql.Open("pgx", baseURL)
|
||||
if err != nil {
|
||||
t.Fatalf("storetest: open pg admin conn: %v", err)
|
||||
}
|
||||
// CREATE DATABASE cannot run inside a transaction.
|
||||
if _, err := admin.Exec("CREATE DATABASE " + dbName); err != nil {
|
||||
_ = admin.Close()
|
||||
t.Fatalf("storetest: create test database %s: %v", dbName, err)
|
||||
}
|
||||
pgCloneMu.Lock()
|
||||
_, err = admin.Exec("CREATE DATABASE " + dbName + " TEMPLATE " + pgTemplateName)
|
||||
pgCloneMu.Unlock()
|
||||
_ = admin.Close()
|
||||
if err != nil {
|
||||
t.Fatalf("storetest: clone test database %s from template %s: %v", dbName, pgTemplateName, err)
|
||||
}
|
||||
|
||||
// The clone arrives already migrated, so store.NewPostgres's own migrate()
|
||||
// is a fast no-op — schema_migrations came with the copy. Deliberately NOT
|
||||
// skipped: going through the same constructor production uses is what keeps
|
||||
// the fixture honest, and it is the same choice NewSQLite makes.
|
||||
s, err := store.NewPostgres(replaceDBName(baseURL, dbName))
|
||||
if err != nil {
|
||||
dropPostgresDB(baseURL, dbName)
|
||||
t.Fatalf("storetest: open test postgres store: %v", err)
|
||||
t.Fatalf("storetest: open cloned test postgres store: %v", err)
|
||||
}
|
||||
t.Cleanup(func() {
|
||||
_ = s.Close()
|
||||
@@ -55,6 +116,87 @@ func NewPostgres(t *testing.T) *store.Store {
|
||||
return s
|
||||
}
|
||||
|
||||
// buildPGTemplate creates ONE fully-migrated database per test binary and
|
||||
// returns its name. Callers hold pgTemplateMu for read.
|
||||
//
|
||||
// The connection that runs the migrations is closed before returning, and that
|
||||
// close is load-bearing rather than hygiene: CREATE DATABASE ... TEMPLATE fails
|
||||
// outright while any session is still connected to the source.
|
||||
//
|
||||
// LEAK POSTURE: a test binary killed mid-run leaves its pad_tmpl_* database
|
||||
// behind, because Cleanup never runs. That is bounded rather than unbounded —
|
||||
// the Postgres container is per-worktree (TASK-2708) and `make test-pg` tears
|
||||
// its stack down with -v, so an orphaned template dies with the container that
|
||||
// holds it. Not worth a reaper.
|
||||
func buildPGTemplate(baseURL string) (string, error) {
|
||||
atomic.AddInt32(&pgBuildCount, 1)
|
||||
name := "pad_tmpl_" + strings.ReplaceAll(uuid.New().String()[:8], "-", "")
|
||||
|
||||
admin, err := sql.Open("pgx", baseURL)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("open pg admin conn: %w", err)
|
||||
}
|
||||
if _, err := admin.Exec("CREATE DATABASE " + name); err != nil {
|
||||
_ = admin.Close()
|
||||
return "", fmt.Errorf("create template database %s: %w", name, err)
|
||||
}
|
||||
_ = admin.Close()
|
||||
|
||||
// The one time this binary pays for the migration chain.
|
||||
s, err := store.NewPostgres(replaceDBName(baseURL, name))
|
||||
if err != nil {
|
||||
dropPostgresDB(baseURL, name)
|
||||
return "", fmt.Errorf("migrate template database %s: %w", name, err)
|
||||
}
|
||||
// Stamp the template with a marker a FRESHLY-MIGRATED database cannot
|
||||
// have. This is what makes the mechanism testable at all: a clone and a
|
||||
// per-test migration produce otherwise byte-identical databases, so no
|
||||
// assertion about schema or behaviour can tell them apart, and the
|
||||
// build-once counter only proves the template was BUILT once — not that
|
||||
// anything was cloned FROM it. A regression that quietly reverted
|
||||
// `CREATE DATABASE ... TEMPLATE` to a plain CREATE would pass every other
|
||||
// test in the repository while restoring the whole cost this change removes.
|
||||
//
|
||||
// A table comment is the carrier because it lives in the database's own
|
||||
// pg_description and therefore travels with a file-level clone, while
|
||||
// being invisible to every query the product makes and to the table
|
||||
// walkers in the NUL-scan tests, which enumerate DATA. A database comment
|
||||
// would not work: those live in the shared pg_shdescription keyed by
|
||||
// database oid, and a clone gets a new oid.
|
||||
//
|
||||
// applied_at was the obvious alternative and is NOT sufficient: it is
|
||||
// RFC3339, so second-resolution, and a fresh 385ms migration can land
|
||||
// inside the same second as the template's and produce an identical set.
|
||||
if _, err := s.DB().Exec(fmt.Sprintf("COMMENT ON TABLE schema_migrations IS '%s'", templateMarker(name))); err != nil {
|
||||
_ = s.Close()
|
||||
dropPostgresDB(baseURL, name)
|
||||
return "", fmt.Errorf("stamp template %s: %w", name, err)
|
||||
}
|
||||
if err := s.Close(); err != nil {
|
||||
dropPostgresDB(baseURL, name)
|
||||
return "", fmt.Errorf("close template connection %s: %w", name, err)
|
||||
}
|
||||
return name, nil
|
||||
}
|
||||
|
||||
// templateMarker is the sentinel stamped on the template and expected on every
|
||||
// clone. See buildPGTemplate for why a table comment carries it.
|
||||
func templateMarker(templateName string) string {
|
||||
return "storetest-pg-template:" + templateName
|
||||
}
|
||||
|
||||
// dropPGTemplate removes the per-binary template database, if one was built.
|
||||
// Called from Cleanup under the write lock.
|
||||
func dropPGTemplate() {
|
||||
if pgTemplateName == "" {
|
||||
return
|
||||
}
|
||||
if baseURL := os.Getenv("PAD_TEST_POSTGRES_URL"); baseURL != "" {
|
||||
dropPostgresDB(baseURL, pgTemplateName)
|
||||
}
|
||||
pgTemplateName = ""
|
||||
}
|
||||
|
||||
func dropPostgresDB(baseURL, dbName string) {
|
||||
admin, err := sql.Open("pgx", baseURL)
|
||||
if err != nil {
|
||||
|
||||
@@ -0,0 +1,153 @@
|
||||
package storetest
|
||||
|
||||
import (
|
||||
"os"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// TASK-2900. These mirror the SQLite template's tests, and they exist for a
|
||||
// reason worth stating plainly: **a Postgres template that silently stopped
|
||||
// being used would still pass every other test in the repository.** The clone
|
||||
// and the per-test migration produce byte-identical databases, so no assertion
|
||||
// about behaviour can tell them apart. Only the build COUNT can, which makes
|
||||
// these the wiring tests for the mechanism rather than tests of the fixture.
|
||||
|
||||
// TestNewPostgres_TemplateBuildsOnce asserts the migration chain runs at most
|
||||
// once per test binary no matter how many databases are handed out — the whole
|
||||
// point of the change. pgBuildCount is monotonic and capped at 1 by
|
||||
// pgTemplateOnce, so asserting it is exactly 1 after at least one NewPostgres
|
||||
// call is valid whatever ran before this test in the binary.
|
||||
func TestNewPostgres_TemplateBuildsOnce(t *testing.T) {
|
||||
if os.Getenv("PAD_TEST_POSTGRES_URL") == "" {
|
||||
t.Skip("PAD_TEST_POSTGRES_URL not set")
|
||||
}
|
||||
_ = NewPostgres(t)
|
||||
_ = NewPostgres(t)
|
||||
_ = NewPostgres(t)
|
||||
|
||||
if got := atomic.LoadInt32(&pgBuildCount); got != 1 {
|
||||
t.Fatalf("buildPGTemplate ran %d times total, want exactly 1 (sync.Once)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestNewPostgres_ClonesAreIsolatedAndFunctional proves each call returns its
|
||||
// own writable, working database rather than a shared one: a real end-to-end
|
||||
// write through one store must not be visible from another.
|
||||
//
|
||||
// This is the assertion that would fail if CREATE DATABASE ... TEMPLATE were
|
||||
// ever replaced by something that handed out the template itself, which is the
|
||||
// failure mode a speed optimisation invites.
|
||||
func TestNewPostgres_ClonesAreIsolatedAndFunctional(t *testing.T) {
|
||||
if os.Getenv("PAD_TEST_POSTGRES_URL") == "" {
|
||||
t.Skip("PAD_TEST_POSTGRES_URL not set")
|
||||
}
|
||||
sA := NewPostgres(t)
|
||||
|
||||
u, err := sA.CreateUser(models.UserCreate{
|
||||
Email: "pgtemplate-probe@example.com",
|
||||
Username: "pgtemplate-probe",
|
||||
Name: "PG Template Probe",
|
||||
Password: "correcthorsebattery",
|
||||
Role: "admin",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("create user: %v", err)
|
||||
}
|
||||
ws, err := sA.CreateWorkspace(models.WorkspaceCreate{Name: "Probe", Slug: "pgtemplate-probe", OwnerID: u.ID})
|
||||
if err != nil {
|
||||
t.Fatalf("create workspace: %v", err)
|
||||
}
|
||||
if got, err := sA.GetWorkspaceBySlug(ws.Slug); err != nil || got == nil {
|
||||
t.Fatalf("workspace not readable back from its own clone: got=%v err=%v", got, err)
|
||||
}
|
||||
|
||||
sB := NewPostgres(t)
|
||||
if leaked, err := sB.GetWorkspaceBySlug(ws.Slug); err != nil {
|
||||
t.Fatalf("query store B: %v", err)
|
||||
} else if leaked != nil {
|
||||
t.Fatal("workspace created in store A is visible in store B; clones are not isolated")
|
||||
}
|
||||
}
|
||||
|
||||
// TestNewPostgres_DatabasesAreClonedNotMigrated is the test that actually
|
||||
// pins the mechanism, and it exists because the build-once counter above does
|
||||
// NOT.
|
||||
//
|
||||
// pgBuildCount proves the template was BUILT once. It says nothing about
|
||||
// whether anything was cloned from it: revert `CREATE DATABASE ... TEMPLATE`
|
||||
// to a plain `CREATE DATABASE` and the counter still reads 1, every other test
|
||||
// in the repository still passes, and the entire cost this change removes
|
||||
// comes back silently. That is the regression this test is for, and it was
|
||||
// only findable by asking what a mutation would leave green.
|
||||
//
|
||||
// The discriminator is a marker stamped on the template after migrating,
|
||||
// carried into a clone by the file-level copy and absent from anything
|
||||
// migrated from scratch. See buildPGTemplate for why it is a table comment and
|
||||
// why schema_migrations.applied_at is not good enough.
|
||||
func TestNewPostgres_DatabasesAreClonedNotMigrated(t *testing.T) {
|
||||
if os.Getenv("PAD_TEST_POSTGRES_URL") == "" {
|
||||
t.Skip("PAD_TEST_POSTGRES_URL not set")
|
||||
}
|
||||
s := NewPostgres(t)
|
||||
|
||||
pgTemplateMu.RLock()
|
||||
want := templateMarker(pgTemplateName)
|
||||
pgTemplateMu.RUnlock()
|
||||
|
||||
var got *string
|
||||
err := s.DB().QueryRow(`SELECT obj_description('schema_migrations'::regclass, 'pg_class')`).Scan(&got)
|
||||
if err != nil {
|
||||
t.Fatalf("read template marker from the handed-out database: %v", err)
|
||||
}
|
||||
if got == nil {
|
||||
t.Fatal("the database handed out carries NO template marker — it was migrated from scratch, " +
|
||||
"not cloned from the template; the per-test migration cost is back")
|
||||
}
|
||||
if *got != want {
|
||||
t.Fatalf("template marker mismatch: got %q, want %q — the database was cloned from something "+
|
||||
"other than this binary's template", *got, want)
|
||||
}
|
||||
}
|
||||
|
||||
// TestNewPostgres_CloneCarriesTheMigratedSchema is the counterpart to the
|
||||
// isolation test above, and it fails in the opposite direction: a clone that
|
||||
// arrived EMPTY would also be "isolated".
|
||||
//
|
||||
// It asserts the schema came across with the file copy by writing through a
|
||||
// table that only the late migrations create, rather than by reading
|
||||
// schema_migrations — a row count there would be satisfied by a template whose
|
||||
// migration bookkeeping was copied without the objects it describes.
|
||||
func TestNewPostgres_CloneCarriesTheMigratedSchema(t *testing.T) {
|
||||
if os.Getenv("PAD_TEST_POSTGRES_URL") == "" {
|
||||
t.Skip("PAD_TEST_POSTGRES_URL not set")
|
||||
}
|
||||
s := NewPostgres(t)
|
||||
|
||||
u, err := s.CreateUser(models.UserCreate{
|
||||
Email: "pgtemplate-schema@example.com",
|
||||
Username: "pgtemplate-schema",
|
||||
Name: "PG Template Schema",
|
||||
Password: "correcthorsebattery",
|
||||
Role: "admin",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("create user: %v", err)
|
||||
}
|
||||
ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Schema", Slug: "pgtemplate-schema", OwnerID: u.ID})
|
||||
if err != nil {
|
||||
t.Fatalf("create workspace: %v", err)
|
||||
}
|
||||
// Collections carry `traits`, added late in the chain (TASK-2657) and
|
||||
// tightened by TASK-2710's partial unique indexes. A clone missing the tail
|
||||
// of the migrations fails here rather than passing quietly.
|
||||
col, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Probe", Prefix: "PRB"})
|
||||
if err != nil {
|
||||
t.Fatalf("create collection in clone: %v", err)
|
||||
}
|
||||
if _, err := s.CreateItem(ws.ID, col.ID, models.ItemCreate{Title: "probe item"}); err != nil {
|
||||
t.Fatalf("create item in clone: %v", err)
|
||||
}
|
||||
}
|
||||
@@ -107,15 +107,24 @@ func lockedCopyFromTemplate(dst string) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// Cleanup removes the process-wide template database, if one was built.
|
||||
// Call it from TestMain after m.Run() so a lazily-built template file
|
||||
// doesn't linger past the test binary's lifetime.
|
||||
// Cleanup removes the process-wide template databases, if any were built.
|
||||
// Call it from TestMain after m.Run() so a lazily-built template doesn't
|
||||
// linger past the test binary's lifetime.
|
||||
//
|
||||
// Both templates: the SQLite template FILE (IDEA-1914) and, since TASK-2900,
|
||||
// the Postgres template DATABASE built by NewPostgres. Existing callers need
|
||||
// no change — the invariant they already rely on is "Cleanup tears down
|
||||
// whatever templates this binary built", and that is what widened.
|
||||
func Cleanup() {
|
||||
templateMu.Lock()
|
||||
defer templateMu.Unlock()
|
||||
if templatePath != "" {
|
||||
_ = os.RemoveAll(filepath.Dir(templatePath))
|
||||
}
|
||||
templateMu.Unlock()
|
||||
|
||||
pgTemplateMu.Lock()
|
||||
dropPGTemplate()
|
||||
pgTemplateMu.Unlock()
|
||||
}
|
||||
|
||||
// buildTemplate runs the full migration chain once into a process-wide
|
||||
|
||||
Reference in New Issue
Block a user