Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tapdb/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const (
// daemon.
//
// NOTE: This MUST be updated when a new migration is added.
LatestMigrationVersion = 54
LatestMigrationVersion = 55
)

// DatabaseBackend is an interface that contains all methods our different
Expand Down
120 changes: 120 additions & 0 deletions tapdb/migrations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1279,3 +1279,123 @@
"delete should fail after reverting CASCADE",
)
}

// TestSequenceConsistency is a Postgres-only test that verifies every
// auto-increment sequence is consistent with its table's max ID after
// all migrations have run. This catches migrations that copy rows with
// explicit IDs (e.g. INSERT INTO new_t (id, ...) SELECT id, ... FROM
// old_t) without advancing the sequence.
func TestSequenceConsistency(t *testing.T) {
ctx := context.Background()

db := NewTestDBWithVersion(t, 30)

if db.Backend() != sqlc.BackendTypePostgres {
t.Skip("sequence consistency only applies to Postgres")
}

// Seed a universe_leaves row with an explicit id = 100.
// Migration 31 will copy this row (with its explicit ID)
// into a recreated table, which won't advance the sequence.
InsertTestdata(
t, db.BaseDB,
"migrations_test_00031_dummy_data.sql",
)

// Reset every sequence to match its table's current max so
// that only desyncs introduced by migrations are detected.
_, err := db.ExecContext(ctx, `
DO $$
DECLARE
r RECORD;
BEGIN
FOR r IN
SELECT
seq.relname AS seq_name,
tbl.relname AS tbl_name,
a.attname AS col_name
FROM pg_class seq
JOIN pg_depend d
ON d.objid = seq.oid
JOIN pg_class tbl
ON d.refobjid = tbl.oid
JOIN pg_attribute a
ON a.attrelid = tbl.oid
AND a.attnum = d.refobjsubid
WHERE seq.relkind = 'S'
AND d.deptype = 'a'
LOOP
EXECUTE format(
'SELECT setval(%L, COALESCE('
|| '(SELECT MAX(%I) FROM %I),'
|| ' 1))',
r.seq_name, r.col_name,
r.tbl_name
);
END LOOP;
END
$$;
`)
require.NoError(t, err)

// Run all remaining migrations.
err = db.ExecuteMigrations(TargetLatest)
require.NoError(t, err)

// Find all sequences owned by table columns.
rows, err := db.QueryContext(ctx, `
SELECT
seq.relname AS sequence_name,
tbl.relname AS table_name,
a.attname AS column_name
FROM pg_class seq
JOIN pg_depend d ON d.objid = seq.oid
JOIN pg_class tbl ON d.refobjid = tbl.oid
JOIN pg_attribute a ON a.attrelid = tbl.oid
AND a.attnum = d.refobjsubid
WHERE seq.relkind = 'S'
AND d.deptype = 'a'
`)
require.NoError(t, err)

defer rows.Close()

type seqInfo struct {
seqName string

Check failure on line 1364 in tapdb/migrations_test.go

View workflow job for this annotation

GitHub Actions / Lint check

File is not properly formatted (gofmt)
table string
column string
}

var sequences []seqInfo
for rows.Next() {
var s seqInfo
err := rows.Scan(&s.seqName, &s.table, &s.column)
require.NoError(t, err)
sequences = append(sequences, s)
}
require.NoError(t, rows.Err())
require.NotEmpty(t, sequences, "expected at least one sequence")

for _, s := range sequences {
var lastValue int64
err := db.QueryRowContext(ctx, fmt.Sprintf(
"SELECT last_value FROM %s", s.seqName,
)).Scan(&lastValue)
require.NoError(t, err)

var maxID int64
err = db.QueryRowContext(ctx, fmt.Sprintf(
"SELECT COALESCE(MAX(%s), 0) FROM %s",
s.column, s.table,
)).Scan(&maxID)
require.NoError(t, err)

require.GreaterOrEqual(
t, lastValue, maxID,
"sequence %s on %s.%s is behind: "+
"last_value=%d, max(%s)=%d",
s.seqName, s.table, s.column,
lastValue, s.column, maxID,
)
Comment on lines +1381 to +1399
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The check lastValue >= maxID is not sufficient to guarantee that the next auto-incremented ID will not collide with existing data. In PostgreSQL, if last_value is equal to maxID but the is_called flag is false, the next call to nextval() will return maxID, causing a primary key violation. A more robust check would involve verifying that the effective next value (accounting for is_called) is strictly greater than maxID. Additionally, when constructing SQL queries with identifiers dynamically, it is safer to use quoted identifiers (e.g., via %q in fmt.Sprintf) to avoid issues with reserved words or case sensitivity.

}
}
2 changes: 2 additions & 0 deletions tapdb/sqlc/migrations/000055_fix_universe_leaves_seq.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- Sequence reset is forward-only; no rollback needed.
SELECT 1;
11 changes: 11 additions & 0 deletions tapdb/sqlc/migrations/000055_fix_universe_leaves_seq.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-- Reset BIGSERIAL sequences that were left behind by migrations
-- that inserted rows with explicit id values.
--
-- Migration 31 copied universe_leaves rows with explicit ids.
-- Migration 40 inserted supply_commit_states (0-6) and
-- supply_commit_update_types (0-2) with explicit ids.
--
-- On SQLite these are no-ops (replaced via sqliteSchemaReplacements).
SELECT setval(pg_get_serial_sequence('universe_leaves', 'id'), COALESCE((SELECT MAX(id) FROM universe_leaves), 0));
SELECT setval(pg_get_serial_sequence('supply_commit_states', 'id'), COALESCE((SELECT MAX(id) FROM supply_commit_states), 0));
SELECT setval(pg_get_serial_sequence('supply_commit_update_types', 'id'), COALESCE((SELECT MAX(id) FROM supply_commit_update_types), 0));
Comment on lines +9 to +11
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In PostgreSQL, setval will throw an error if the value provided is less than the sequence's MINVALUE (which is 1 by default for BIGSERIAL columns). Using COALESCE(..., 0) will cause this migration to fail on any database where the universe_leaves table is empty (e.g., a fresh installation or a node that hasn't used the Universe feature). To safely handle empty tables and ensure the next value is 1, you should use the three-argument form of setval with a fallback of 1 and an is_called flag based on whether any rows exist.

