fix fk meta - #26469
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
…xone into 0730-fix-fk-meta
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 3d9239a.
The earlier ALTER-added/unnamed-FK, explicit RESTRICT, and competing-secondary-index findings are addressed. Three blocking closure gaps remain:
-
pkg/bootstrap/versions/v4_0_6/upgrade.go:345 only accepts an index when candidate.columns and key.columns have the same length. That contradicts the new planner contract at pkg/sql/plan/build_ddl.go:4816, which deliberately accepts an ordered leading prefix. A legacy table with PRIMARY KEY(id, code) and an FK referencing parent(id) is valid before and after this change, but the migration returns an empty referenced_index_name; a new FK with the same shape records PRIMARY. My focused counterexample reproduced actual "" versus expected "PRIMARY". Backfill must use the same prefix and deterministic key-selection contract as new binding.
-
referenced_index_name is denormalized at bind time and REFERENTIAL_CONSTRAINTS now trusts it unconditionally, but its index lifecycle is not closed. Both standalone DROP INDEX and ALTER TABLE ... DROP INDEX currently accept a UNIQUE index even when the parent has RefChildTbls, and neither path rejects the operation nor rebinds/updates child ForeignKeyDef and mo_foreign_keys. Therefore parent UNIQUE KEY uq_parent(id), child FK -> uq_parent, DROP INDEX uq_parent leaves UNIQUE_CONSTRAINT_NAME pointing to a constraint that no longer exists. Either reject dropping the selected referenced key or atomically rebind and update every engine/catalog consumer; add the public SQL regression.
-
The same-version offset fix at pkg/bootstrap/service_upgrade_tenant.go:253 only repairs the background executor. Existing tenants do not yet have the three new mo_foreign_keys columns, while the new binary immediately selects/inserts them. CN startup schedules BootstrapUpgrade asynchronously and starts the SQL frontend; asyncUpgradeTenantTask waits for its timer, and MaybeUpgradeTenant returns immediately when 4.0.6 == 4.0.6. In that deterministic window, ordinary CREATE TABLE reaches GetFkReferredTo and queries missing columns, and FK DDL also inserts them. Make tenant admission/readiness offset-aware or otherwise keep old/new catalog access compatible until the tenant migration commits. Add a same-version-offset test that executes DDL before the background task runs.
Also refresh test/distributed/cases/foreign_key/fk_show_columns.result, which still contains the old 16-column mo_foreign_keys schema.
Local validation: full tests passed for pkg/bootstrap/versions/v4_0_6, pkg/util/sysview, pkg/sql/plan, pkg/bootstrap, and pkg/sql/compile; vet passed for those owning/dependent packages. The prefix counterexample fails for the semantic reason above, and a focused planner witness confirms DROP INDEX remains publicly reachable. Current CI is green, but both upgrade compatibility jobs were skipped.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Approved on exact head 04dc7bc.
The previous blocking findings are closed systemically:
- Legacy FK backfill now uses the same ordered-leading-prefix and deterministic PRIMARY/lexical-UNIQUE selection contract as new binding, with prefix, non-prefix, precedence, and tie controls.
- Referenced-key lifetime is closed across standalone DROP INDEX, ALTER TABLE DROP INDEX/KEY, and DROP PRIMARY KEY. New definitions protect the exact persisted binding; legacy definitions fail safe for every compatible key because their historical selection is not provable. A same-statement self-FK removal remains allowed, and an unselected compatible UNIQUE remains droppable for persisted bindings.
- FK catalog access negotiates the tenant-local 16/19-column layout. Before all metadata columns exist, CREATE/ALTER uses legacy reads and explicit 16-column inserts; after commit it switches to the extended layout. This removes the deterministic async same-version-offset DDL failure window without blocking tenant admission or adding shared upgrade state.
- fk_show_columns expectations are refreshed, and BVT coverage now verifies the selected UNIQUE/PRIMARY cannot be removed while the surviving metadata remains stable.
Fresh local validation passed for build, vet, and full tests in pkg/bootstrap/versions/v4_0_6, pkg/sql/plan, pkg/bootstrap, pkg/sql/compile, and pkg/util/sysview. CI is running and was not awaited.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 04dc7bc from first principles after refreshing all commits, comments, reviews, and checks. No blocking findings remain.
This pass independently revalidated the full closure:
- New binding and legacy backfill share the ordered-leading-prefix contract and deterministic PRIMARY/lexical-UNIQUE selection. Prefix, non-prefix, PRIMARY precedence, and competing-UNIQUE controls are covered.
- Referenced-key lifetime is closed across standalone DROP INDEX, ALTER TABLE DROP INDEX/KEY, and DROP PRIMARY KEY. Persisted bindings protect only the selected key; legacy empty bindings fail safe across every compatible key; same-statement self-FK removal remains valid.
- The asynchronous same-version tenant-upgrade window is safe. Catalog reads negotiate the tenant-local layout, legacy writes name all 16 columns explicitly, and DML metadata shared locks serialize against ALTER TABLE exclusive metadata locks or force a definition-changed retry. A row therefore cannot be inserted behind the migration scan and remain permanently unbackfilled.
- Before origin columns exist, action provenance degrades conservatively to LEGACY_AMBIGUOUS instead of inventing precision, while ON DELETE/UPDATE behavior remains exact. After all columns commit, extended 19-column writes preserve exact binding and origin metadata.
- The public BVT oracle verifies that an unselected compatible UNIQUE is droppable, selected UNIQUE/PRIMARY keys are rejected, and referential metadata survives the rejected DDL.
Fresh local validation passed: build, vet, and complete package tests with cache disabled for pkg/bootstrap/versions/v4_0_6, pkg/sql/plan, pkg/bootstrap, pkg/sql/compile, and pkg/util/sysview. CI is still running and was not awaited; both upgrade compatibility launch jobs are currently skipped.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed the full diff at ee05af5 against merge-base ffc1603. No blocking correctness findings. The FK metadata migration, composite ordering, action provenance, referenced-index lifecycle, asynchronous tenant-upgrade compatibility, and failure-path handling are coherent. Focused tests passed for ./pkg/bootstrap/versions/v4_0_6, ./pkg/bootstrap, ./pkg/sql/plan, and ./pkg/util/sysview. The broader ./pkg/sql/compile run was blocked locally by the sandbox denying Unix socket creation in an unrelated lockservice test; PR CI is green.
# Conflicts: # pkg/bootstrap/service_upgrade_tenant.go # pkg/bootstrap/service_upgrade_tenant_test.go # pkg/bootstrap/versions/v4_0_6/upgrade_test.go # pkg/util/sysview/predefined_test.go # test/distributed/cases/database/system_table.result
Merge Queue Status
This pull request spent 1 hour 15 minutes 38 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #26469 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
6 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #25388
What this PR does / why we need it: