Skip to content

fix: retry stale regular index hints across CNs - #28641

Open
gouhongshen wants to merge 2 commits into
matrixorigin:mainfrom
gouhongshen:agent/issue-28639-94d7deef
Open

fix: retry stale regular index hints across CNs#28641
gouhongshen wants to merge 2 commits into
matrixorigin:mainfrom
gouhongshen:agent/issue-28639-94d7deef

Conversation

@gouhongshen

@gouhongshen gouhongshen commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • BUG
  • Feature
  • Enhancement
  • Documentation
  • Test
  • Refactor
  • Performance
  • Chore

Which issue(s) this PR fixes:

Related to #28639

What this PR does / why we need it:

A verifier CN can plan a regular FORCE INDEX query from a table definition received before a concurrent CREATE INDEX. The planner then returned MySQL 1176 before the compiler could take its existing metadata lock and request a definition-change retry.

This PR adds an additive query-plan marker for an unresolved permanent index hint and carries the parent table object reference. Compilation takes the existing metadata-lock path for that marker. A committed DDL change rebuilds the statement from the current definition; without a definition change, compilation returns the original 1176 before any data source starts. Temporary tables, prepared statements, and metadata-unlocked scans retain immediate planner validation. Sirius offload rejects marked plans so it cannot bypass the native metadata boundary.

The CI follow-up makes the terminal missing-index error select the first recorded unresolved hint directly rather than using a range whose first iteration always returned. It preserves the error contract and satisfies staticcheck without adding a retry or fallback path.

Rebased onto origin/main at ee0c2bf563bf27ff2c80891bb57a1de5886bf90b. The generated plan binding was regenerated from the merged schema during rebase, preserving both upstream schema changes and this PR’s additive marker; current head: 435e98df14.

Issue-to-test proof:

  • TestIssue27487ConcurrentInsertIsIncludedInNewIndex/regular_secondary_index pauses a verifier CN after it has compiled a regular-index query from the old definition, releases the writer so CREATE INDEX commits, then proves the retried query and EXPLAIN FORCE INDEX use idx_k and include the concurrent committed row.
  • regular_secondary_index_writer_rollback exercises the same DDL boundary with a rolled-back writer and proves the new index is visible while neither forced nor base-table reads expose the uncommitted row.
  • Planner tests cover USE, FORCE, and IGNORE missing hints; protobuf round-trip; temporary-table and PREPARE immediate 1176 behavior; and order-scoped FORCE. Compiler tests cover parent metadata locking, first-hint error preservation across multiple markers, and the empty-marker path.

Tests:

  • make generate-pb
  • .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 ./pkg/pb/plan
  • .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 ./pkg/sql/plan
  • .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 ./pkg/sql/compile
  • .agents/skills/mo-dev/scripts/mo-cgo-test -v -count=1 -timeout=10m -run ^TestIssue27487ConcurrentInsertIsIncludedInNewIndex$ ./pkg/tests/issues
  • .agents/skills/mo-dev/scripts/mo-cgo-test -race -v -count=2 -timeout=8m -run ^TestIssue27487ConcurrentInsertIsIncludedInNewIndex$ ./pkg/tests/issues
  • .agents/skills/mo-dev/scripts/mo-cgo-test -race -count=1 ./pkg/sql/compile
  • CGO_CFLAGS="-I$PWD/cgo -I$PWD/thirdparties/install/include" GOWORK=off go vet -mod=readonly ./pkg/pb/plan ./pkg/sql/compile ./pkg/sql/plan ./pkg/tests/issues
  • CGO_CFLAGS="-I$PWD/cgo -I$PWD/thirdparties/install/include" golangci-lint run --concurrency 1 -c .golangci.yml ./pkg/sql/compile

BVT was not applicable: the proof requires two authenticated embedded CN connections, a held writer transaction, and a deterministic compiler fault boundary, which the single-endpoint mo-tester workflow cannot express.

Residual risks:

The new protobuf field is additive and is covered by generated-code and marshal/unmarshal tests. The production retry uses the existing metadata-lock definition-change path; only an unresolved permanent, lockable index hint enters that path.

@mergify mergify Bot added the kind/bug Something isn't working label Sep 10, 2026
@matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Sep 10, 2026
@gouhongshen
gouhongshen force-pushed the agent/issue-28639-94d7deef branch from e5a9038 to c30779e Compare September 10, 2026 14:52
@gouhongshen

Copy link
Copy Markdown
Contributor Author

Unrelated CI blockers from MatrixOne ALL CI run 34491898016 (attempt 1):

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@gouhongshen
gouhongshen force-pushed the agent/issue-28639-94d7deef branch from 435e98d to b264c48 Compare September 11, 2026 05:48
@gouhongshen
gouhongshen force-pushed the agent/issue-28639-94d7deef branch from b264c48 to dd95a4d Compare September 11, 2026 06:10
@gouhongshen
gouhongshen force-pushed the agent/issue-28639-94d7deef branch from dd95a4d to 2f7921e Compare September 11, 2026 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants