Uh oh!
There was an error while loading. Please reload this page.
feat(databases-on-aws): use native ALTER TABLE DROP COLUMN for DSQL - #264
Open
mripley wants to merge 1 commit into
Open
feat(databases-on-aws): use native ALTER TABLE DROP COLUMN for DSQL#264mripley wants to merge 1 commit into
mripley wants to merge 1 commit into
Conversation
Aurora DSQL shipped ALTER TABLE ... DROP COLUMN on 2026-08-03. The skill
still routed a column drop through the Table Recreation Pattern, so agents
generated a CREATE new / INSERT ... SELECT / DROP TABLE / RENAME sequence --
destructive, slow, and now unnecessary. DROP COLUMN is synchronous and
metadata-only; unlike VALIDATE CONSTRAINT it has no ALTER TABLE ASYNC form
and returns no job_id.
- development-guide.md (always loaded before schema changes): add the rule
here so an agent gets the signal without loading a reference file.
- ddl-migrations/column-operations.md: replace the DROP COLUMN recreation
procedure with the native statement plus the rules that actually bite --
PK key columns rejected with 0A000, CASCADE only for out-of-table
dependents, CASCADE itself failing 0A000 when the dependency chain reaches
a PK key column, IF EXISTS, and the 255-active/1,600-lifetime budget.
- ddl-migrations/overview.md: move DROP COLUMN to the directly-supported
list; keep the PK-column case on the recreation path.
- mysql-migrations/{ddl-column-changes,type-mapping}.md: map MySQL DROP
COLUMN 1:1, out of the recreation tables. No batching -- the operation
does not touch rows.
- SKILL.md: lead Workflow 7 with the native path and retitle it, since it
no longer covers only table recreation.
Table recreation is unchanged for ALTER COLUMN TYPE, SET NOT NULL,
DROP CONSTRAINT, and MODIFY PRIMARY KEY.
Adds eval 17 as the regression net. Measured: 0/2 against the pre-change
skill (the agent recommends recreation verbatim), 2/2 after. Its grader
matches the agent's own answer rather than the transcript, so reading a
reference file that documents both approaches cannot satisfy it, and it is
ordered above the eval-5 "table recreation pattern" branch, which would
otherwise grade the negative assertion backwards.mripley
requested review from
Morlej, amaksimo, gxjx-x, jaichabria, pkale, praba2210 and spencercorwinAugust 28, 2026 20:14
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
spencercorwin
requested changes
Sep 2, 2026
There was a problem hiding this comment.
Code review
I found 7 validated issues after consolidating 20 independent reviews. It seems a lot of these are resolved with a rebase on main.
This was done using an automated AI fleet review with specific review skills.
Validated findings
| Priority | Finding | Confidence | Validation |
|---|---|---|---|
| P1 | Port eval 17 to the current schema and runner. Current main uses schema version 2, already assigns IDs 17–21, requires grader, and removed files. Taking this hunk as written would overwrite an existing ID or fail corpus validation. | 100 | Compared the PR with main at 026f03dc9a2f217ab96aa61f87f10fe8c09e067d. git merge-tree also reports conflicts in the eval corpus and runner. |
| P1 | Do not resolve the release conflict to version 1.8.0. Current main is already at 1.8.1, so the next feature release must be higher and remain equal across all three manifests. | 100 | Read all three version fields on the PR and current main; the PR is also currently reported as mergeable_state: dirty. |
| P1 | Replace the eval-17 regex with structured or semantic grading. The grader passes wrong targets, post-statement negation, and prose that still recommends recreation. It also fails a correct warning that says not to use DROP TABLE. This defeats the regression test. | 100 | Called grade_eval directly with crafted answers. Three incorrect answers scored 2/2, while a correct answer containing “Do not use DROP TABLE or RENAME TO” scored 1/2. |
| P1 | Provide a drop-specific recreation path for primary-key key columns. The linked MODIFY PRIMARY KEY procedure preserves and copies old_pk_column, so following it does not remove the column the user asked to drop. | 95 | The live cluster returned 0A000 for a primary-key key-column drop. Static tracing confirmed that the linked fallback recreates the old key as a regular column. |
| P2 | Reconcile the synchronous drop rule with “DDL ALWAYS runs asynchronously”. Both statements are mandatory guidance in the same always-loaded file, so agents cannot know whether to wait for a job. | 95 | On the supplied cluster, native DROP COLUMN returned in about 124 ms and the next query immediately saw the updated schema. No asynchronous job flow was required. |
| P2 | Add the multi-drop exception to the authoritative one-column rule. This file permits several drops, while the always-loaded development guide still requires a separate ALTER TABLE for every column modification. | 95 | The live cluster accepted ALTER TABLE t DROP COLUMN a, DROP COLUMN b and immediately exposed only the remaining columns. |
| P2 | Describe inbound foreign keys regardless of which table owns them, and preserve that direction in the MySQL guide. A self-referential inbound foreign key also requires CASCADE, while an outbound foreign key on the dropped column does not. | 100 | On the live cluster, self-referential and cross-table inbound references failed without CASCADE using 2BP01; both succeeded with CASCADE. An outbound foreign-key column dropped successfully without it. |
Dropped findings
| Candidate | Validation | Why dropped |
|---|---|---|
Native DROP COLUMN, multiple drops, and IF EXISTS might be unsupported | Ran each operation on the supplied cluster. All succeeded; a missing column without IF EXISTS returned 42703. | The PR is correct. |
| SQLSTATE values and generated-primary-key behavior might be speculative | Reproduced 0A000 for primary-key drops and generated-key cascade, and 2BP01 for dependent objects. | The documented codes and generated-key example are correct. |
| View, generated-column, and foreign-key dependency rules might be inaccurate | Created each dependency type on the supplied cluster and tested plain and cascading drops. | The core dependency rules are correct; only the self-reference and direction wording remains. |
| The synchronous or immediate behavior might be unsupported | Timed a native drop and queried information_schema immediately afterward. | Runtime behavior supports the new direct-drop description; only the contradictory blanket async rule is actionable. |
| The 255-active-column limit is clearly wrong | The cluster accepted 256 visible columns, then rejected the 257th with 54011 while saying “at most 255 columns.” | The service behavior and error text conflict by one. This is real but low-impact and below the review’s 80-confidence importance threshold. |
| Storage reclamation after updates is false | Compared the claim with public DSQL guidance; no contrary runtime evidence was found. | Insufficient evidence of a defect. |
Native drop must call dsql_lint first | Traced the skill’s lint requirements. They clearly apply to external SQL and generated recreation DDL, but not unambiguously to this fixed native statement. | The requirement is scope-dependent, so this is not a confident bug. |
| Bare drops must list every implicitly removed index and constraint | Reviewed the confirmation rules and dependency behavior. | This is a safety preference, not a demonstrated functional defect. |
| Markdown and dprint failures | mise run lint:md reported MD031, and mise run fmt:check reported two unformatted files. | The loaded review playbook excludes issues that dedicated linters and formatters catch automatically. |
| Stale neighboring DDL guidance should expand this PR | Compared with current main, which already changed several neighboring operations. | The PR explicitly scopes these out, and rebasing should preserve current main rather than expanding this change. |
Recommended action: rebase onto current main, port the eval into schema version 2, fix the validated guidance conflicts, and rerun the repository checks.
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 freeto 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.
Aurora DSQL shipped
ALTER TABLE ... DROP COLUMNon 2026-08-03. The DSQL skill still routed a column drop through the Table Recreation Pattern, so agents generated aCREATE new/INSERT ... SELECT/DROP TABLE/RENAMEsequence for what is now a one-line metadata change.Related
awslabs/mcp: feat(aurora-dsql-mcp-server): use native ALTER TABLE DROP COLUMN for DSQL mcp#4556awslabs/aurora-dsql-orms: test(tortoise): enable aerich downgrade coverage for native DROP COLUMN aurora-dsql-orms#600Changes
DROP COLUMNis synchronous and metadata-only. UnlikeVALIDATE CONSTRAINTit has noALTER TABLE ASYNCform and returns nojob_id— worth stating explicitly, since steering written for the VALIDATE CONSTRAINT launch invites that generalization.references/development-guide.md— adds the rule to the file that is always loaded before schema changes. This is the highest-leverage edit: the routing table alone did not guarantee an agent reached the detailed rules.references/ddl-migrations/column-operations.md— replaces the recreation procedure with the native statement plus the rules that actually bite:0A000; that case still needs table recreation.CASCADEis required only for dependents outside the table (a view, an inbound foreign key) or a same-tableGENERATED ... STOREDcolumn. A bare drop already sweeps the column's indexes and its same-tableCHECK/DEFAULT/UNIQUE/outbound-FK constraints, soCASCADEthere only widens the blast radius.CASCADEitself fails0A000when the dependency chain reaches a PK key column (e.g. a PK columnGENERATED ALWAYS AS (dropped_col) STORED) — no form ofDROP COLUMNsucceeds, so recreation is the fallback.IF EXISTS, gradual storage reclamation, and the 255-active / 1,600-lifetime column budget.references/ddl-migrations/overview.md— movesDROP COLUMNout of the recreation table into the directly-supported list.references/mysql-migrations/{ddl-column-changes,type-mapping}.md— maps MySQLDROP COLUMN1:1 and relocates the >3,000-row batching note to the type-change section, which is the path that actually copies rows.SKILL.md— Workflow 7 leads with the native path and is retitled, since it no longer covers only table recreation.1.7.1→1.8.0across.claude-plugin/plugin.json,.codex-plugin/plugin.json, andmarketplace.json.Table recreation is unchanged for
ALTER COLUMN TYPE,SET NOT NULL,DROP CONSTRAINT, andMODIFY PRIMARY KEY.Testing
Adds eval 17 as the regression net, with measured before/after:
Two grader details worth review attention:
column-operations.mddocuments both approaches, so grading against tool results would let a mere file read satisfy the assertion."table recreation pattern"branch. That branch passes when recreation is present, so a negative assertion falling into it would be graded backwards and a regression would score PASS. Eval 5 and its branch are unchanged, and the verification suite re-grades eval 5 to prove it.Known-stale neighbours, deliberately out of scope
DROP CONSTRAINT,SET/DROP DEFAULT,DROP NOT NULL,DROP EXPRESSION, andADD GENERATED AS IDENTITYall shipped 2026-07-06 and are still documented here as requiring table recreation — including on lines this PR touches. Kept separate to hold this PR to one logical change; each needs its own verification. NoteDROP CONSTRAINTis supported for CHECK/UNIQUE/FOREIGN KEY but not PRIMARY KEY, so that fix is a carve-out rather than a blanket removal.Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.