Add a migrate command, and own the Postgres system-database migrations - #13
Open
devhawk wants to merge 16 commits into
Open
Add a migrate command, and own the Postgres system-database migrations#13devhawk wants to merge 16 commits into
devhawk wants to merge 16 commits into
Conversation
dbosctl needs to create and upgrade a DBOS system database without linking a language SDK to do it. Copy the Postgres migration set and its builder from dbos-transact-golang (edb0f01) into internal/migrations rather than importing the SDK: the SDK's only entry point to this code is a client constructor that also opens pools and starts background work, and its release train is not ours. The SQL files are verbatim; the only edit to the builder is the embed path prefix. The SQLite set is deliberately not copied — dbosctl migrates servers. `make migrations` re-vendors the SQL from a local transact checkout and runs the fidelity tests, which fail if a re-vendored file changed its placeholder count or if upstream added a migration the hand-maintained builder does not render.
Ported from the same upstream package as the migration set, with three deliberate differences: - progress goes to an io.Writer rather than an *slog.Logger, because dbosctl has no logger and writes human-readable progress to stderr; - no retry wrapper, because the SDK retries to keep a transient database error from failing an application deploy, and a CLI can just be run again; - an Apply entry point that owns the whole sequence — create the database if missing, detect CockroachDB, check for pending work, migrate — since that ordering is load-bearing and was previously spread through the SDK's client constructor. Online migrations still run outside a transaction (CREATE INDEX CONCURRENTLY requires it) and still sweep up indexes left INVALID by a crashed earlier run.
Two print paths the migrate command needs, kept apart from the runner because neither opens a connection: Statements renders the migration script an operator can review or hand to a DBA, and GrantQueries renders the privileges an application role needs on the DBOS schema. Statements is byte-identical to what the Go SDK's `dbos migrate --print-migrations` emits, down to the unquoted bookkeeping table name, so the two tools' scripts diff clean against each other. Grant applies those privileges, separately from Apply: a database whose owner also runs the application needs none.
`dbosctl migrate` creates or upgrades a DBOS system database, with the flags the Go SDK's `dbos migrate` has: -D/--db-url, --schema, -r/--app-role, --print-migrations, and --print-user-role. The printed scripts are byte-identical to that command's, so the two can be diffed against each other. This is the first dbosctl command that opens a database rather than talking to Conductor, so it takes a URL (flag, else $DBOS_SYSTEM_DATABASE_URL) instead of a profile. Unlike the SDK's version it does not read dbos-config.yaml: dbosctl's own config is a set of Conductor profiles, and running the shell commands listed under database.migrate is an application-development concern, not an operations one. A SQLite URL is refused by name rather than mis-migrated, since only the Postgres set is vendored. Progress goes to stderr with the password masked, so stdout stays pure SQL in the print modes.
Four container-backed tests, under the existing integration tag: - a first run creates the database, schema, and tables and records the latest version, and a second run reports an up-to-date database without moving it; - --schema migrates a second schema without touching the default one; - --app-role leaves a role that can actually SELECT/INSERT/UPDATE/DELETE the system tables, which is the whole point of the flag; - a DBOS SDK connects to a dbosctl-migrated database and leaves it alone. The last one is the claim vendoring rests on. The SDK pinned in go.mod knows fewer migrations than are vendored here, so it also covers the skew that will be normal in practice: an application older than the tool that provisioned its database.
Its own README section rather than a table row: it is the one command that takes a database URL instead of a profile, and the print modes are the reason an operator whose DDL goes through review would reach for it at all.
The vendored set was taken from an untagged main commit; v1.2.0 has since shipped, and every vendored file — the 61 SQL migrations, the builder, the runner, and the statement renderer — is byte-identical in it. Naming a release rather than a commit gives the next person re-vendoring something they can check out by tag. `make migrations` against a v1.2.0 checkout is a no-op, which is what says this is a provenance change and not a content one.
The pin was v1.0.0-rc.1, whose migration set stops well short of the vendored one, so the compatibility test was only ever exercising an SDK older than the tool that provisioned its database. v1.2.0 ships the same set that is vendored here, which makes the test say the thing worth saying: two copies of one migration set do not fight over a database. Test-only dependency; the binary links none of it.
login talks to the OIDC issuer, logout and config touch only local files. What sets migrate apart is not that it skips Conductor but that it opens a database at all, which is why it takes a URL instead of a profile.
internal/migrations is the master copy of the Postgres system-database migrations from here on: new ones are written in this repo and the SDKs follow. Drop `make migrations`, which copied them out of a transact checkout, and with it the framing that this package is downstream of one. What replaces the re-vendoring instructions is instructions for adding a migration — the three edits it takes, when a migration has to be marked Online, and the fact that a migration numbered 100 or above is a cross-SDK change that cannot land here alone. The tests that used to guard against re-vendoring drift now guard against a half-finished addition; they check the same things.
CockroachDB was standing in for two different questions. Whether the server takes CockroachDB's dialect is one; whether the deployment can use LISTEN/NOTIFY is another, and PostgreSQL behind a connection pooler in transaction mode answers no to the second while answering no to the first as well. Nothing can detect a pooler, so --no-listen-notify asks. BuildMigrations now takes both switches. Live migration combines them — CockroachDB forces LISTEN/NOTIFY off whatever the flag says, since those migrations cannot be applied there at all — and print mode has only the flag, because it never connects. A script generated without the triggers says so in its header, since nothing downstream could tell the two apart. Migration 20 had to be split to make this work: its last two statements pin search_path on the trigger functions from migration 1's LISTEN/NOTIFY block, and without that block those functions do not exist for ALTER FUNCTION to find. Migrations 43 and 44 now run everywhere. Every statement in them is an IF EXISTS no-op where the trigger was never created, and gating them left a hole: a process migrating without LISTEN/NOTIFY would skip the drops while advancing the version past them, stranding the triggers forever on that database — still notifying inside every write transaction, which is the cost those migrations exist to remove. This is the model the Java SDK already implements, down to the same reasoning about the drops; the two sets stay in agreement. Verified against CockroachDB v25.2 in a container as well as the Postgres integration tests: all 55 migrations apply, 43 and 44 included, and no notification functions or triggers are left behind.
Migrations 43 and 44 went unconditional on the reasoning that DROP TRIGGER IF EXISTS is a no-op wherever the trigger was never created. That holds on PostgreSQL and not on CockroachDB: v24.1, the oldest release DBOS supports, has no DROP TRIGGER at all, and v24.3 answers "DROP TRIGGER is only implemented in the declarative schema changer". Both were checked in a container. So the dialect gates them and the LISTEN/NOTIFY flag does not. The two look like the same condition and are not: on PostgreSQL the statements parse whatever the flag says, and gating them there would strand the triggers on a database migrated with them and later migrated by a process passing --no-listen-notify. On CockroachDB there is nothing to drop in the first place. The new tier makes that a tested claim rather than a comment. It is opt-in locally (DBOS_TEST_COCKROACH) because the image is large, and always on in CI, which is where the cost belongs. The image is pinned to a minor stream for the same reason CONDUCTOR_IMAGE can be: a CockroachDB release should not turn a PR red on its own.
Print mode connects to nothing, so it cannot detect the engine the way a live migration does, and until now it rendered PostgreSQL unconditionally. A CockroachDB shop that reviews DDL before applying it therefore had no usable path: the script it got would fail partway through and leave a half-migrated database. The flag feeds the dialect switch BuildMigrations already takes, so it covers every migration that differs and not only the ones anybody remembers — no ALTER FUNCTION ... SET search_path (20, 38, 105), migration 28's DROP INDEX rather than DROP CONSTRAINT, no DROP TRIGGER (43, 44), no CONCURRENTLY. It also settles the other switch: CockroachDB has no LISTEN/NOTIFY on any version, so asking for the dialect turns the triggers off rather than conflicting with them. The header says which engine the script is for, because nothing downstream can tell by reading the SQL. On a live migration the flag is a usage error: the server is asked directly, and its answer beats anything a flag could claim.
Migrations 36, 40 and 41 each add a column and build a partial index whose predicate names it. CockroachDB before v25 will not index a column that became visible in the same transaction, and every DBOS SDK applies a migration as one transaction, so none of them can migrate such a database — in any language. No CI notices, because all four test CockroachDB on a current release. The detail lives in migration 36: the error, the versions it was measured on, and the three arrangements that all fail, since "send the statements separately" is the obvious fix and is not one — two queries inside a transaction fail the same way as one multi-statement query. It also records which migrations look like they share the shape and do not, so the next reader does not re-derive it: 4, 8 and 12 build full indexes, and 16's predicate names a column that already existed. 40 and 41 point at 36 rather than repeat it. This documents the issue; it does not fix it. The comments travel into --print-migrations output, which puts the warning in front of the operator most likely to hit it.
PostgreSQL and CockroachDB are both supported system databases, and the migration set differs between them in a dozen places — no CONCURRENTLY, a different migration 28, no notification triggers, no ALTER FUNCTION, no DROP TRIGGER. Running one suite against one engine and a smaller one against the other left most of those differences unexercised on the engine that has them. So the suite now takes its engine from DBOS_TEST_ENGINE and CI matrixes over both: every test that is not about a single engine runs twice, and a failure names the engine in the checks list. Two tests stay engine-specific and skip elsewhere, since their subject only exists on one side. The tier is its own job rather than a matrix over the integration job, which would stand conductor up twice to learn nothing. It also needs no license key, so unlike the conductor tier it still gates a fork PR. Unset means skip, not a default engine: these tests pull a database image and the CockroachDB leg takes minutes, so running them is a choice that `make test-migrations ENGINE=...` makes explicitly. The role in the grants test loses its password — nothing logs in as it, and CockroachDB's insecure mode, which is how the container runs, refuses to set one.
devhawk
commented
Aug 21, 2026
| // skip the drops while advancing the version past them. No later process | ||
| // would retry, so the triggers would survive forever, still notifying inside | ||
| // every write transaction — the cost these migrations exist to remove. | ||
| migration43SQLProcessed, migration44SQLProcessed := "", "" |
Collaborator
Author
There was a problem hiding this comment.
Note, if we decide to drop support for CRDB v24 (see migration 36 for details), we could safely run migration 43 and 44 on CRDB. They drop event and stream trigger that CRDB never gets. But it would simplify the code to always execute those migrations, regardless of dialect
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dbosctl migratecreates or upgrades a DBOS system database. It is the firstcommand here that opens a database rather than calling Conductor, so it takes a
URL —
-D/--db-url, else$DBOS_SYSTEM_DATABASE_URL— instead of a profile.The flags match the Go SDK's
dbos migrate:--schema,-r/--app-role,--print-migrations,--print-user-role. The printed scripts were byte-identicalto that command's when this branch started, so the two can be diffed against
each other.
The migrations live here now
internal/migrationsstarted as a copy of the Go SDK's set and is now the mastercopy: new migrations are written here and the SDKs follow. Nothing re-vendors it.
That is what lets an operator provision a system database without picking an SDK
and installing its toolchain — the schema is shared by all four.
Copying rather than importing keeps dbosctl's release train its own, and avoids
linking a Go SDK whose only entry point to this code is a client constructor that
also opens pools and starts background work. The cost is drift, which the fidelity
tests are for: they fail if a migration file's placeholder count stops matching
its arguments, or if a file is added without being rendered.
SQLite is deliberately absent. dbosctl migrates servers; a SQLite system database
is migrated by the application process that opens it, and a SQLite URL is refused
by name rather than mis-migrated.
Two switches, not one
CockroachDB was standing in for two different questions:
--cockroachfor printmode, which connects to nothing. It reaches further than the triggers: no
ALTER FUNCTION … SET search_path(migrations 20, 38, 105), migration 28'sDROP INDEXrather thanDROP CONSTRAINT, noDROP TRIGGER(43, 44), noCONCURRENTLY.--no-listen-notify, because PostgreSQLbehind a transaction-mode pooler answers no and nothing can detect that.
CockroachDB forces it off regardless, since those migrations cannot be applied
there at all.
Migrations 43 and 44 are gated on the dialect and deliberately not on the
flag. On PostgreSQL those statements parse either way, and gating them there
would strand the triggers on a database migrated with them and later migrated by
a process passing
--no-listen-notify. This is the model the Java SDK alreadyimplements, down to the same reasoning.
Testing
A
migrationsCI job matrixed over[postgres, cockroach], so every test thatis not about one engine runs against both and a failure names the engine. It
needs no license key, so unlike the conductor tier it gates fork PRs too.
Locally green on both legs, plus
make lint,make test, andmake test-integration.The compatibility test is the one this all rests on: a database dbosctl migrated
is one a DBOS SDK connects to and leaves alone — it finds nothing pending and does
not touch the version row.
Known issue, documented not fixed
Migrations 36, 40 and 41 each add a column and build a partial index whose
predicate names it. CockroachDB before v25 will not index a column that became
visible in the same transaction, and every DBOS SDK applies a migration as one
transaction — so no SDK can migrate CockroachDB v24.x, in any language, and no
CI catches it because all four test a current release. Measured on v24.1.32 and
v24.3.35; v25.2 and later are fine.
Migration 36 carries the detail: the error, the versions, and the three
arrangements that all fail (sending the statements separately does not help — the
transaction is the constraint). 40 and 41 point at it.