fix(db2): five bugs found by running the generator against real engines - #264
Merged
Conversation
The SQLite oracle needs no credentials, so it runs everywhere — but it cannot tell you whether the *per-dialect* quoting is right. Backticks for MySQL, brackets for T-SQL, double quotes elsewhere: only the servers know, and getting it wrong is how a migration dies halfway through a customer's database. Runs the same hostile schemas — spaces, reserved words, punctuation, non-ASCII, an index and an FK over awkward names — against Postgres, MySQL, MariaDB, SQL Server, CockroachDB and YugabyteDB from `docker compose`. 30 cases, all green, with the identifier-quoting fix in place. Gated behind FOX_IT_DB=1 so the default run and CI stay DB-free; unreachable engines skip individually so a partial stack still tells you something. Each case asserts it generated statements before executing them, so an empty plan cannot pass vacuously, and every table it creates is dropped afterwards. Verified the harness can actually fail: the unquoted form of the same statement is rejected by the live server with `syntax error at or near "Order"`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ithfully Creating a table exercises one code path; ADD/DROP/MODIFY COLUMN go through the per-dialect hooks, which is where the dialects diverge most. Adds an ALTER case over awkward names to every live engine, and a DB2 target for when that container is up. The ALTER case failed on Postgres and YugabyteDB at first, and the failure was this harness, not the product — worth writing down because it looks so much like a real bug. Postgres's dependent-view hooks stash view definitions in a `CREATE TEMP TABLE … ON COMMIT DROP` and read them back several statements later, so the plan only holds together when it runs the way MigrationModule runs it: one unpooled connection, one transaction. A connection per statement loses the temp table with the session; a transaction per statement drops it at the first commit. Both report `relation "_fs_vdep_…" does not exist`. `runPlan` now mirrors MigrationModule exactly, so what the test proves is what the product actually does. 49 cases green across Postgres, MySQL, MariaDB, SQL Server, CockroachDB and YugabyteDB. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DB2 leaves a table in *reorg-pending* after `ALTER TABLE … DROP COLUMN` (and
some type changes). `SELECT` still works — every INSERT/UPDATE/DELETE, and
every index or key rebuild, fails with **SQL0668N reason code 7** until REORG
runs. So the migration reported success and handed back a table nobody could
write to, and because reads kept working it could be a long time before anyone
connected the two.
Adds a `postColumnChangeStatements` hook, emitted after the column changes and
*before* the keys and indexes are rebuilt — those rebuilds are blocked by the
same pending state. DB2 implements it as
`CALL SYSPROC.ADMIN_CMD('REORG TABLE …')`; bare `REORG TABLE` is a CLP command,
not SQL, and cannot be sent over a client connection. No other dialect
implements the hook, and a unit test holds them to that.
Two harness faults had to be fixed before this bug could even be seen, both
worth recording because each produced a confident green:
* The liveness probe was `SELECT 1`, which DB2 rejects (SQL0104N — it wants a
FROM clause). DB2 was therefore marked unreachable and every DB2 case
returned early, reported as **passing** in 0ms while touching nothing.
Targets now carry their own probe, and an unreachable engine calls
`ctx.skip()` so it can never again be mistaken for a pass.
* The post-migration check was a `SELECT`. Reads are exactly what reorg-pending
still allows, so it went green against a table the user could no longer
write to. It is an INSERT/DELETE now.
Verified both directions against DB2 11.5: without the REORG the live suite
fails with SQL0668N on the write-back; with it, all 49 cases pass across
Postgres, MySQL, MariaDB, SQL Server, CockroachDB, YugabyteDB and DB2.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…, drifting defaults Ran the shipped DEMO_A/DEMO_B samples through the actual app path against DB2 11.5 — provider introspection, CompareModule, generated plan applied to a throwaway schema, then re-introspected and re-compared. Three bugs, each of which a string test could not have seen. **Every DB2 comparison carried six phantom ROLE objects.** `SYSCAT.ROLES` was read unfiltered, so DB2's own built-in roles (SYSDEBUG, SYSGEOADM, SYSTS_*) were reported as user objects in every schema. They cannot be recreated — DB2 reserves the SYS prefix and answers `CREATE ROLE SYSDEBUG` with SQL0707N — so the migration also contained eleven statements that can never succeed. In this database *every* role is a system role, so the whole ROLE section was noise. **Sessions never worked on DB2 at all.** The query selected `SESSION_DB_PARTITION_NUM`, which `MON_GET_CONNECTION` does not expose: DB2 answered SQL0206N and the utility failed outright. It is `CURRENT SERVER` now, which is what the `database_name` column claims to be — a partition number was the wrong value for that slot regardless. **Migrations to DB2 never converged.** DB2 rejects adding a NOT NULL column to a populated table without a default (SQL0193N), so the dialect appends `WITH DEFAULT` — correct, and documented. But that leaves the column holding a default (`''`, `0`) the source never declared, so re-comparing straight after a *successful* migration still reported the column as changed and proposed the same work again, for ever. A new `afterAddColumnStatements` hook drops the implicit default once the rows are backfilled; verified on the server that DROP DEFAULT is accepted immediately after the ADD, needs no REORG between, and leaves the catalog default NULL — matching the source exactly. End state on the samples: 30/30 statements execute, and re-comparing after the migration reports **no differences at all**. All five utilities (pool, sessions, system, sizes, index-fragmentation) run against the live server, and DEMO_A is byte-for-byte untouched by the run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…self back
Drove the whole product loop against real DB2 for the first time: seed a scratch
schema with MigrationModule, capture v1 with Lokee, migrate DEMO_A into it,
capture v2, diff the versions, plan a revert, execute it, and check the database
actually went back.
It did not. The revert reported success and changed nothing.
Table drops are ordered before the ALTERs, and dropping a parent table takes its
inbound foreign keys with it — so `ALTER TABLE … DROP FOREIGN KEY` ran against a
constraint DB2 had already removed and raised SQL0204N. DB2 has transactional
DDL, so that one statement rolled the *entire* revert back. Everything else in
the plan was correct; none of it survived.
The dialect already knows this shape: its DROP TABLE/VIEW go through a SQL PL
`CONTINUE HANDLER FOR SQLSTATE '42704'` because DB2 has no DROP IF EXISTS. The
FK drop simply never got the same treatment, even though the generic fallback it
overrides says `DROP CONSTRAINT IF EXISTS` for exactly this reason. It is
wrapped now.
Verified end to end on DB2 11.5: after the revert the live schema matches v1
exactly, and the new version's root hash equals v1's — content-addressed proof
that what came back is identical, not merely similar.
One note for whoever writes the next harness: `MigrationModule.execute` reports
failure through its **event stream** (`{type:'done', success:false,
rolledBack:true}`), not by throwing. My first pass wrapped it in try/catch, saw
no exception, and cheerfully reported a rolled-back revert as applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Routine bodies are the least portable thing in SQL and nothing here exercised them: the DB2 samples contain no routines at all (its seed's compound blocks are only drop-cleanup), so DEMO_A/DEMO_B never tested a single one. Adds the round trip, which is the only version of this test worth running: create a function and a procedure with native DDL, read them back through the provider, ask the generator to recreate them in a *second* schema, and execute that. Asserting "the object exists" would pass on a definition that is empty, missing its terminator, or has the source schema baked into it; executing the regenerated DDL somewhere else catches all three. Covers Postgres, MySQL, SQL Server and DB2 — each with its own body, since there is no portable one. Engines without a spec skip visibly rather than reporting a green they did not earn. All four pass: the captured definition is faithful enough to rebuild the routine elsewhere. MySQL needs the second *database* (a MySQL schema is a database), which the demo user has no rights to create, so that one target carries admin credentials — an environment limit, not a product one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…se buttons `browseSchema`, `isBrowsing`, `dialectOptions` and the `PROVIDER_SETTINGS` import stopped being referenced when Browse became its own pane and the buttons came out of Compare. Dead either way, but they read as if Compare still has a browse path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6b1037bb-b449-4f64-8669-1ac9a7697026) |
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
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.
Follow-on to #263, which merged while this work was still going. Same branch of
effort, new base: everything here was written after that merge and none of it
is in
main.The theme: the generator had only ever been checked against strings and SQLite.
Pointing it at the real servers — six of which were already running in
docker compose, plus DB2 pulled for this — found five bugs in a day.Bugs
DB2 left tables unwritable after a column drop. DB2 puts a table into
reorg-pending after
DROP COLUMN:SELECTstill works, every INSERT/UPDATE/DELETE fails with SQL0668N reason 7, and so does rebuilding the table's indexes.
The migration reported success and handed back a table nobody could write to —
and because reads kept working, it could be a long time before anyone connected
the two. New
postColumnChangeStatementshook emitsCALL SYSPROC.ADMIN_CMD('REORG TABLE …')before the keys and indexes are rebuilt.Every DB2 comparison carried six phantom objects.
SYSCAT.ROLESwas readunfiltered, so DB2's own built-in roles (SYSDEBUG, SYSGEOADM, SYSTS_*) appeared
as user objects in every schema. They cannot be recreated — DB2 reserves the SYS
prefix and answers
CREATE ROLE SYSDEBUGwith SQL0707N — so the migration alsocarried eleven statements that can never succeed.
Sessions never worked on DB2 at all. The utility selected
SESSION_DB_PARTITION_NUM, whichMON_GET_CONNECTIONdoes not expose: SQL0206N,dead on arrival. It is
CURRENT SERVERnow, which is what thedatabase_namecolumn claims to be.
Migrations to DB2 never converged. DB2 rejects adding a NOT NULL column to a
populated table without a default (SQL0193N), so the dialect appends
WITH DEFAULT— correct, and documented. But that leaves a default the source neverdeclared, so re-comparing straight after a successful migration reported the
same change again, for ever. New
afterAddColumnStatementshook drops theimplicit default once the rows are backfilled.
A revert could roll itself back and report success. Table drops are ordered
before the ALTERs, and dropping a parent table takes its inbound foreign keys
with it — so
ALTER TABLE … DROP FOREIGN KEYhit a constraint DB2 had alreadyremoved (SQL0204N). DB2 has transactional DDL, so that one statement rolled the
entire revert back. The dialect already wraps its DROP TABLE/VIEW in a
CONTINUE HANDLER FOR SQLSTATE '42704'because DB2 has no DROP IF EXISTS; the FKdrop simply never got the same treatment, though the generic fallback it
overrides says
DROP CONSTRAINT IF EXISTSfor exactly this reason.Coverage added
generated-ddl-live.test.ts, gated behindFOX_IT_DB=1so CI stays DB-free:non-ASCII, index + FK) on Postgres, MySQL, MariaDB, SQL Server, CockroachDB,
YugabyteDB and DB2.
regenerate into a second schema, execute. Asserting "the object exists" would
pass on a body that is empty, missing its terminator, or has the source schema
baked in. All four engines with a routine spec pass.
table still works" are different claims.
Notes for review
Three harness faults produced confident greens before any of the above was
visible, and each is now guarded — worth knowing if you extend this file:
SELECT 1, which DB2 rejects (SQL0104N). DB2 wasmarked unreachable and every DB2 case passed in 0ms while touching nothing.
Targets carry their own probe now, and unreachable engines call
ctx.skip()so a skip can never again read as a pass.
SELECT— exactly what reorg-pending stillallows — so it went green against a table the user could not write to.
MigrationModule.executereports failure through its event stream(
{type:'done', success:false, rolledBack:true}), not by throwing. Atry/catch around it reported a rolled-back revert as applied.
Not every red was the product's: I ignored the bind
paramsthe utility builderreturns, omitted a required
tableargument, seeded a scratch schema from thewrong side, and pointed a round trip's source and target at the same schema.
Those were fixed in the harness before any bug above was reported.
Verification
npx vitest run— 1798 passed, 87 skippedFOX_IT_DB=1live suite — 53 passed, 3 visibly skipped (engines with no routine spec)cd apps/web && npx tsc --noEmit— clean; eslint — 0 errorsafter the migration reports no differences, and after a Lokee revert the
new version's root hash equals v1's — content-addressed proof the schema
came back identical. DEMO_A is untouched by the run.
🤖 Generated with Claude Code
Note
High Risk
Changes DB2 migration DDL, transactional revert behavior, and schema comparison output—areas that directly affect customer database writes and migration success reporting.
Overview
Fixes five DB2 issues uncovered by executing generated DDL on real servers (not just SQLite parsing), and adds an optional live test harness (
FOX_IT_DB=1) for CREATE/ALTER, routine round-trips, and post-migration write probes.DB2 migration generator gains dialect hooks
afterAddColumnStatementsandpostColumnChangeStatements, wired inSqlGeneratorModulebefore index/key rebuilds. DB2 now drops the implicit default afterADD … WITH DEFAULT(so re-compare converges), runsREORG TABLEviaSYSPROC.ADMIN_CMDafter column drops/type changes (avoids reorg-pending tables that still SELECT but reject writes), and wraps FK drops in the same SQLSTATE42704handler as other drops so transactional DDL does not roll back the whole plan when the constraint is already gone.DB2 introspection filters built-in
SYS%roles fromSYSCAT.ROLES/ROLEAUTHso comparisons no longer include phantom roles and impossibleCREATE ROLEstatements.DBA Sessions utility for DB2 uses
CURRENT SERVER AS database_nameinstead of the invalidSESSION_DB_PARTITION_NUMcolumn (SQL0206N).UI:
TopToolbardrops unusedPROVIDER_SETTINGS/ browse-related store fields (no functional toolbar change in the diff).Unit tests cover DB2 REORG ordering, DROP DEFAULT convergence, and FK drop tolerance; live tests skip unreachable engines per-dialect probe (e.g. DB2
FROM SYSIBM.SYSDUMMY1).Reviewed by Cursor Bugbot for commit 45b2016. Bugbot is set up for automated code reviews on this repo. Configure here.