Suggested change
SELECT setval(pg_get_serial_sequence('universe_leaves', 'id'), COALESCE((SELECT MAX(id) FROM universe_leaves), 0));
SELECT setval(pg_get_serial_sequence('supply_commit_states', 'id'), COALESCE((SELECT MAX(id) FROM supply_commit_states), 0));
SELECT setval(pg_get_serial_sequence('supply_commit_update_types', 'id'), COALESCE((SELECT MAX(id) FROM supply_commit_update_types), 0));
SELECT setval(pg_get_serial_sequence('universe_leaves', 'id'), COALESCE((SELECT MAX(id) FROM universe_leaves), 1), (SELECT MAX(id) FROM universe_leaves) IS NOT NULL);
SELECT setval(pg_get_serial_sequence('supply_commit_states', 'id'), COALESCE((SELECT MAX(id) FROM supply_commit_states), 1), (SELECT MAX(id) FROM supply_commit_states) IS NOT NULL);
SELECT setval(pg_get_serial_sequence('supply_commit_update_types', 'id'), COALESCE((SELECT MAX(id) FROM supply_commit_update_types), 1), (SELECT MAX(id) FROM supply_commit_update_types) IS NOT NULL);

18 changes: 15 additions & 3 deletions tapdb/sqlite.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,21 @@ const (

var (
// sqliteSchemaReplacements is a map of schema strings that need to be
// replaced for sqlite. There currently aren't any replacements, because
// the SQL files are written with SQLite compatibility in mind.
sqliteSchemaReplacements = map[string]string{}
// replaced for sqlite.
sqliteSchemaReplacements = map[string]string{
"SELECT setval(pg_get_serial_sequence(" +
"'universe_leaves', 'id'), COALESCE((" +
"SELECT MAX(id) FROM universe_leaves" +
"), 0));": "SELECT 1;",
"SELECT setval(pg_get_serial_sequence(" +
"'supply_commit_states', 'id'), " +
"COALESCE((SELECT MAX(id) FROM " +
"supply_commit_states), 0));": "SELECT 1;",
"SELECT setval(pg_get_serial_sequence(" +
"'supply_commit_update_types', 'id'), " +
"COALESCE((SELECT MAX(id) FROM " +
"supply_commit_update_types), 0));": "SELECT 1;",
}
Comment on lines +44 to +56
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The strings in sqliteSchemaReplacements must match the SQL in the migration files exactly for the replacement to work. If the SQL in 000055_fix_universe_leaves_seq.up.sql is updated to fix the setval(..., 0) bug, these keys must be updated accordingly. Failure to do so will cause the replacement to fail, and SQLite will attempt to execute the Postgres-specific SQL, leading to migration errors on SQLite backends.

)

// SqliteConfig holds all the config arguments needed to interact with our
Expand Down
Loading