Skip to content

feat(storage): add PostgreSQL storage-table schema definitions - #936

Merged
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/postgres-schema-ddl
Aug 5, 2026
Merged

feat(storage): add PostgreSQL storage-table schema definitions#936
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/postgres-schema-ddl

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds the PostgreSQL translation of SchemaBot's 13 embedded storage-table DDL files under pkg/schema/postgres/, exposed as schema.PostgresFS. This is the schema-definition slice of the dialect-split storage bootstrap; the PostgreSQL EnsureSchema bootstrapper and advisory locker follow in a separate PR.

What

  • pkg/schema/postgres/*.sql: one file per storage table, mirroring pkg/schema/mysql/ table-for-table and column-for-column. Each file holds one CREATE TABLE plus its CREATE INDEX statements.
  • schema.PostgresFS embed alongside MySQLFS.
  • Unit tests: file-set parity with the MySQL directory, index parity (every MySQL index must have a postgres counterpart with the same table, ordered column list, and uniqueness — and vice versa), a lint for MySQL-only constructs (backticks, AUTO_INCREMENT, ON UPDATE CURRENT_TIMESTAMP, unsigned, tinyint, datetime, engine/charset/collation options), table-name/filename agreement, and index-name rules (table prefix, schema-wide uniqueness, 63-char limit).
  • Integration test: executes every file against a real postgres:16 testcontainer, then diffs information_schema.columns against the TiDB-parsed MySQL files — column sets, nullability, the declared type mapping, and varchar widths must match exactly.
  • New deps: testcontainers-go/modules/postgres (pinned to the repo's existing testcontainers v0.40.0) and pgx/v5 stdlib driver (test-only for now; the PostgreSQL store will use it next).

Why

The storage bootstrap dispatch already fails closed for non-MySQL dialects; this supplies the PostgreSQL schema definitions it will route to.

Why a separate DDL directory rather than sharing the MySQL files (or generating both from one source): the differences are grammatical, not just type names — backtick quoting, AUTO_INCREMENT, inline KEY clauses, ENGINE/CHARSET table options, and unsigned integer types are all syntax errors in PostgreSQL, while index names move from a per-table to a schema-wide namespace. The MySQL files are also format-pinned: EnsureSchema diffs them via Spirit's TiDB parser, which requires canonical SHOW CREATE TABLE form, so they cannot drift toward anything dialect-neutral. And several translations are deliberate engineering choices (jsonb over json, identity columns, application-side updated_at) that a mechanical transliteration layer would get wrong or bury in special cases. Behavioral equivalence between the two schemas is enforced where it belongs — the cross-dialect storagetest parity suite (#935) will validate every store against the same interface contract once both land, and this PR's structural tests (file-set, index, column-set/nullability/type/width parity against the TiDB-parsed MySQL files) catch schema drift directly.

Translation decisions worth review:

  • updated_at: application-side stamping, no trigger. MySQL's ON UPDATE CURRENT_TIMESTAMP has no PostgreSQL equivalent. Rather than installing per-table triggers in the bootstrap, the PostgreSQL store will stamp updated_at in its UPDATE statements, and the cross-dialect parity suite enforces the behavior. Columns keep DEFAULT CURRENT_TIMESTAMP for inserts.
  • Index names are table-prefixed (idx_applies_repo_pr, not idx_repo_pr): MySQL index names are per-table, PostgreSQL's share one schema-wide namespace, so the MySQL names (reused across tables) would collide. A few names are also normalized where the MySQL name embedded its table (idx_apply_control_request_statusidx_apply_control_requests_status); the index parity test pins table/columns/uniqueness so renames can't hide a structural change.
  • Type mapping: identity PKs (GENERATED BY DEFAULT AS IDENTITY, allowing explicit ids like AUTO_INCREMENT), jsonb for json, boolean for tinyint(1) (the Go structs are bool), bigint for int unsigned (PostgreSQL has no unsigned types; integer would halve the value range), timestamp for datetime.
  • Timestamp precision widens to PostgreSQL's default microseconds. MySQL datetime is whole-second (except the one deliberate datetime(6) lease-expiry column); bare PG timestamp is microsecond everywhere. This is deliberate — second-truncating with timestamp(0) would replicate a MySQL storage artifact rather than a requirement, and the store layer already treats time precision as dialect-specific. Consequence to keep in mind: written time.Time values round-trip second-truncated on MySQL but exact on PostgreSQL, so cross-dialect behavioral tests must not assert exact stored-time equality across dialects.
  • Collation: the PG schema is case-sensitive; MySQL's utf8mb4_0900_ai_ci is case- and accent-insensitive. Every varchar comparison, and every varchar unique index, dedups case-insensitively on MySQL but byte-wise on PostgreSQL. Nothing in the store layer normalizes case today, so this stands as an open decision for the PostgreSQL store PR: citext, LOWER() expression indexes, or a documented app-side canonicalization guarantee (e.g. for repository names, which GitHub treats case-insensitively but case-preservingly).
  • Identity columns accept explicit ids but do not advance the backing sequence past them. Unlike AUTO_INCREMENT, an explicit-id backfill must setval the sequence afterwards or later default inserts collide. No code inserts explicit ids today; noting it for the store/backfill PR.

Merge coordination: the file-set and index parity tests intentionally turn main red if a MySQL table or index lands without a postgres counterpart. Two open PRs interact: #867 adds mysql/check_refresh_requests.sql (and an index on checks), #648 changes applies/apply_operations indexes. Whichever merges second must add the postgres counterpart — that's the tests doing their job.

Translates the 13 embedded MySQL bootstrap files to pkg/schema/postgres/
and exposes them as schema.PostgresFS. updated_at stamping becomes the
application's responsibility on PostgreSQL (no ON UPDATE CURRENT_TIMESTAMP
equivalent; no trigger installed). Index names gain a table prefix because
PostgreSQL index names share one schema-wide namespace.
Copilot AI lite review requested due to automatic review settings August 5, 2026 09:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a PostgreSQL dialect copy of SchemaBot’s embedded storage-table schema files and wires them into pkg/schema as an embedded FS, with tests to enforce cross-dialect parity and validate that the DDL executes against a real Postgres instance.

Changes:

  • Added pkg/schema/postgres/*.sql storage-table DDL translations and embedded them as schema.PostgresFS.
  • Added unit tests to enforce file-set parity, table/index naming rules, and to lint against MySQL-only syntax in Postgres files.
  • Added an integration test that executes all Postgres DDL files in a Postgres testcontainer and diffs column names + nullability against the TiDB-parsed MySQL schema.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/schema/schema.go Embeds Postgres schema files alongside MySQL via PostgresFS.
pkg/schema/postgres/applies.sql Postgres DDL for applies storage table and indexes.
pkg/schema/postgres/apply_comments.sql Postgres DDL for apply_comments storage table and indexes.
pkg/schema/postgres/apply_control_requests.sql Postgres DDL for apply_control_requests storage table and indexes.
pkg/schema/postgres/apply_logs.sql Postgres DDL for apply_logs storage table and indexes.
pkg/schema/postgres/apply_operations.sql Postgres DDL for apply_operations storage table and indexes.
pkg/schema/postgres/apply_target_locks.sql Postgres DDL for apply_target_locks storage table and uniqueness/index rules.
pkg/schema/postgres/checks.sql Postgres DDL for checks storage table and indexes.
pkg/schema/postgres/locks.sql Postgres DDL for locks storage table and indexes.
pkg/schema/postgres/plan_comments.sql Postgres DDL for plan_comments storage table and indexes.
pkg/schema/postgres/plans.sql Postgres DDL for plans storage table and indexes.
pkg/schema/postgres/settings.sql Postgres DDL for settings storage table and indexes.
pkg/schema/postgres/tasks.sql Postgres DDL for tasks storage table and indexes.
pkg/schema/postgres/webhook_events.sql Postgres DDL for webhook_events storage table and indexes.
pkg/schema/postgres_test.go Unit tests enforcing MySQL/Postgres schema file parity and Postgres naming/syntax rules.
pkg/schema/postgres_integration_test.go Integration test executing Postgres DDL and comparing column sets/nullability to MySQL (TiDB-parsed).
go.mod Adds dependencies for pgx stdlib driver and testcontainers Postgres module.
go.sum Adds checksums for newly introduced Postgres-related dependencies.
Suppressed comments (1)

pkg/schema/postgres_integration_test.go:98

  • The rows close error is discarded. If the driver reports an error on Close (e.g., protocol/stream issues), it’s useful for the test to surface it instead of silently ignoring it.
	defer func() { _ = rows.Close() }()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/schema/postgres/tasks.sql
Comment thread pkg/schema/postgres_integration_test.go Outdated
Kiran01bm and others added 2 commits August 5, 2026 19:50
…ty tests

Review findings: the drift net compared only column names and
nullability, so a missing or de-uniquified postgres index — which the
future PG store's ON CONFLICT upserts depend on — or a wrong type/width
passed every check. Add a structural index parity test (table, ordered
columns, uniqueness, both directions), extend the integration test to
assert the declared type mapping and varchar widths, cross-check the
index-lint regex against literal CREATE INDEX counts, pre-pull
postgres:16 in CI, and fix doc drift.
@Kiran01bm
Kiran01bm marked this pull request as ready for review August 5, 2026 09:56
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@aparajon

aparajon commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 3d04ec7.

Verdict: clean — no fix-before-merge findings. The translations are faithful, the parity tests fail closed, and the two dimensions the tests don't cover (primary keys and column defaults) I audited by hand across all 13 tables and found zero divergences.

The core risk in a hand-translated schema is a silent semantic gap the structural tests don't reach. The tests cover a lot mechanically — file-set parity both directions, index shape parity both directions (with a parse-count cross-check so an index the regex can't read fails loudly instead of escaping the lint), and a real-server integration test that reads back information_schema after the DDL runs, so it asserts what PostgreSQL actually created. What they don't cover, I checked manually: every one of the 13 files declares PRIMARY KEY (id), and a mechanical column-by-column defaults comparison found only the expected translations (identity columns are implicitly NOT NULL; tinyint(1) DEFAULT '1'/'0'boolean DEFAULT TRUE/FALSE). The datetime(6) lease-expiry column maps into PostgreSQL's default microsecond timestamp with no precision loss, and the unique index on nullable idempotency_key has the same multiple-NULLs semantics in both dialects.

Follow-up, not blocking
PK parity is untested A future edit could drop PRIMARY KEY (id) from a postgres file and every test would still pass. The integration test could assert it cheaply — information_schema.table_constraints on the PG side against the TiDB-parsed PK on the MySQL side.
Default parity is untested Same shape: information_schema.columns.column_default is already one column away from what the test reads today. All 13 tables match at this head, but drift would be invisible.
updated_at enforcement lands later The app-side stamping decision is sound, but nothing enforces it yet — the cross-dialect parity suite has no updated_at-advances assertion today. The PostgreSQL store PR should bring one.

Verified solid: every forbidden-construct lint token is genuinely absent from the postgres files and the lint's policy bans (charset/collate) are documented with their rationale; the index-name rules (table prefix, schema-wide uniqueness, 63-char identifier limit) match PostgreSQL's actual namespace semantics; the type mapping is declared exactly once (expectedPostgresColumnType) and its choices are right — bigint for unsigned ints rather than range-halving integer, jsonb, identity PKs that accept explicit ids; the integration test asserts column sets in both directions so an extra postgres column fails too; both Copilot threads are already resolved correctly (CloseAndLog applied; engine_migration_id kept because it mirrors an existing production column and a one-sided rename would fail the parity tests this PR adds); the branch is 0 behind main; CI is fully green; and locally at 3d04ec7 the build, the unit parity tests with -race, and the postgres:16 integration test all pass. The merge-coordination note in the PR body (#867/#648 must add postgres counterparts if they land second) is accurate — that's the file-set and index parity tests doing their job.

This review was generated by Claude Code (claude-fable-5).

@aparajon aparajon left a comment

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.

🤖 Approving on Armand's behalf after the adversarial correctness review above (no fix-before-merge findings). This stamp was left by Claude Code (claude-fable-5).

@Kiran01bm
Kiran01bm merged commit 3226738 into main Aug 5, 2026
32 checks passed
@Kiran01bm
Kiran01bm deleted the kiran01bm/postgres-schema-ddl branch August 5, 2026 22:01
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.

3 participants