Skip to content

Consolidate migrations 001-080 into a checkpoint baseline#542

Merged
aaronsb merged 4 commits into
mainfrom
feat/migration-checkpoint
Jul 2, 2026
Merged

Consolidate migrations 001-080 into a checkpoint baseline#542
aaronsb merged 4 commits into
mainfrom
feat/migration-checkpoint

Conversation

@aaronsb

@aaronsb aaronsb commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • Checkpoint baseline (ADR-210's "periodically consolidate", executed): schema/00_baseline.sql is now a generated consolidation of migrations 001–080; the old files move to schema/migrations/archived/ and the next migration is 081.
  • Found and fixed a latent bug: warm migration 058 has never applied — anywhere. Details below.
  • Schema reference docs upgraded: the generator now parses pg_dump-style DDL (multi-word types, ALTER TABLE … ADD CONSTRAINT PKs/FKs) and emits per-schema mermaid ER diagrams, published by the existing GitHub Pages workflow, regenerated via make docs-schema.

Finding: migration 058 never ran

While replaying all 78 migration files into a pristine apache/age container with psql -v ON_ERROR_STOP=1, the replay aborted at 058_precreate_graph_labels.sql:

ERROR:  syntax error at or near "CREATE"
LINE 14: ... 'SELECT * FROM cypher(''knowledge_graph'', $$ CREATE VLA...

Two stacked defects, both invisible in production:

  1. Dollar-quote collision. The EXECUTE format(...) string embeds $$ CREATE VLABEL … $$ inside a DO $$ block — the inner $$ terminates the outer block, so the file has never parsed. migrate-db.sh treats failures as "skip, retry next run", so it failed silently on every platform start since the file was written. Confirmed on the live dev database: schema_migrations records versions 57 and 59, no 58.
  2. The statement was invalid anyway. openCypher has no CREATE VLABEL; label precreation is AGE's ag_catalog.create_vlabel()/create_elabel() API, whose cstring parameters need an explicit cast from a text variable (the first rewrite attempt surfaced function ag_catalog.create_vlabel(unknown, text) does not exist, silently swallowed by the migration's own EXCEPTION handlers).

Consequence: every fresh install has relied on lazy label auto-creation during ingestion — the exact concurrent-DDL race (relation "Source" already exists) 058 was written to eliminate — and 078's MATCH (:Ontology …) assumed a precreated label that never was. The fixed version creates all 45 missing labels cleanly in replay, and shows as pending-warm on existing deployments, so they finally apply it on next platform start. Modified in place deliberately: "never modify existing migrations" protects applied migrations; this one never applied.

How the checkpoint was built

schema/scripts/checkpoint.sh (kept for future re-pins):

  1. replay — old baseline + all 78 migrations into a throwaway apache/age:release_PG18_1.7.0 container (graph_accel mounted) via raw psql, no operator tooling.
  2. generate — pg_dump relational DDL + seed data + extensions; graph-seeding cypher migrations (014, 044, 058, 078) carried verbatim because AGE graphs cannot round-trip pg_dump; schema_migrations rows 1–80 recorded by the baseline.
  3. verify — a second container built from the candidate baseline alone, diffed against the replayed original on: full schema dump, AGE label catalog, per-table row counts, normalized seed data. All four match.

Upgrade-path semantics

  • Fresh installs: full resting state at initdb; nothing in archived/ re-runs.
  • Pre-checkpoint databases: migrate-db.sh scans archived/ before migrations/, so a DB at e.g. version 60 still applies 61–80, then 081+.
  • Dry-run against the live dev DB: cold shows 0 pending, warm shows exactly 058.

Test plan

  • checkpoint.sh verify — all four A/B diffs clean
  • migrate-db.sh --dry-run and --dry-run --warm against the live pre-checkpoint dev DB behave correctly
  • make docs-schema regenerates; table counts now match the real database (the old textual parser over-counted dropped/renamed tables)
  • After merge: rebuild/republish the kg-postgres image (it bakes 00_baseline.sql at build time)

https://claude.ai/code/session_018VLKzWianGCKvY7CPbow8Z

aaronsb added 3 commits July 1, 2026 23:17
…plied

