From 115b3fa8116d8b3eb4b874940fdd5c9237c72925 Mon Sep 17 00:00:00 2001 From: Brandur Date: Thu, 11 Jun 2026 15:04:26 -0500 Subject: [PATCH] Fixes for SQLite version 007 migration I tried running the River test suite on an alternate River repo I had, and ran into this failure: --- FAIL: Example_libSQL (0.01s) panic: error applying version 007 [UP]: SQL logic error: Cannot add a column with non-constant default (1) [recovered, repanicked] goroutine 1 [running]: testing.(*InternalExample).processRunResult(0x27a61a107bd8, {0x0, 0x0}, 0x1009add28?, 0x0, {0x1018f4c80, 0x27a61a8d7960}) /opt/homebrew/Cellar/go/1.26.1/libexec/src/testing/example.go:92 +0x4b0 testing.runExample.func2() /opt/homebrew/Cellar/go/1.26.1/libexec/src/testing/run_example.go:59 +0xdc panic({0x1018f4c80?, 0x27a61a8d7960?}) /opt/homebrew/Cellar/go/1.26.1/libexec/src/runtime/panic.go:860 +0x12c github.com/riverqueue/river/riverdriver/riverdrivertest_test.Example_libSQL() /Users/brandur/Documents/projects/alt_river/riverdriver/riverdrivertest/example_libsql_test.go:30 +0x368 testing.runExample({{0x1011457d5, 0xe}, 0x1019fdb28, {0x101158290, 0x23}, 0x0}) /opt/homebrew/Cellar/go/1.26.1/libexec/src/testing/run_example.go:63 +0x1f8 testing.runExamples(0x101a16038?, {0x101acd420, 0x2, 0x37?}) /opt/homebrew/Cellar/go/1.26.1/libexec/src/testing/example.go:41 +0xec testing.(*M).Run(0x27a618bbba40) /opt/homebrew/Cellar/go/1.26.1/libexec/src/testing/testing.go:2445 +0x60c main.main() _testmain.go:80 +0x80 FAIL github.com/riverqueue/river/riverdriver/riverdrivertest 2.364s FAIL It turns out that it was surfacing a problem in the new SQLite version 007 migration that only manifests when existing `river_queue` data was present. It was balking at adding a new column in an `ALTER TABLE` with a non-constant default value like `CURRENT_TIMESTAMP`. This turns out to be a relatively easy fix because elsewhere in the migration we were already rewriting that able anyway (usually in SQLite you have to rewrite tables on DDL changes, but there's a few tricks under some circumstances you can use not to), so all we have to do is move the `DEFAULT` up to that rewrite statement. I asked Codex if it could find any similar bugs in the migration, and it identified one more similar one that could happen if you migration down from 007 to 006 and have `river_job` data present. Similarly, we wrap this one up into an already existing table rewrite in the migration. Added a couple test cases to verify these changes work as expected. --- riverdriver/riverdrivertest/migration.go | 72 +++++++++++++++++++ ...tbox_sqlite_jsonb_and_sql_cleanup.down.sql | 35 +-------- ...outbox_sqlite_jsonb_and_sql_cleanup.up.sql | 18 +---- 3 files changed, 75 insertions(+), 50 deletions(-) diff --git a/riverdriver/riverdrivertest/migration.go b/riverdriver/riverdrivertest/migration.go index 1a1c17c1..0bbd5ae5 100644 --- a/riverdriver/riverdrivertest/migration.go +++ b/riverdriver/riverdrivertest/migration.go @@ -2,6 +2,7 @@ package riverdrivertest import ( "context" + "fmt" "slices" "strings" "testing" @@ -144,6 +145,77 @@ func exerciseMigration[TTx any](ctx context.Context, t *testing.T, } }) + t.Run("MigrateDownFromVersionSevenWithJobData", func(t *testing.T) { + t.Parallel() + + driver, schema := driverWithSchema(ctx, t, &riverdbtest.TestSchemaOpts{ + DisableReuse: true, + }) + exec := driver.GetExecutor() + + job := testfactory.Job(ctx, t, exec, &testfactory.JobOpts{Schema: schema}) + + migrator, err := rivermigrate.New(driver, &rivermigrate.Config{ + Line: riverdriver.MigrationLineMain, + Logger: riversharedtest.Logger(t), + Schema: schema, + }) + require.NoError(t, err) + + _, err = migrator.Migrate(ctx, rivermigrate.DirectionDown, &rivermigrate.MigrateOpts{ + TargetVersion: 6, + }) + require.NoError(t, err) + + job, err = exec.JobGetByID(ctx, &riverdriver.JobGetByIDParams{ID: job.ID, Schema: schema}) + require.NoError(t, err) + require.NotZero(t, job.ID) + }) + + t.Run("MigrateUpFromVersionSixWithQueueData", func(t *testing.T) { + t.Parallel() + + driver, schema := driverWithSchema(ctx, t, &riverdbtest.TestSchemaOpts{ + DisableReuse: true, + LineTargetVersions: map[string]int{ + riverdriver.MigrationLineMain: 6, + }, + }) + exec := driver.GetExecutor() + + queueTable := "river_queue" + if driver.DatabaseName() != riverdriver.DatabaseNameSQLite { + queueTable = schema + ".river_queue" + } + + _ = testfactory.Queue(ctx, t, exec, &testfactory.QueueOpts{ + Name: ptrutil.Ptr("default"), + Schema: schema, + }) + + migrator, err := rivermigrate.New(driver, &rivermigrate.Config{ + Line: riverdriver.MigrationLineMain, + Logger: riversharedtest.Logger(t), + Schema: schema, + }) + require.NoError(t, err) + + _, err = migrator.Migrate(ctx, rivermigrate.DirectionUp, nil) + require.NoError(t, err) + + queue, err := exec.QueueGet(ctx, &riverdriver.QueueGetParams{Schema: schema, Name: "default"}) + require.NoError(t, err) + require.Equal(t, "default", queue.Name) + require.NotZero(t, queue.UpdatedAt) + + require.NoError(t, exec.Exec(ctx, fmt.Sprintf("INSERT INTO %s (name) VALUES ('queue-with-defaults')", queueTable))) + + queue, err = exec.QueueGet(ctx, &riverdriver.QueueGetParams{Schema: schema, Name: "queue-with-defaults"}) + require.NoError(t, err) + require.Equal(t, "queue-with-defaults", queue.Name) + require.NotZero(t, queue.UpdatedAt) + }) + type testBundle struct { driver riverdriver.Driver[TTx] } diff --git a/riverdriver/riversqlite/migration/main/007_notification_outbox_sqlite_jsonb_and_sql_cleanup.down.sql b/riverdriver/riversqlite/migration/main/007_notification_outbox_sqlite_jsonb_and_sql_cleanup.down.sql index e36cf848..24944ad8 100644 --- a/riverdriver/riversqlite/migration/main/007_notification_outbox_sqlite_jsonb_and_sql_cleanup.down.sql +++ b/riverdriver/riversqlite/migration/main/007_notification_outbox_sqlite_jsonb_and_sql_cleanup.down.sql @@ -30,43 +30,12 @@ CREATE TABLE /* TEMPLATE: schema */river_client_queue ( CONSTRAINT num_jobs_running_zero_or_positive CHECK (num_jobs_running >= 0) ); --- --- Revert addition of `DEFAULT 25` to `river_job.max_attempts`. --- - -ALTER TABLE /* TEMPLATE: schema */river_job - RENAME COLUMN max_attempts TO max_attempts_old; - -ALTER TABLE /* TEMPLATE: schema */river_job - ADD COLUMN max_attempts integer NOT NULL; - -UPDATE /* TEMPLATE: schema */river_job -SET max_attempts = max_attempts_old; - -ALTER TABLE /* TEMPLATE: schema */river_job - DROP COLUMN max_attempts_old; - --- --- Changes `river_queue.updated_at` to revert the default of `CURRENT_TIMESTAMP`. --- - -ALTER TABLE /* TEMPLATE: schema */river_queue - RENAME COLUMN updated_at TO updated_at_old; - -ALTER TABLE /* TEMPLATE: schema */river_queue - ADD COLUMN updated_at timestamp NOT NULL; - -UPDATE /* TEMPLATE: schema */river_queue -SET updated_at = updated_at_old; - -ALTER TABLE /* TEMPLATE: schema */river_queue - DROP COLUMN updated_at_old; - -- -- SQLite JSONB conversion rollback. -- -- Convert JSONB binary columns back to JSON text format and restore json() --- defaults. +-- defaults. The `river_job` rebuild also reverts the addition of `DEFAULT 25` +-- to `river_job.max_attempts`. -- -- SQLite doesn't allow `ALTER TABLE ADD COLUMN` with non-constant defaults like -- `json('{}')`, so rebuild each affected table instead. diff --git a/riverdriver/riversqlite/migration/main/007_notification_outbox_sqlite_jsonb_and_sql_cleanup.up.sql b/riverdriver/riversqlite/migration/main/007_notification_outbox_sqlite_jsonb_and_sql_cleanup.up.sql index aad44165..511aedeb 100644 --- a/riverdriver/riversqlite/migration/main/007_notification_outbox_sqlite_jsonb_and_sql_cleanup.up.sql +++ b/riverdriver/riversqlite/migration/main/007_notification_outbox_sqlite_jsonb_and_sql_cleanup.up.sql @@ -135,7 +135,7 @@ CREATE TABLE /* TEMPLATE: schema */river_queue ( created_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, metadata blob NOT NULL DEFAULT (jsonb('{}')), paused_at timestamp, - updated_at timestamp NOT NULL + updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ); INSERT INTO /* TEMPLATE: schema */river_queue ( @@ -259,19 +259,3 @@ SET max_attempts = max_attempts_old; ALTER TABLE /* TEMPLATE: schema */river_job DROP COLUMN max_attempts_old; - --- --- Changes `river_queue.updated_at` to have a default of `CURRENT_TIMESTAMP`. --- - -ALTER TABLE /* TEMPLATE: schema */river_queue - RENAME COLUMN updated_at TO updated_at_old; - -ALTER TABLE /* TEMPLATE: schema */river_queue - ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; - -UPDATE /* TEMPLATE: schema */river_queue -SET updated_at = updated_at_old; - -ALTER TABLE /* TEMPLATE: schema */river_queue - DROP COLUMN updated_at_old;