Skip to content

ateapi: add versioned PostgreSQL schema migrations - #1196

Merged
Julian Gutierrez Oschmann (juli4n) merged 11 commits into
agent-substrate:mainfrom
iplay88keys:iplay88keys/postgresql-schema-migrations-poc
Sep 2, 2026
Merged

ateapi: add versioned PostgreSQL schema migrations#1196
Julian Gutierrez Oschmann (juli4n) merged 11 commits into
agent-substrate:mainfrom
iplay88keys:iplay88keys/postgresql-schema-migrations-poc

Conversation

@iplay88keys

@iplay88keys Jeremy Alvis (iplay88keys) commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Warning

Recreate PostgreSQL databases from earlier development builds.

Summary

This change replaces startup schema setup with embedded, versioned SQL migrations.

ateapi uses Goose to apply migrations before readiness. Goose stores one ledger record for each applied migration.

Goose runs each migration and inserts its ledger record in one PostgreSQL transaction.

Closes #901.

Based on this design: https://docs.google.com/document/d/13ixDKRoAIFXeLxS-_1nikNcAy76ca8m0eVgobfib93E/edit?usp=sharing

Migration behavior

ateapi gets a session advisory lock for the configured schema before it applies pending migrations.

One replica applies migrations while other replicas wait. Goose reads the ledger again after it gets the lock.

If a migration fails, PostgreSQL rolls back its SQL and ledger record. Earlier successful migrations remain applied and recorded.

Kubernetes restarts the failed replica. The next startup resumes from the first migration without a ledger record.

Changes

  • Add Goose and a per-migration ledger.
  • Replace the initial up and down files with one transactional, up-only migration.
  • Keep migration 1 aligned with the current schema, including actor egress policy storage.
  • Remove existence guards and explicit transaction statements from the baseline migration.
  • Apply all pending migrations before ateapi becomes ready.
  • Serialize each migration run with a PostgreSQL session advisory lock.
  • Start without changes when the database schema is current or ahead.
  • Reject application tables that do not have a migration ledger.
  • Log the starting, current, and latest versions.
  • Log the applied migration count and duration.
  • Retry only initial database connection failures.
  • Return schema and migration errors without a retry.
  • Add --postgres-schema and ATE_API_POSTGRES_SCHEMA.
  • Use public as the default PostgreSQL schema.
  • Use the configured schema for the main and watch pools.
  • Restrict outbox partition maintenance to the configured schema.
  • Let the installer use an external PostgreSQL database.
  • Add the migration design and recovery policy to the repository.

Migration file policy

Migration files use sequential versions and contain exactly one Goose Up section.

CI rejects down migrations, nontransactional migrations, environment substitution, explicit transaction control, and IF NOT EXISTS guards.

Before the first stable v1 release, developers can change or squash migrations. Developers must recreate databases after migration history changes.

After that release, CI rejects changes or deletions against the latest stable release tag that contains migrations.

Goose does not store migration checksums. The binary embeds each migration file, and release-tag checks protect released migration history.

Compatibility

No release includes PostgreSQL support. The v0.0.0 release predates the PostgreSQL backend.

Users must recreate databases from earlier PostgreSQL development builds.

Every committed migration prefix must work with the current and previous ateapi releases. This rule supports rolling upgrades and temporary binary rollback.

A binary rollback does not roll back the database schema.

Testing

Tests cover:

  • Fresh database migration.
  • Concurrent startup.
  • Advisory lock waits.
  • Current and ahead database schemas.
  • Rejection of application tables without a migration ledger.
  • Atomic rollback of a failed migration.
  • Retention of earlier successful migrations.
  • Resume from the failed migration after restart.
  • Configured schema isolation.
  • Outbox partition isolation.
  • Migration file policy checks.
  • Stable release migration immutability.

@google-cla

google-cla Bot commented Aug 25, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

…sql-schema-migrations-poc