Discovered while generating the v080 checkpoint baseline (see follow-up
commit): replaying every migration into a pristine apache/age container
with psql -v ON_ERROR_STOP=1 aborted at 058 with

    ERROR:  syntax error at or near "CREATE"
    LINE 14: ... 'SELECT * FROM cypher(''knowledge_graph'', $$ CREATE VLA...

Two stacked defects, both previously invisible:

1. Dollar-quote collision (parse-time, deterministic). The EXECUTE format
   string embedded `$$ CREATE VLABEL ... $$` inside a DO $$ block; the
   inner $$ terminates the outer block body, so the file has never parsed
   at all. Production never noticed because migrate-db.sh treats a failed
   migration as "skip, retry on next run" — it has failed silently on
   every platform start since the file was written. Confirmed on the live
   dev database: schema_migrations records versions 57 and 59 with no 58.

2. Even without the quoting bug, the statement was invalid: openCypher has
   no CREATE VLABEL statement. Label precreation is AGE's
   ag_catalog.create_vlabel()/create_elabel() API, whose parameters are
   cstring — a text variable needs an explicit ::cstring cast or function
   resolution fails. (This second failure mode surfaced during the first
   rewrite attempt, masked by the migration's own EXCEPTION handlers:
   "function ag_catalog.create_vlabel(unknown, text) does not exist".)

Consequences of it never running: every fresh install has relied on lazy
label auto-creation during ingestion — the exact concurrent-DDL race
('relation "Source" already exists') this migration was written to
eliminate — and migration 078's MATCH on :Ontology assumed a precreated
label that 058 never actually delivered.

The rewrite keeps the IF NOT EXISTS guards and EXCEPTION handlers, and
creates labels via PERFORM ag_catalog.create_vlabel('knowledge_graph',
lbl::cstring) / create_elabel(...). Verified in the checkpoint replay: it
creates all 45 missing labels on a fresh graph with zero failures, and it
now shows as pending-warm on the dev database, so existing deployments
finally apply it on their next platform start.

Safe to modify in place — "never modify existing migrations" protects
migrations that have been applied somewhere; this one never was, anywhere.

Claude-Session: https://claude.ai/code/session_018VLKzWianGCKvY7CPbow8Z
Executes ADR-210's 'periodically consolidate' plan. schema/00_baseline.sql
is now a generated checkpoint of the full resting state at version 080:

- Generated, not hand-merged: schema/scripts/checkpoint.sh replays the old
  baseline + all 78 migration files into a throwaway apache/age container
  via raw psql, then dumps relational DDL and seed data with pg_dump.
  Graph-seeding cypher migrations (014, 044, 058, 078) are carried into
  the baseline verbatim because AGE graphs cannot round-trip pg_dump.
- Verified faithful: a second container built from the candidate alone
  matches the replayed original on full schema dump, AGE label catalog,
  per-table row counts, and normalized seed data.
- Migrations 001-080 move to schema/migrations/archived/. The baseline
  records versions 1-80 in schema_migrations, so fresh installs skip them;
  migrate-db.sh scans archived/ first, so pre-checkpoint databases keep
  their incremental upgrade path. Next migration: 081.
- Warm migration 058 stays in migrations-warm/ for pre-checkpoint
  deployments (none ever applied it; see previous commit).

The kg-postgres image bakes 00_baseline.sql at build time and must be
rebuilt/republished with the next release.

Claude-Session: https://claude.ai/code/session_018VLKzWianGCKvY7CPbow8Z
The checkpoint baseline is pg_dump output, which the docs generator did not
fully understand: multi-word types (character varying, timestamp without
time zone) were truncated to their first word, and constraints emitted as
separate ALTER TABLE ... ADD CONSTRAINT statements (how pg_dump writes PKs,
FKs, UNIQUEs) were dropped entirely. Both are now parsed and folded back
into the column tables.

With FK edges available, each schema section now opens with a mermaid
erDiagram of its relationships — rendered natively by GitHub and by
mkdocs-material (superfences already configured), published to GitHub
Pages by the existing docs workflow, regenerated via make docs-schema.

Table counts in the regenerated page drop (42->40 kg_api, 15->14 kg_auth)
because the old textual parser counted tables that later migrations
dropped or renamed; the checkpoint reflects the true resting state.

