Merge pull request #1874 from binwiederhier/schema-compare-tests

Schema compare tests

db/test/schema.go

@@ -0,0 +1,185 @@
+package dbtest
+
+import (
+	"database/sql"
+	"fmt"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// Querier is the subset of *sql.DB / *db.DB needed to introspect a schema.
+type Querier interface {
+	Query(query string, args ...any) (*sql.Rows, error)
+}
+
+// SQLiteSchema returns a normalized, comparable description of the database schema: tables
+// with their columns, named indexes, and foreign keys. Column order, declared type spelling
+// (INT vs INTEGER) and default values are not part of the description, so the schema produced
+// by a migration chain can be compared to a freshly created one.
+func SQLiteSchema(t testing.TB, d Querier) string {
+	t.Helper()
+	lines := make([]string, 0)
+	for _, table := range sqliteTables(t, d) {
+		lines = append(lines, "table "+table)
+		lines = append(lines, sqliteColumns(t, d, table)...)
+		lines = append(lines, sqliteForeignKeys(t, d, table)...)
+		lines = append(lines, sqliteIndexes(t, d, table)...)
+	}
+	return strings.Join(lines, "\n")
+}
+
+// PostgresSchema is SQLiteSchema's PostgreSQL counterpart, describing the current schema's
+// tables, columns, constraints and indexes in a normalized, comparable way.
+func PostgresSchema(t testing.TB, d Querier) string {
+	t.Helper()
+	lines := make([]string, 0)
+	for _, table := range postgresTables(t, d) {
+		lines = append(lines, "table "+table)
+		lines = append(lines, postgresColumns(t, d, table)...)
+	}
+	lines = append(lines, postgresConstraints(t, d)...)
+	lines = append(lines, postgresIndexes(t, d)...)
+	return strings.Join(lines, "\n")
+}
+
+func sqliteTables(t testing.TB, d Querier) []string {
+	t.Helper()
+	return queryStrings(t, d, `SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%' ORDER BY name`)
+}
+
+func sqliteColumns(t testing.TB, d Querier, table string) []string {
+	t.Helper()
+	rows, err := d.Query(fmt.Sprintf(`PRAGMA table_info(%q)`, table))
+	require.Nil(t, err)
+	defer rows.Close()
+	lines := make([]string, 0)
+	for rows.Next() {
+		var cid, notNull, pk int
+		var name, typ string
+		var dflt sql.NullString
+		require.Nil(t, rows.Scan(&cid, &name, &typ, &notNull, &dflt, &pk))
+		typ = strings.ToUpper(typ)
+		if typ == "INT" { // INT and INTEGER are the same affinity; migrations spell them inconsistently
+			typ = "INTEGER"
+		}
+		lines = append(lines, fmt.Sprintf("  col %s %s notnull=%d pk=%d", name, typ, notNull, pk))
+	}
+	require.Nil(t, rows.Err())
+	sort.Strings(lines)
+	return lines
+}
+
+func sqliteForeignKeys(t testing.TB, d Querier, table string) []string {
+	t.Helper()
+	rows, err := d.Query(fmt.Sprintf(`PRAGMA foreign_key_list(%q)`, table))
+	require.Nil(t, err)
+	defer rows.Close()
+	lines := make([]string, 0)
+	for rows.Next() {
+		var id, seq int
+		var refTable, from, onUpdate, onDelete, match string
+		var to sql.NullString // NULL when referencing the parent's primary key implicitly
+		require.Nil(t, rows.Scan(&id, &seq, &refTable, &from, &to, &onUpdate, &onDelete, &match))
+		lines = append(lines, fmt.Sprintf("  fk %s -> %s(%s) on_delete=%s", from, refTable, to.String, onDelete))
+	}
+	require.Nil(t, rows.Err())
+	sort.Strings(lines)
+	return lines
+}
+
+func sqliteIndexes(t testing.TB, d Querier, table string) []string {
+	t.Helper()
+	rows, err := d.Query(fmt.Sprintf(`PRAGMA index_list(%q)`, table))
+	require.Nil(t, err)
+	type index struct {
+		name            string
+		unique, partial int
+	}
+	indexes := make([]index, 0)
+	for rows.Next() {
+		var seq, unique, partial int
+		var name, origin string
+		require.Nil(t, rows.Scan(&seq, &name, &unique, &origin, &partial))
+		// Skip auto-indexes backing PRIMARY KEY/UNIQUE table constraints; those are described
+		// by the column and constraint listings already
+		if strings.HasPrefix(name, "sqlite_autoindex_") {
+			continue
+		}
+		indexes = append(indexes, index{name, unique, partial})
+	}
+	require.Nil(t, rows.Err())
+	require.Nil(t, rows.Close())
+	lines := make([]string, 0, len(indexes))
+	for _, idx := range indexes {
+		cols := queryStrings(t, d, fmt.Sprintf(`SELECT name FROM pragma_index_info(%q) ORDER BY seqno`, idx.name))
+		lines = append(lines, fmt.Sprintf("  index %s unique=%d partial=%d cols=(%s)", idx.name, idx.unique, idx.partial, strings.Join(cols, ",")))
+	}
+	sort.Strings(lines)
+	return lines
+}
+
+func postgresTables(t testing.TB, d Querier) []string {
+	t.Helper()
+	return queryStrings(t, d, `SELECT table_name FROM information_schema.tables WHERE table_schema = current_schema() AND table_type = 'BASE TABLE' ORDER BY table_name`)
+}
+
+func postgresColumns(t testing.TB, d Querier, table string) []string {
+	t.Helper()
+	rows, err := d.Query(`SELECT column_name, data_type, is_nullable FROM information_schema.columns WHERE table_schema = current_schema() AND table_name = $1 ORDER BY column_name`, table)
+	require.Nil(t, err)
+	defer rows.Close()
+	lines := make([]string, 0)
+	for rows.Next() {
+		var name, typ, nullable string
+		require.Nil(t, rows.Scan(&name, &typ, &nullable))
+		lines = append(lines, fmt.Sprintf("  col %s %s nullable=%s", name, typ, nullable))
+	}
+	require.Nil(t, rows.Err())
+	return lines
+}
+
+func postgresConstraints(t testing.TB, d Querier) []string {
+	t.Helper()
+	return queryStrings(t, d, `
+		SELECT 'constraint ' || conrelid::regclass::text || ': ' || pg_get_constraintdef(oid)
+		FROM pg_constraint
+		WHERE connamespace = current_schema()::regnamespace
+		ORDER BY 1
+	`)
+}
+
+func postgresIndexes(t testing.TB, d Querier) []string {
+	t.Helper()
+	rows, err := d.Query(`SELECT indexname, indexdef, schemaname FROM pg_indexes WHERE schemaname = current_schema() ORDER BY indexname`)
+	require.Nil(t, err)
+	defer rows.Close()
+	lines := make([]string, 0)
+	for rows.Next() {
+		var name, def, schema string
+		require.Nil(t, rows.Scan(&name, &def, &schema))
+		// The index definition qualifies the table with the (test-specific) schema name; strip
+		// it so snapshots from different test schemas compare equal
+		def = strings.ReplaceAll(def, schema+".", "")
+		lines = append(lines, "index "+def)
+	}
+	require.Nil(t, rows.Err())
+	return lines
+}
+
+func queryStrings(t testing.TB, d Querier, query string) []string {
+	t.Helper()
+	rows, err := d.Query(query)
+	require.Nil(t, err)
+	defer rows.Close()
+	values := make([]string, 0)
+	for rows.Next() {
+		var value string
+		require.Nil(t, rows.Scan(&value))
+		values = append(values, value)
+	}
+	require.Nil(t, rows.Err())
+	return values
+}

docs/releases.md

@@ -2066,6 +2066,7 @@ and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/release
 * Fix Twilio phone calls and phone number verifications failing silently when Twilio rejected the request, and move the Twilio integration into its own `twilio` package
 * Move the Prometheus metrics into a dedicated `metrics` package
 * Message cache databases from ntfy older than v1.10.0 (November 2021) can no longer be migrated; upgrade via an older ntfy version first, or delete the cache database
+* Fix `user_phone` table in the SQLite user database referencing a dropped table after the v2.14 schema migration; repaired automatically by a new migration
 
 ### ntfy iOS app v1.8.0 (UNRELEASED)
 

message/cache_postgres_test.go

@@ -0,0 +1,76 @@
+package message_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	dbtest "heckel.io/ntfy/v2/db/test"
+	"heckel.io/ntfy/v2/message"
+	"heckel.io/ntfy/v2/model"
+)
+
+func TestPostgresStore_Migration_From14(t *testing.T) {
+	// A pre-framework database at version 14: full v14 schema, version tracked in the
+	// hand-rolled schema_version table, and no idx_message_attachment_expires yet
+	testDB := dbtest.CreateTestPostgres(t)
+	_, err := testDB.Exec(`
+		CREATE TABLE message (
+			id BIGSERIAL PRIMARY KEY,
+			mid TEXT NOT NULL,
+			sequence_id TEXT NOT NULL,
+			time BIGINT NOT NULL,
+			event TEXT NOT NULL,
+			expires BIGINT NOT NULL,
+			topic TEXT NOT NULL,
+			message TEXT NOT NULL,
+			title TEXT NOT NULL,
+			priority INT NOT NULL,
+			tags TEXT NOT NULL,
+			click TEXT NOT NULL,
+			icon TEXT NOT NULL,
+			actions TEXT NOT NULL,
+			attachment_name TEXT NOT NULL,
+			attachment_type TEXT NOT NULL,
+			attachment_size BIGINT NOT NULL,
+			attachment_expires BIGINT NOT NULL,
+			attachment_url TEXT NOT NULL,
+			attachment_deleted BOOLEAN NOT NULL DEFAULT FALSE,
+			sender TEXT NOT NULL,
+			user_id TEXT NOT NULL,
+			content_type TEXT NOT NULL,
+			encoding TEXT NOT NULL,
+			published BOOLEAN NOT NULL DEFAULT FALSE
+		);
+		CREATE INDEX idx_message_mid ON message (mid);
+		CREATE INDEX idx_message_sequence_id ON message (sequence_id);
+		CREATE INDEX idx_message_topic_published_time ON message (topic, published, time, id);
+		CREATE INDEX idx_message_published_expires ON message (published, expires);
+		CREATE INDEX idx_message_sender_attachment_expires ON message (sender, attachment_expires) WHERE user_id = '';
+		CREATE INDEX idx_message_user_id_attachment_expires ON message (user_id, attachment_expires);
+		CREATE TABLE message_stats (key TEXT PRIMARY KEY, value BIGINT);
+		INSERT INTO message_stats (key, value) VALUES ('messages', 0);
+		CREATE TABLE schema_version (store TEXT PRIMARY KEY, version INT NOT NULL);
+		INSERT INTO schema_version (store, version) VALUES ('message', 14);
+	`)
+	require.Nil(t, err)
+	store, err := message.NewPostgresStore(testDB, 0, 0)
+	require.Nil(t, err)
+	// The 14 -> 15 step ran: version bumped, partial index created
+	var version int
+	require.Nil(t, testDB.QueryRow(`SELECT version FROM schema_version WHERE store = 'message'`).Scan(&version))
+	require.Equal(t, 15, version)
+	var indexCount int
+	require.Nil(t, testDB.QueryRow(`SELECT COUNT(*) FROM pg_indexes WHERE indexname = 'idx_message_attachment_expires' AND schemaname = current_schema()`).Scan(&indexCount))
+	require.Equal(t, 1, indexCount)
+	// And the store works
+	require.Nil(t, store.AddMessage(model.NewDefaultMessage("mytopic", "hi there")))
+	messages, err := store.Messages("mytopic", model.SinceAllMessages, false)
+	require.Nil(t, err)
+	require.Len(t, messages, 1)
+
+	// The migrated database must be structurally identical to a freshly created one
+	freshDB := dbtest.CreateTestPostgres(t)
+	_, err = message.NewPostgresStore(freshDB, 0, 0)
+	require.Nil(t, err)
+	require.Equal(t, dbtest.PostgresSchema(t, freshDB), dbtest.PostgresSchema(t, testDB))
+}