# Conflicts:
#	cmd/ateapi/internal/store/atepg/atepg.go
#	cmd/ateapi/main.go
#	go.sum
#	hack/install-ate.sh
@iplay88keys
Jeremy Alvis (iplay88keys) marked this pull request as ready for review August 25, 2026 16:11
@iplay88keys Jeremy Alvis (iplay88keys) changed the title Iplay88keys/postgresql schema migrations poc ateapi: add versioned PostgreSQL schema migrations Aug 25, 2026
…sql-schema-migrations-poc

# Conflicts:
#	cmd/ateapi/internal/controlapi/actor_snapshot_test.go
…sql-schema-migrations-poc

# Conflicts:
#	go.sum
…sql-schema-migrations-poc

# Conflicts:
#	cmd/ateapi/internal/store/atepg/schema.go
Comment on lines +231 to +238
steps := int(current) - int(target)
if steps <= 0 {
return current, false, true, nil
}
if err := migrator.Steps(-steps); err != nil && !errors.Is(err, migrate.ErrNoChange) {
return current, false, true, fmt.Errorf("rolling back %d PostgreSQL migration(s) to version %d: %w", steps, target, err)
}
return target, false, true, nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If a failed migration on one replica triggers a Steps() downgrade, a second replica can race and run Steps() again, stepping below the pre-run version and running a released migration's down file (DROP TABLE IF EXISTS). This only becomes possible once migration 000002 exists, but we should defend against it now. Safer to fail closed at Force(N-1) and leave downgrades to operators.

Suggested change
steps := int(current) - int(target)
if steps <= 0 {
return current, false, true, nil
}
if err := migrator.Steps(-steps); err != nil && !errors.Is(err, migrate.ErrNoChange) {
return current, false, true, fmt.Errorf("rolling back %d PostgreSQL migration(s) to version %d: %w", steps, target, err)
}
return target, false, true, nil
return current, false, true, nil

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see what you are referring to. It seems that this would only be an issue if the replicas all roll at the same time. I was going based on a Kubernetes rollout where one pod gets recreated and the others stay as the old version until that pod succeeds. Since the migrations are the first step and crash the pod on failure (after rolling the migrations back), the other pods continue running the old version and don't touch migrations as a result.

There are situations where that might not be the case, so I'm looking into how to resolve this race condition anyway.

@jpbetz Joe Betz (jpbetz) Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd also be okay with some sort of guard rail that make the problem outcome impossible. Anything that keeps this from getting into the unrecoverable state is good enough for me. It's ~intractable in distributed systems to ensure that the individual processes do expected transitions and never get into states (like two in rolled forward state before the migration, even if we don't want that).. so we need to make sure that if they do, nothing bad happens. It's fine if the system errors out.. it's bad if it does the crazy unexpected thing and deletes data.

@iplay88keys Jeremy Alvis (iplay88keys) Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to preserve best-effort rollback to the pre-run version rather than stop after Force, because it returns a failed rollout to the clean migration state it started from instead of leaving a partially applied migration set.

The underlying problem here looks to be the scope of the advisory lock rather than the rollback itself. Instead of removing the Steps call, I propose adding a schema-scoped outer advisory lock held from before the initial version read through Up and any recovery. That ensures only one replica owns the complete migration attempt while preserving the documented recovery behavior.

slog.InfoContext(ctx, "PostgreSQL migrations ready", attributes...)
}()