Claude-Session: https://claude.ai/code/session_018VLKzWianGCKvY7CPbow8Z
@aaronsb

aaronsb commented Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Code Review

Scope per request: hand-written logic (checkpoint.sh, migrate-db.sh scan, 058 rewrite, generate-schema-docs.py). Baseline content not relitigated (A/B verification accepted); structure skimmed.

Medium — checkpoint.sh is already stale for its stated purpose (future re-pins)

Location: schema/scripts/checkpoint.sh:52-57, 126-129

Problem: This PR moves migrations to schema/migrations/archived/, but the script it introduces still points at the old locations:

  • GRAPH_SEED_MIGRATIONS lists migrations/014_vocabulary_as_graph.sql, migrations/044_..., migrations/078_... — those files now live under migrations/archived/. The next generate run dies at cat "$SCHEMA_DIR/$g" (fail-loud, at least).
  • migration_files() globs migrations/*.sql, which now matches nothing. Without nullglob, the literal pattern .../migrations/*.sql flows into cmd_replay, which will attempt psql_exec < '*.sql' and die with a confusing "migration *.sql failed" instead of cleanly skipping.

Why it matters: The header says the script is "kept for future re-pins", but it broke in the same commit that used it. Whoever runs checkpoint v2 hits both.

Suggestion: Update the seed paths to migrations/archived/... (or better: note that a v2 re-pin must recompute this list, since new graph-seeding migrations 081+ won't be in it), and add [ -f "$f" ] || continue in the replay loop.

Medium (pre-existing, adjacent) — migrate-db.sh's skip-and-retry path is unreachable under set -e

Location: operator/database/migrate-db.sh:2, 300-322

Problem: MIGRATION_OUTPUT=$(run_psql_file "$FILE" 2>&1) — with ON_ERROR_STOP=1, psql exits 3 on failure, the assignment's exit status is non-zero, and set -e aborts the whole script before MIGRATION_EXIT_CODE=$? on the next line ever runs. The carefully documented "skip, retry next run" branch (lines 308-322) only fires when psql exits 0 but prints ERROR/ROLLBACK — essentially never with ON_ERROR_STOP set.

Why it matters now: Not introduced by this PR, but the checkpoint raises exposure: a pre-checkpoint database (say v60) applies archived 61-80 in one run; a single transient failure aborts the run instead of skipping, and nothing after the failing migration is attempted. The comment block documents behavior the shell doesn't deliver.

Suggestion:

MIGRATION_EXIT_CODE=0
MIGRATION_OUTPUT=$(run_psql_file "$FILE" 2>&1) || MIGRATION_EXIT_CODE=$?

Fine as a follow-up rather than in this PR.

Low — 058: correct rewrite, but the failure-swallowing that hid the original bug is still there

Location: schema/migrations-warm/058_precreate_graph_labels.sql

The fix itself is right: ag_catalog.create_vlabel()/create_elabel() is the actual AGE API, the explicit ::cstring cast is required from a text loop variable, the ag_label existence check plus ON CONFLICT (version) DO NOTHING makes it idempotent, and the carried copy in the baseline (section 4 before section 5's records) sequences correctly — 058's own version insert lands first, section 5's collides harmlessly.

But the EXCEPTION WHEN OTHERS THEN RAISE NOTICE handlers are the same mechanism that swallowed create_vlabel(unknown, text) does not exist during your own first rewrite attempt. If the API drifts again, the migration "succeeds", records version 58, and creates zero labels — invisible unless someone runs verbose. Suggest RAISE WARNING instead of NOTICE, and/or a closing block that counts created labels against the expected 45 and warns on shortfall. The history comment explaining the in-place modification is exactly the right call.

Low — default admin credential in the baseline lost its warning comment

Location: schema/00_baseline.sql:6847

The bcrypt hash for admin/admin originates in migration 020, where it carried -- password: 'admin' (CHANGE IN PRODUCTION!). The pg_dump seed reproduces the hash comment-free. Not a new exposure (public repo, known default, pre-existing behavior on every fresh install), but the generated file drops the human-facing warning. Suggest checkpoint.sh inject a comment above the seed-data section, and confirm operator init forces rotation.

Low (docs) — generator misses sequence defaults and has a falsified uniqueness assumption

Location: schema/scripts/generate-schema-docs.py

  • apply_alter_constraints handles ADD CONSTRAINT but not ALTER COLUMN ... SET DEFAULT nextval(...) — pg_dump emits serial defaults that way, and the baseline has 26 of them, so those id columns render without their auto-increment default. Cosmetic.
  • render_er_diagram's comment "Table names are unique across schemas today" is already false: public.schema_migrations and kg_api.schema_migrations coexist. Neither participates in an FK edge, so no rendering bug today, but bare-name mermaid entities would silently merge if that changes. Suggest an assertion or schema-prefix-on-collision.

Otherwise the parsing changes are sound: multi-word type alternation is ordered before the bare-word fallback so character varying(255) and timestamp(3) with time zone parse whole; ALTER_CONSTRAINT handles ONLY and multi-line statements via DOTALL; FK targets strip the column list correctly on both write (fk.group(2)) and read (FK_FLAG stopping at (); the empty-migrations table branch renders correctly since the for loop body is vacuous.

What's solid

  • migrate-db.sh scan: correct. Glob order archived/*.sql then *.sql preserves numeric ordering (all archived < 081); unmatched globs fall to the existing [ -f ] guard; warm mode is unaffected (migrations-warm/archived/ doesn't exist, literal pattern skipped); fresh databases skip 1-80 via the baseline's schema_migrations records, so warm 058 correctly shows pending only on pre-checkpoint deployments.
  • checkpoint.sh verify design: the A/B two-container diff (schema, AGE labels, row counts, normalized data) is the right falsifiability move for a generated artifact, and the normalization rationale (varchar cast round-trip noise, sorted data for heap-order differences) is documented where it happens. One inherent limit worth knowing: the timestamp/UUID normalization is applied symmetrically, so a real difference in a timestamp-valued seed column would be masked — acceptable trade-off, just not free.
  • Baseline structure: section ordering (extensions → DDL → seed under session_replication_role = replica with the superuser requirement documented → graph seed → migration records) is coherent; ON CONFLICT DO NOTHING throughout the seed keeps it re-runnable; no plaintext secrets beyond the known default admin hash noted above; no stray BEGIN/COMMIT (initdb's per-file ON_ERROR_STOP failure mode is visible, which is fine here).
  • ADR-210 annotation and the ways/README updates keep the paper trail honest — the "executed" note in the ADR is exactly how a deferred plan should be closed out.

Assessment: No blocking issues. The two Medium items are robustness debts in tooling paths (future re-pin, failure handling) rather than correctness problems in what this PR ships. The upgrade-path semantics for both fresh and pre-checkpoint databases check out.


AI-assisted review via Claude

- checkpoint.sh: GRAPH_SEED_MIGRATIONS now points into migrations/archived/
  (this PR moved the files it referenced), and migration_files() filters
  unexpanded globs so replay works with an empty migrations/ directory.
  Validated end-to-end from the post-checkpoint repo state: replay picks up
  only warm 058 and the regenerated candidate verifies faithful.
- migrate-db.sh: the skip-and-retry path was dead code — under set -e a
  bare command-substitution assignment aborts the script on psql failure
  before the exit code is examined. Capture it with an || arm. Matters now
  that pre-checkpoint databases apply batches of archived migrations.
- 058 + baseline: label-creation failures escalate NOTICE -> WARNING so a
  future signature drift is visible instead of swallowed.
- baseline seed-data section: re-inject the CHANGE-IN-PRODUCTION warning
  for development-default credentials (lost from migration 020's comment
  during consolidation); generator emits it for future re-pins.
- generate-schema-docs.py: parse ALTER COLUMN ... SET DEFAULT nextval(...)
  (27 serial defaults were missing from the reference); disambiguate ER
  diagram entities whose names collide across schemas.

Claude-Session: https://claude.ai/code/session_018VLKzWianGCKvY7CPbow8Z
@aaronsb aaronsb merged commit 1e896bf into main Jul 2, 2026
5 of 6 checks passed
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.

1 participant