message/cache_sqlite_test.go

@@ -9,6 +9,7 @@ import (
 
 	_ "github.com/mattn/go-sqlite3" // SQLite driver
 	"github.com/stretchr/testify/require"
+	dbtest "heckel.io/ntfy/v2/db/test"
 	"heckel.io/ntfy/v2/message"
 	"heckel.io/ntfy/v2/model"
 )
@@ -50,6 +51,19 @@ func TestSqliteStore_Migration_From1(t *testing.T) {
 	s := newSqliteTestStoreFromFile(t, filename, "")
 	checkSqliteSchemaVersion(t, filename)
 
+	// The migrated database must be structurally identical to a freshly created one
+	freshFile := newSqliteTestStoreFile(t)
+	fresh, err := message.NewSQLiteStore(freshFile, "", time.Hour, 0, 0, false)
+	require.Nil(t, err)
+	t.Cleanup(func() { fresh.Close() })
+	freshDB, err := sql.Open("sqlite3", freshFile)
+	require.Nil(t, err)
+	defer freshDB.Close()
+	migratedDB, err := sql.Open("sqlite3", filename)
+	require.Nil(t, err)
+	defer migratedDB.Close()
+	require.Equal(t, dbtest.SQLiteSchema(t, freshDB), dbtest.SQLiteSchema(t, migratedDB))
+
 	// Add delayed message
 	delayedMessage := model.NewDefaultMessage("mytopic", "some delayed message")
 	delayedMessage.Time = time.Now().Add(time.Minute).Unix()

message/cache_test.go

@@ -36,60 +36,6 @@ func newTestPostgresStore(t *testing.T) *message.Cache {
 	return store
 }
 
-func TestPostgresStore_Migration_From14(t *testing.T) {
-	// A pre-framework database at version 14: full v14 schema, version tracked in the
-	// hand-rolled schema_version table, and no idx_message_attachment_expires yet
-	testDB := dbtest.CreateTestPostgres(t)
-	_, err := testDB.Exec(`
-		CREATE TABLE message (
-			id BIGSERIAL PRIMARY KEY,
-			mid TEXT NOT NULL,
-			sequence_id TEXT NOT NULL,
-			time BIGINT NOT NULL,
-			event TEXT NOT NULL,
-			expires BIGINT NOT NULL,
-			topic TEXT NOT NULL,
-			message TEXT NOT NULL,
-			title TEXT NOT NULL,
-			priority INT NOT NULL,
-			tags TEXT NOT NULL,
-			click TEXT NOT NULL,
-			icon TEXT NOT NULL,
-			actions TEXT NOT NULL,
-			attachment_name TEXT NOT NULL,
-			attachment_type TEXT NOT NULL,
-			attachment_size BIGINT NOT NULL,
-			attachment_expires BIGINT NOT NULL,
-			attachment_url TEXT NOT NULL,
-			attachment_deleted BOOLEAN NOT NULL DEFAULT FALSE,
-			sender TEXT NOT NULL,
-			user_id TEXT NOT NULL,
-			content_type TEXT NOT NULL,
-			encoding TEXT NOT NULL,
-			published BOOLEAN NOT NULL DEFAULT FALSE
-		);
-		CREATE TABLE message_stats (key TEXT PRIMARY KEY, value BIGINT);
-		INSERT INTO message_stats (key, value) VALUES ('messages', 0);
-		CREATE TABLE schema_version (store TEXT PRIMARY KEY, version INT NOT NULL);
-		INSERT INTO schema_version (store, version) VALUES ('message', 14);
-	`)
-	require.Nil(t, err)
-	store, err := message.NewPostgresStore(testDB, 0, 0)
-	require.Nil(t, err)
-	// The 14 -> 15 step ran: version bumped, partial index created
-	var version int
-	require.Nil(t, testDB.QueryRow(`SELECT version FROM schema_version WHERE store = 'message'`).Scan(&version))
-	require.Equal(t, 15, version)
-	var indexCount int
-	require.Nil(t, testDB.QueryRow(`SELECT COUNT(*) FROM pg_indexes WHERE indexname = 'idx_message_attachment_expires' AND schemaname = current_schema()`).Scan(&indexCount))
-	require.Equal(t, 1, indexCount)
-	// And the store works
-	require.Nil(t, store.AddMessage(model.NewDefaultMessage("mytopic", "hi there")))
-	messages, err := store.Messages("mytopic", model.SinceAllMessages, false)
-	require.Nil(t, err)
-	require.Len(t, messages, 1)
-}
-
 func forEachBackend(t *testing.T, f func(t *testing.T, s *message.Cache)) {
 	t.Run("sqlite", func(t *testing.T) {
 		f(t, newSqliteTestStore(t))

user/manager_postgres_schema.go

@@ -96,7 +96,7 @@ const (
 )
 
 const (
-	postgresCurrentSchemaVersion = 8
+	postgresCurrentSchemaVersion = 9
 )
 
 const (
@@ -135,5 +135,6 @@ var (
 	postgresMigrations = map[int]schema.MigrateFunc{
 		6: schema.AsMigrateFunc(postgresMigrate6To7UpdateQueries),
 		7: schema.AsMigrateFunc(postgresMigrate7To8UpdateQueries),
+		8: schema.NopMigrateFunc, // 8 -> 9 repairs a SQLite-only foreign key defect; nothing to do on Postgres
 	}
 )

user/manager_sqlite_schema.go

@@ -114,7 +114,7 @@ const (
 )
 
 const (
-	sqliteCurrentSchemaVersion = 8
+	sqliteCurrentSchemaVersion = 9
 )
 
 // Schema migrations for SQLite
@@ -347,6 +347,25 @@ const (
 		CREATE UNIQUE INDEX idx_user_stripe_subscription_id ON user (stripe_subscription_id);
 		CREATE UNIQUE INDEX idx_user_token ON user_token (token);
 	`
+
+	// 8 -> 9: Repair the user_phone foreign key. The 5 -> 6 migration renamed user to
+	// user_old, which rewrote user_phone's REFERENCES clause to user_old -- a table that was
+	// then dropped (the rebuilt tables got correct fresh foreign keys; user_phone was the only
+	// child table not rebuilt). Rebuilding user_phone re-points the foreign key at user; on
+	// healthy databases the rebuild is a harmless no-op schema-wise.
+	sqliteMigrate8To9UpdateQueries = `
+		ALTER TABLE user_phone RENAME TO user_phone_old;
+		CREATE TABLE user_phone (
+			user_id TEXT NOT NULL,
+			phone_number TEXT NOT NULL,
+			PRIMARY KEY (user_id, phone_number),
+			FOREIGN KEY (user_id) REFERENCES user (id) ON DELETE CASCADE
+		);
+		INSERT INTO user_phone (user_id, phone_number)
+		SELECT user_id, phone_number FROM user_phone_old
+		WHERE user_id IN (SELECT id FROM user); -- Drop orphaned rows that the broken foreign key failed to cascade-delete
+		DROP TABLE user_phone_old;
+	`
 )
 
 var (
@@ -362,6 +381,7 @@ var (
 		5: schema.AsMigrateFunc(sqliteMigrate5To6UpdateQueries),
 		6: schema.AsMigrateFunc(sqliteMigrate6To7UpdateQueries),
 		7: schema.AsMigrateFunc(sqliteMigrate7To8UpdateQueries),
+		8: schema.AsMigrateFunc(sqliteMigrate8To9UpdateQueries),
 	}
 )
 

user/manager_test.go

@@ -1567,6 +1567,83 @@ func TestToFromSQLWildcard(t *testing.T) {
 	require.Equal(t, "foo", fromSQLWildcard(toSQLWildcard("foo")))
 }
 
+// testPostgresV6Schema is the PostgreSQL schema exactly as created by the version that first
+// shipped Postgres support (schema version 6), taken from the code at that time; used to
+// verify the migration chain from its oldest supported version.
+const testPostgresV6Schema = `
+	CREATE TABLE IF NOT EXISTS tier (
+		id TEXT PRIMARY KEY,
+		code TEXT NOT NULL,
+		name TEXT NOT NULL,
+		messages_limit BIGINT NOT NULL,
+		messages_expiry_duration BIGINT NOT NULL,
+		emails_limit BIGINT NOT NULL,
+		calls_limit BIGINT NOT NULL,
+		reservations_limit BIGINT NOT NULL,
+		attachment_file_size_limit BIGINT NOT NULL,
+		attachment_total_size_limit BIGINT NOT NULL,
+		attachment_expiry_duration BIGINT NOT NULL,
+		attachment_bandwidth_limit BIGINT NOT NULL,
+		stripe_monthly_price_id TEXT,
+		stripe_yearly_price_id TEXT,
+		UNIQUE(code),
+		UNIQUE(stripe_monthly_price_id),
+		UNIQUE(stripe_yearly_price_id)
+	);
+	CREATE TABLE IF NOT EXISTS "user" (
+	    id TEXT PRIMARY KEY,
+		tier_id TEXT REFERENCES tier(id),
+		user_name TEXT NOT NULL UNIQUE,
+		pass TEXT NOT NULL,
+		role TEXT NOT NULL CHECK (role IN ('anonymous', 'admin', 'user')),
+		prefs JSONB NOT NULL DEFAULT '{}',
+		sync_topic TEXT NOT NULL,
+		provisioned BOOLEAN NOT NULL,
+		stats_messages BIGINT NOT NULL DEFAULT 0,
+		stats_emails BIGINT NOT NULL DEFAULT 0,
+		stats_calls BIGINT NOT NULL DEFAULT 0,
+		stripe_customer_id TEXT UNIQUE,
+		stripe_subscription_id TEXT UNIQUE,
+		stripe_subscription_status TEXT,
+		stripe_subscription_interval TEXT,
+		stripe_subscription_paid_until BIGINT,
+		stripe_subscription_cancel_at BIGINT,
+		created BIGINT NOT NULL,
+		deleted BIGINT
+	);
+	CREATE TABLE IF NOT EXISTS user_access (
+		user_id TEXT NOT NULL REFERENCES "user"(id) ON DELETE CASCADE,
+		topic TEXT NOT NULL,
+		read BOOLEAN NOT NULL,
+		write BOOLEAN NOT NULL,
+		owner_user_id TEXT REFERENCES "user"(id) ON DELETE CASCADE,
+		provisioned BOOLEAN NOT NULL,
+		PRIMARY KEY (user_id, topic)
+	);
+	CREATE TABLE IF NOT EXISTS user_token (
+		user_id TEXT NOT NULL REFERENCES "user"(id) ON DELETE CASCADE,
+		token TEXT NOT NULL UNIQUE,
+		label TEXT NOT NULL,
+		last_access BIGINT NOT NULL,
+		last_origin TEXT NOT NULL,
+		expires BIGINT NOT NULL,
+		provisioned BOOLEAN NOT NULL,
+		PRIMARY KEY (user_id, token)
+	);
+	CREATE TABLE IF NOT EXISTS user_phone (
+		user_id TEXT NOT NULL REFERENCES "user"(id) ON DELETE CASCADE,
+		phone_number TEXT NOT NULL,
+		PRIMARY KEY (user_id, phone_number)
+	);
+	CREATE TABLE IF NOT EXISTS schema_version (
+		store TEXT PRIMARY KEY,
+		version INT NOT NULL
+	);
+	INSERT INTO "user" (id, user_name, pass, role, sync_topic, provisioned, created)
+	VALUES ('u_everyone', '*', '', 'anonymous', '', false, EXTRACT(EPOCH FROM NOW())::BIGINT)
+	ON CONFLICT (id) DO NOTHING;
+`
+
 func TestMigrationFrom1(t *testing.T) {
 	filename := filepath.Join(t.TempDir(), "user.db")
 	db, err := sql.Open("sqlite3", filename)
@@ -1650,6 +1727,8 @@ func TestMigrationFrom1(t *testing.T) {
 	require.Equal(t, 1, len(everyoneGrants))
 	require.Equal(t, "stats", everyoneGrants[0].TopicPattern)
 	require.Equal(t, PermissionRead, everyoneGrants[0].Permission)
+
+	checkMigratedSqliteSchema(t, filename)
 }
 
 func TestMigrationFrom4(t *testing.T) {
@@ -1741,12 +1820,15 @@ func TestMigrationFrom4(t *testing.T) {
 	`)
 	require.Nil(t, err)
 
-	// Insert a few ACL entries
+	// Insert a few ACL entries, and phone numbers: one for a live user, one orphaned (its user
+	// is gone; the broken pre-v9 foreign key never cascade-deleted it)
 	_, err = db.Exec(`
 		BEGIN;
 		INSERT INTO user_access (user_id, topic, read, write) values ('u_everyone', 'mytopic_', 1, 1);
 		INSERT INTO user_access (user_id, topic, read, write) values ('u_everyone', 'up%', 1, 1);
 		INSERT INTO user_access (user_id, topic, read, write) values ('u_everyone', 'down_%', 1, 1);
+		INSERT INTO user_phone (user_id, phone_number) VALUES ('u_everyone', '+12223334444');
+		INSERT INTO user_phone (user_id, phone_number) VALUES ('u_gone', '+15556667777');
 		COMMIT;
 	`)
 	require.Nil(t, err)
@@ -1796,6 +1878,68 @@ func TestMigrationFrom4(t *testing.T) {
 
 	require.Nil(t, a.Authorize(nil, "up123", PermissionRead))
 	require.Nil(t, a.Authorize(nil, "up", PermissionRead)) // % matches 0 or more characters
+
+	// The 8 -> 9 repair kept the live user's phone number and dropped the orphaned row
+	phoneNumbers := make([]string, 0)
+	rows, err = db.Query(`SELECT phone_number FROM user_phone ORDER BY phone_number`)
+	require.Nil(t, err)
+	for rows.Next() {
+		var phoneNumber string
+		require.Nil(t, rows.Scan(&phoneNumber))
+		phoneNumbers = append(phoneNumbers, phoneNumber)
+	}
+	require.Nil(t, rows.Close())
+	require.Equal(t, []string{"+12223334444"}, phoneNumbers)
+
+	checkMigratedSqliteSchema(t, filename)
+}
+
+// TestMigrationFrom6Postgres tests the Postgres migration chain from its oldest supported
+// version (6, the version PostgreSQL support first shipped with).
+func TestMigrationFrom6Postgres(t *testing.T) {
+	testDB := dbtest.CreateTestPostgres(t)
+	_, err := testDB.Exec(testPostgresV6Schema)
+	require.Nil(t, err)
+	_, err = testDB.Exec(`INSERT INTO schema_version (store, version) VALUES ('user', 6)`)
+	require.Nil(t, err)
+	// Create manager to trigger migration
+	a, err := NewPostgresManager(testDB, &Config{DefaultAccess: PermissionDenyAll, BcryptCost: bcrypt.MinCost, QueueWriterInterval: DefaultUserStatsQueueWriterInterval})
+	require.Nil(t, err)
+	var version int
+	require.Nil(t, testDB.QueryRow(`SELECT version FROM schema_version WHERE store = 'user'`).Scan(&version))
+	require.Equal(t, postgresCurrentSchemaVersion, version)
+	// The manager works against the migrated schema
+	require.Nil(t, a.AddUser("phil", "mypass", RoleUser, false))
+	u, err := a.User("phil")
+	require.Nil(t, err)
+	require.Nil(t, a.AddEmail(u.ID, "phil@example.com"))
+	// The migrated database must be structurally identical to a freshly created one
+	freshDB := dbtest.CreateTestPostgres(t)
+	_, err = NewPostgresManager(freshDB, &Config{DefaultAccess: PermissionDenyAll, BcryptCost: bcrypt.MinCost, QueueWriterInterval: DefaultUserStatsQueueWriterInterval})
+	require.Nil(t, err)
+	require.Equal(t, dbtest.PostgresSchema(t, freshDB), dbtest.PostgresSchema(t, testDB))
+}
+
+// checkMigratedSqliteSchema verifies that a migrated database is structurally identical to a
+// freshly created one (this pins, among other things, that the foreign keys of the tables
+// rebuilt in migration 5 -> 6 still point at "user", not at a dropped "user_old"), and that
+// its data passes SQLite's foreign key consistency check.
+func checkMigratedSqliteSchema(t *testing.T, filename string) {
+	t.Helper()
+	freshFile := filepath.Join(t.TempDir(), "fresh.db")
+	fresh := newTestManagerFromFile(t, freshFile, "", PermissionDenyAll, bcrypt.MinCost, DefaultUserStatsQueueWriterInterval)
+	defer fresh.Close()
+	freshDB, err := sql.Open("sqlite3", freshFile)
+	require.Nil(t, err)
+	defer freshDB.Close()
+	migratedDB, err := sql.Open("sqlite3", filename)
+	require.Nil(t, err)
+	defer migratedDB.Close()
+	require.Equal(t, dbtest.SQLiteSchema(t, freshDB), dbtest.SQLiteSchema(t, migratedDB))
+	rows, err := migratedDB.Query(`PRAGMA foreign_key_check`)
+	require.Nil(t, err)
+	defer rows.Close()
+	require.False(t, rows.Next(), "foreign_key_check reported violations in the migrated database")
 }
 
 func checkSchemaVersion(t *testing.T, d *db.DB) {

webpush/store_test.go

@@ -30,11 +30,14 @@ const (
 			warned_at INT NOT NULL DEFAULT 0
 		);
 		CREATE UNIQUE INDEX idx_endpoint ON subscription (endpoint);
+		CREATE INDEX idx_subscriber_ip ON subscription (subscriber_ip);
 		CREATE TABLE subscription_topic (
 			subscription_id TEXT NOT NULL,
 			topic TEXT NOT NULL,
-			PRIMARY KEY (subscription_id, topic)
+			PRIMARY KEY (subscription_id, topic),
+			FOREIGN KEY (subscription_id) REFERENCES subscription (id) ON DELETE CASCADE
 		);
+		CREATE INDEX idx_topic ON subscription_topic (topic);
 		CREATE TABLE schemaVersion (id INT PRIMARY KEY, version INT NOT NULL);
 		INSERT INTO schemaVersion VALUES (1, 1);
 	`
@@ -49,16 +52,58 @@ const (
 			updated_at BIGINT NOT NULL,
 			warned_at BIGINT NOT NULL DEFAULT 0
 		);
+		CREATE INDEX idx_webpush_subscriber_ip ON webpush_subscription (subscriber_ip);
+		CREATE INDEX idx_webpush_updated_at ON webpush_subscription (updated_at);
+		CREATE INDEX idx_webpush_user_id ON webpush_subscription (user_id);
 		CREATE TABLE webpush_subscription_topic (
 			subscription_id TEXT NOT NULL REFERENCES webpush_subscription (id) ON DELETE CASCADE,
 			topic TEXT NOT NULL,
 			PRIMARY KEY (subscription_id, topic)
 		);
+		CREATE INDEX idx_webpush_topic ON webpush_subscription_topic (topic);
 		CREATE TABLE schema_version (store TEXT PRIMARY KEY, version INT NOT NULL);
 		INSERT INTO schema_version (store, version) VALUES ('webpush', 1);
 	`
 )
 
+// TestStoreSchemaEquivalence verifies that a database adopted from the pre-framework layout is
+// structurally identical to a freshly created one: same tables, columns, indexes and keys.
+func TestStoreSchemaEquivalence(t *testing.T) {
+	t.Run("sqlite", func(t *testing.T) {
+		freshFile := filepath.Join(t.TempDir(), "fresh.db")
+		fresh, err := webpush.NewSQLiteStore(freshFile, "")
+		require.Nil(t, err)
+		defer fresh.Close()
+		migratedFile := filepath.Join(t.TempDir(), "migrated.db")
+		d, err := sql.Open("sqlite3", migratedFile)
+		require.Nil(t, err)
+		_, err = d.Exec(testPreFrameworkSQLiteSchema)
+		require.Nil(t, err)
+		require.Nil(t, d.Close())
+		migrated, err := webpush.NewSQLiteStore(migratedFile, "")
+		require.Nil(t, err)
+		defer migrated.Close()
+		freshDB, err := sql.Open("sqlite3", freshFile)
+		require.Nil(t, err)
+		defer freshDB.Close()
+		migratedDB, err := sql.Open("sqlite3", migratedFile)
+		require.Nil(t, err)
+		defer migratedDB.Close()
+		require.Equal(t, dbtest.SQLiteSchema(t, freshDB), dbtest.SQLiteSchema(t, migratedDB))
+	})
+	t.Run("postgres", func(t *testing.T) {
+		freshDB := dbtest.CreateTestPostgres(t)
+		_, err := webpush.NewPostgresStore(freshDB)
+		require.Nil(t, err)
+		migratedDB := dbtest.CreateTestPostgres(t)
+		_, err = migratedDB.Exec(testPreFrameworkPostgresSchema)
+		require.Nil(t, err)
+		_, err = webpush.NewPostgresStore(migratedDB)
+		require.Nil(t, err)
+		require.Equal(t, dbtest.PostgresSchema(t, freshDB), dbtest.PostgresSchema(t, migratedDB))
+	})
+}
+
 func TestStoreSQLiteOpensExistingDatabase(t *testing.T) {
 	filename := filepath.Join(t.TempDir(), "webpush.db")
 	d, err := sql.Open("sqlite3", filename)