if !dirty && current >= latest {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's document somewhere what exactly the schema evolution rules are to make this "old binary and new schema" HA case safe.

There are at least two rules:

  • Additive schema changes must be backward compatible (merged in a way that N-1 readers and writers can handle, e.g. new columns are nullable or have a default). This is not super surprising but still should be noted.
  • All removals must lag by an additional release. This is slightly more subtle and really should be written down.

Maybe a docs/dev page about migrations and the rules they must follow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I completely agree and will put something together. Having this clearly documented is important.

Comment thread hack/verify/postgresql-migrations.sh Outdated
fi
done

if (( $# == 1 )) && ! git diff --quiet --no-renames --diff-filter=MD "$1" HEAD -- "${MIGRATIONS_DIR}"; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How many migration scripts do we want to have, particularly for our v1? Fresh installs replay the whole chain, every file needs a tested down file, and it's basically a forever cost.

This check is what freezes them: once this merges, 000001 can't be edited, so #1283 and #1204 each become a permanent 000002/000003 for a pre-release schema.

Let's consider at least rolling them up into a single v1 schema change (e.g. don't pass the base sha from pr-workflow.yaml until we tag). We should also consider if we can have a single schema change per release. We'll need to consider how this impacts development.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we could allow migrations to be squashed until v1 releases. Then we would freeze the baseline and verify immutability against release tags.

After the v1 release, I don’t think we have to limit ourselves to one migration per release. The CI checks apply and roll back the migrations on every PR. I agree that rolling the current pre-release history into one baseline avoids permanently maintaining migrations that do not support any released upgrade path.

So, when we go to release v1, we would need to:

  • Manually squash any accumulated pre-release migrations into 000001_initial.up.sql and its down file
  • Run the tests which run an apply/down against that migration
  • Re-enable the base.sha argument in a follow-up PR, freezing all migrations from then onward

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This sounds right. Benjamin Elder (@BenTheElder) do you know how we can track this as a step in the v1 release? It's a one-time step that has a forever startup benefit. I think we should do it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This sounds right. Benjamin Elder (@BenTheElder) do you know how we can track this as a step in the v1 release? It's a one-time step that has a forever startup benefit. I think we should do it.

Good question, we might need a milestone tagged issue. Tim Hockin (@thockin) has been writing some drafts about how he sees compatibility going forward. I've been thinking about it from progressive feature support which is a bit unrelated :^)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#1399 track the pre-v1 work that needs to be done. If there is any place to track such work (label or pre-release project board), let's track this issue there.

return errors.Join(migrationErr, sourceErr, databaseErr)
}

func openMigrator(pool *pgxpool.Pool) (*migrate.Migrate, uint, error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's switch from a dirty bit to a per-migration ledger with checksums. goose or tern would give us this. PTAL at those if you haven't already.

There are two problems with the dirty bit:

  • It can become stale. The version row is written in a separate statement from the migration, so a crash at an inconvenient time leaves "dirty" set even though the DDL committed.
  • It says nothing about which SQL was applied, so an edited migration file goes unnoticed and the schema can get into states that are not supported or expected.

Tools that commit the version row in the same transaction as the migration don't need a dirty bit at all, and a checksum catches the drift. Many established systems (Flyway, Liquibase, goose, tern) use this approach because it's safer. Let's follow their lead on this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion on keeping golang-migrate here. It's just that have used it before and am familiar with how it works. I did some research into other migration libraries a while ago, but I can re-evaluate them with this in mind. I checked both suggested engines and from what I can tell, neither Goose or Tern have checksums. I do see the benefit to having the migrations and the version row being committed in the same transaction and that is something that Goose at least provides.

I'm curious about how important checksums are here when I have included a CI check in this PR which enforces migration immutability.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have included a CI check in this PR which enforces migration immutability.

The dirty corruption issues I reported above look real. Those need to be addressed. Are they?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's just that have used it before and am familiar with how it works.

It's more important that we make a solid choice than we choose something that one contributor if familiar with.

That said, I'm fine with using what you decide, I was just making a suggestion to make sure we've fully considered all options.

@iplay88keys Jeremy Alvis (iplay88keys) Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's more important that we make a solid choice than we choose something that one contributor if familiar with.

Yes, I absolutely agree 👍

The dirty corruption issues I reported above look real. Those need to be addressed. Are they?

Not yet, but I'm looking into it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After further research, it looks like the dirty-state ambiguity cannot be cleanly solved with golang-migrate, because its dirty-state updates and migration SQL execute as separate operations. I think moving to goose is the better approach due to the migration/version row being part of the same transaction as discussed above. I'll work on updating this PR and the design doc accordingly.

-- See the License for the specific language governing permissions and
-- limitations under the License.

BEGIN;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want it to be possible for the system to become unavailable indefinitely if a rollout goes bad? Without a lock timeout, an ALTER TABLE in a future migration that queues behind one long-running read blocks every query on that table until that read finishes.

Alternative would be to do something like this and rely on the pod restart to retry.

Suggested change
BEGIN;
BEGIN;
SET LOCAL lock_timeout = '5s';

Use Goose to record each applied migration and resume after a failure.
Serialize migration runs with a PostgreSQL session lock.
Remove down migrations and reject unsafe migration directives.
Add CI checks and schema evolution rules for rolling updates.
…sql-schema-migrations-poc

# Conflicts:
#	cmd/ateapi/internal/store/atepg/atepg_test.go
#	go.mod
#	hack/install-ate.sh
@iplay88keys

Copy link
Copy Markdown
Contributor Author

Joe Betz (@jpbetz) Thanks for the in-depth review on this. I've spent a lot of time today digging into the threads, researching different options, and re-evaluating the requirements that we have as well as those that I was optimistically adding. The culmination of all of that is a pretty significant re-write of this PR. At a high level, the new state is:

  • The PR now uses Goose instead of golang-migrate
  • A session advisory lock serializes the full migration run for each schema
  • The PR no longer uses dirty-state recovery, Force, or automatic rollback to the pre-run version
  • If a migration fails, PostgreSQL rolls back that migration and its ledger record and the next startup resumes from the first migration without a ledger record.
  • This iteration does not include down migrations
  • Developers can change or squash migrations before the first stable v1 release. After that release, CI protects released migration files against changes and deletions.
  • Added dev documentation around the migration requirements

The full current state is in the updated PR description above. I'm working on updating the design doc to match.

@jpbetz

Copy link
Copy Markdown
Contributor

Thanks for all the excellent work on this Jeremy Alvis (@iplay88keys)! I'll be another pass in a couple hours with the aim of getting this ready to merge.

@jpbetz

Joe Betz (jpbetz) commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

OK, just finished a full 2nd review pass. All open issues have been addressed and the implementation looks good. Thanks Jeremy Alvis (@iplay88keys) for the careful work on this.

/approve
/lgtm

@jpbetz

Copy link
Copy Markdown
Contributor

Since this adds a dependency, here's the delta:

Module Packages linked Vendored Go lines License (verbatim from LICENSE file)
pressly/goose/v3 v3.27.3 11 6,996 (58 files) MIT (3 copyright lines: Staskawicz 2012, Vitek 2016, Fridman/Vitek 2021)
go.uber.org/multierr v1.11.0 1 773 MIT (Uber Technologies)
mfridman/interpolate v0.0.2 1 547 MIT (Buildkite 2014–2017, Fridman 2023 — a Buildkite fork)
sethvargo/go-retry v0.4.0 1 381 Apache-2.0
jackc/pgx/v5/stdlib (module already present) 1 909 (newly vendored) MIT (pgx's existing license)
x/sync/errgroup (already vendored, newly linked) 1 BSD-3-Clause (existing)
Total 16 9,606 MIT ×4, Apache-2.0 ×1, BSD-3-Clause ×1

All licenses are in the CNCF allowlist.

@BenTheElder

Copy link
Copy Markdown
Collaborator

Thanks for the dep check. We have a few MPL deps, I just got rid of the one used in production #1293, the others are test only which AIUI is less of a problem but still not ideal. Things that ship to release artifacts we have to be more careful with.

@iplay88keys

Jeremy Alvis (iplay88keys) commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

OK, just finished a full 2nd review pass. All open issues have been addressed and the implementation looks good. Thanks Jeremy Alvis (Jeremy Alvis (@iplay88keys)) for the careful work on this.

Absolutely, glad to help! Thanks for the thorough reviews and quick feedback loop!

Anna Pendleton (annapendleton) pushed a commit that referenced this pull request Sep 3, 2026
…igMap (#1422)

Totally mirrors the changes added in #1196 to
[hack/install-ate.sh](https://github.com/agent-substrate/substrate/pull/1196/changes#diff-7cb7dc3b13c09863b66efb58d9869235317f6d8218ef328321c0839672620747).

Without this ate-setup is broken.

> It's a good idea to open an issue first for discussion.

- [ ] Tests pass
- [ ] Appropriate changes to documentation are included in the PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Define postresql schema update strategy

4 participants