Repository navigation
/{server, testing}: fix drop unique constraint - #3569
coffeegoddd wants to merge 3 commits into
Conversation
|
|
SummaryCoverage spans database constraint removal across unique, check, primary-key, and foreign-key rules, including preserving unrelated indexes, schema isolation, missing-object behavior, and concurrent operations. It exercises both normal data flows and edge cases involving duplicate data, invalid references, case differences, and multi-schema changes. Merge with caution — a medium-severity, PR-attributable issue prevents schema-qualified foreign-key removal in multi-schema deployments, blocking an intended database operation while leaving existing data intact. The broader constraint and concurrency behavior appears healthy. Tests run by ItoTip Reply with @itoqa to send us feedback on this test run. |
| "Expected a TableNode for ALTER TABLE DROP CONSTRAINT statement") | ||
| } | ||
|
|
||
| // Expression indexes introduce virtual columns, whose table wrapper doesn't expose the underlying |
There was a problem hiding this comment.
Qualified foreign-key drops fail in another schema
What failed: The foreign-key drop command returns ERROR: table not found: child instead of removing the constraint from billing.child. Both billing.child and archive.child still have child_parent_id_fkey.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Database operators cannot remove the foreign key from the requested schema-qualified table. This can block a schema change, but there is no evidence of data loss or corruption.
- Steps to Reproduce:
- Set the search path to public and create billing and archive schemas.
- Create billing.parent and archive.parent, then create billing.child and archive.child with a foreign key named child_parent_id_fkey in each schema.
- Create the billing expression index and insert valid parent and child rows.
- Run ALTER TABLE ONLY billing.child DROP CONSTRAINT child_parent_id_fkey.
- Run ALTER TABLE billing.child DROP CONSTRAINT child_parent_id_fkey as a second qualified attempt and inspect the foreign-key metadata.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The new resolver unwraps the resolved table at lines 42-47, so the target schema remains available as rt.Schema(ctx). For a matching foreign key, however, lines 49-58 call plan.NewAlterDropForeignKey(rt.Database().Name(), rt.Name(), fk.Name) and only attach the database provider; no target schema is supplied. The neighboring unique-index branch at lines 93-98 explicitly calls WithTargetSchema(rt.Schema(ctx)), demonstrating the missing schema propagation. The runtime test created billing.child and archive.child with identical constraint names, set search_path to public, and observed table not found: child for both ALTER TABLE ONLY billing.child and ALTER TABLE billing.child; metadata then showed both schema-qualified foreign keys still present. The smallest fix is to preserve the resolved schema when constructing the foreign-key drop operation, using the corresponding target-schema mechanism supported by the AlterDropForeignKey plan node, or otherwise execute it against the already resolved schema-qualified table rather than reacquiring an unqualified child table.
- Why this is likely a bug: The requested behavior is independently stated in the PR description: schema-qualified drops must affect only the requested table. The failure is deterministic with two schema-qualified syntaxes and is not caused by missing setup, because both schemas, both tables, their parent rows, the child rows, and the duplicate foreign-key names were created successfully before the drop. The resolver has already identified the qualified table, but the new foreign-key operation discards that schema before execution. A targeted schema-preserving change in this PR's new branch should allow billing.child to be changed without touching archive.child.
Relevant code
server/analyzer/resolve_drop_constraint.go:42-58
resolvedTable := *rt
resolvedTable.Table = sql.GetUnderlyingTable(rt.Table)
rt = &resolvedTable
...
if fkt, ok := table.(sql.ForeignKeyTable); ok {
...
if strings.EqualFold(fk.Name, dropConstraint.Name) {
newNode, err := plan.NewAlterDropForeignKey(rt.Database().Name(), rt.Name(), fk.Name).
WithDatabaseProvider(a.Catalog.DbProvider)
return newNode, transform.NewTree, err
}
}server/analyzer/resolve_drop_constraint.go:93-98
if index.ID() != "PRIMARY" && index.IsUnique() && index.ID() == dropConstraint.Name {
newNode, err := plan.NewAlterDropIndex(rt.Database(), rt, dropConstraint.IfExists, index.ID()).
WithTargetSchema(rt.Schema(ctx))
return newNode, transform.NewTree, err
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Qualified foreign-key drops fail in another schema**
**What failed:** The foreign-key drop command returns `ERROR: table not found: child` instead of removing the constraint from billing.child. Both billing.child and archive.child still have child_parent_id_fkey.
- **Impact:** Database operators cannot remove the foreign key from the requested schema-qualified table. This can block a schema change, but there is no evidence of data loss or corruption.
- **Steps to reproduce:**
1. Set the search path to public and create billing and archive schemas.
2. Create billing.parent and archive.parent, then create billing.child and archive.child with a foreign key named child_parent_id_fkey in each schema.
3. Create the billing expression index and insert valid parent and child rows.
4. Run ALTER TABLE ONLY billing.child DROP CONSTRAINT child_parent_id_fkey.
5. Run ALTER TABLE billing.child DROP CONSTRAINT child_parent_id_fkey as a second qualified attempt and inspect the foreign-key metadata.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The new resolver unwraps the resolved table at lines 42-47, so the target schema remains available as rt.Schema(ctx). For a matching foreign key, however, lines 49-58 call plan.NewAlterDropForeignKey(rt.Database().Name(), rt.Name(), fk.Name) and only attach the database provider; no target schema is supplied. The neighboring unique-index branch at lines 93-98 explicitly calls WithTargetSchema(rt.Schema(ctx)), demonstrating the missing schema propagation. The runtime test created billing.child and archive.child with identical constraint names, set search_path to public, and observed table not found: child for both ALTER TABLE ONLY billing.child and ALTER TABLE billing.child; metadata then showed both schema-qualified foreign keys still present. The smallest fix is to preserve the resolved schema when constructing the foreign-key drop operation, using the corresponding target-schema mechanism supported by the AlterDropForeignKey plan node, or otherwise execute it against the already resolved schema-qualified table rather than reacquiring an unqualified child table.
- **Why this is likely a bug:** The requested behavior is independently stated in the PR description: schema-qualified drops must affect only the requested table. The failure is deterministic with two schema-qualified syntaxes and is not caused by missing setup, because both schemas, both tables, their parent rows, the child rows, and the duplicate foreign-key names were created successfully before the drop. The resolver has already identified the qualified table, but the new foreign-key operation discards that schema before execution. A targeted schema-preserving change in this PR's new branch should allow billing.child to be changed without touching archive.child.
**Relevant code:**
`server/analyzer/resolve_drop_constraint.go:42-58`
~~~go
resolvedTable := *rt
resolvedTable.Table = sql.GetUnderlyingTable(rt.Table)
rt = &resolvedTable
...
if fkt, ok := table.(sql.ForeignKeyTable); ok {
...
if strings.EqualFold(fk.Name, dropConstraint.Name) {
newNode, err := plan.NewAlterDropForeignKey(rt.Database().Name(), rt.Name(), fk.Name).
WithDatabaseProvider(a.Catalog.DbProvider)
return newNode, transform.NewTree, err
}
}
~~~
`server/analyzer/resolve_drop_constraint.go:93-98`
~~~go
if index.ID() != "PRIMARY" && index.IsUnique() && index.ID() == dropConstraint.Name {
newNode, err := plan.NewAlterDropIndex(rt.Database(), rt, dropConstraint.IfExists, index.ID()).
WithTargetSchema(rt.Schema(ctx))
return newNode, transform.NewTree, err
}
~~~|
@coffeegoddd DOLT
|
|
Diff SummaryCoverage spans constraint removal across unique, primary-key, foreign-key, and check rules, including schema isolation, index preservation, retries, same-session recovery, and concurrent operations. The main gap is in an edge-case migration path for safely ignoring an already-missing constraint; broader dependency-action compatibility issues were also observed but are unrelated to this change. Merge with caution — this PR introduces a medium-severity failure in the missing-constraint IF EXISTS behavior, which can interrupt repeatable schema migrations instead of treating the operation as a no-op. Other medium-severity dependency-handling findings are pre-existing and should be tracked separately, but the attributable failure warrants a fix before considering the change fully safe. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Missing IF EXISTS drops return a syntax error
Evidence Package🟡 Dependent constraint drops return the wrong error code
Evidence Package🟡 Constraint dependency actions return wrong results
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
31a34b7 to
081c5e5
Compare
Commit: SummaryCoverage spans safe schema changes involving primary keys, foreign keys, unique and check rules, including qualified names, mixed-case names, missing objects, concurrent requests, multi-step updates, session recovery, and preservation of indexes and defaults. The exercised behavior is largely healthy across normal flows and edge cases, with one separate data-integrity issue where an invalid value bypassed an existing check rule. Safe to merge — the only failure is explicitly unrelated to this PR and is not a regression or a previously flagged issue; the PR’s constraint-removal behavior remains covered by passing scenarios. The unrelated check-enforcement defect is a serious follow-up data-integrity concern, but it is not a merge blocker for this change. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Check constraint accepts an invalid row
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |


Fix
DROP CONSTRAINTon tables with expression indexes by resolving constraints against the underlying table. EnsureIF EXISTSremoves existing unique constraints and schema-qualified drops affect the correct table. Fixes #3524