Skip to content

fix(disttae): validate partitioned ODKU against the resolved locked key - #30112

Merged
XuPeng-SH merged 5 commits into
matrixorigin:mainfrom
LeftHandCold:fix/odku-key-partition-correctness-30084
Oct 13, 2026
Merged

XuPeng-SH merged 5 commits into
matrixorigin:mainfrom
LeftHandCold:fix/odku-key-partition-correctness-30084

Conversation

@LeftHandCold

@LeftHandCold LeftHandCold commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #30084

What this PR does / why we need it:

A pessimistic RC ODKU must check committed modifications of the locked, resolved row, after a secondary UNIQUE conflict resolves an incoming primary key to a different existing row. Pruning that version check with the incoming image can miss the competitor's commit and acknowledge a stale write.

Design and correction

  • Route the post-lock version check using the resolved scalar PK when every partition definition depends on that PK. Composite PKs and non-PK/unprovable expressions use the existing all-partition enumeration and version checks.
  • An explicit partition projection is usable only when all definitions share one source column; mixed dependencies conservatively check all relations.
  • Repair positional substitution in the existing partition pruner: visit root, sibling and nested references, remap all occurrences of the selected source column, and preserve unrelated columns and RowID offsets. Remove the unreachable helper shift mode so RowID shifting remains owned by metadata generation.
  • Repair the same premature-return defect in equality-read value substitution and RowID metadata generation. Check all partition definitions before substituting one filter value, preserve named/legacy positional safeguards, and propagate cast construction errors.
  • Preserve existing lock/retry ownership, lower version-check arguments/timestamps, hidden-index and upsert paths. No new cache, retry state, wire or catalog format is introduced.

The previous PR head passed its five controls but still failed repeated expressions. On HASH(id+id), RANGE(id+id), RANGE(id*id) and RANGE((id+id)*id), two acknowledged writes produced two (1,10,2) rows instead of one (1,10,3). A separate sequential-write experiment also exposed PK equality-read errors from incomplete constant substitution. Both defects were independently reproduced on macOS/ARM64 before repair.

Regression coverage and validation

Rebased without conflicts onto main 8aaa8ae9e678f5d2c6fc52aca3b1d675b9d17a2c. Current-code validation uses Go 1.27.1 on macOS/ARM64 with verified CPU native-artifact provenance.

Check Result
Deterministic public SQL, nine schemas Normal and race pass; full scan, PK and UNIQUE lookup each return exactly one (1,10,3)
Barrier cancellation and writer failure Pass; owned writer/callback join, hooks reset, subsequent operation succeeds
Full partitionprune / partitionservice / disttae UT Normal and race pass: 592 root tests including the final mixed-column evaluation delta
Direct lockop / insert / deletion / multi_update consumers 20 selected partition root tests pass normally and under race
Existing ODKU BVT ODKU, edge, modern and resolved-target lock cases pass twice: 664 effective checks per round; catalog teardown asserted and 8 existing ignored DML checks preserved
Incremental configured SCA, eight-package closure Formatting, vet, golangci-lint and parsed molint diagnostics pass
Required design and final overall review gpt-6.1-sol / xhigh; actual runtime records and review dispositions retained

Tests reuse existing fixtures and the existing before-lock hook with one seed row; no sleeps or fabricated lock results are needed for the snapshot-to-lock witness. Typed UTs evaluate bound expressions with distinct incoming/resolved vectors, leading RowID, mixed-column controls and immutable metadata. The existing lock-timeout BVT keeps its original oracle and adds repeated-expression ODKU/read/update/delete coverage.

Expression traversal is linear in expression size. Ambiguous projections can check more partitions; no throughput improvement or zero-overhead claim is made. The existing legacy PruneResult.Close/error-path batch ownership remains a separate baseline concern. This change prevents new violations; it does not repair previously duplicated rows or claim to reproduce the incident's exact 18-versus-3 counts.

Final revision: 5d85502f7deea89b92b8caba6add8352d3fa42ce. GitHub CI for this revision remains a separate, pending gate.

@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 →

@LeftHandCold

Copy link
Copy Markdown
Collaborator Author

Final self-review update: PR #30112 now points to 46628839fd1466dffa5d4dfd2c2ec6b478596cdd.

  • The follow-up changes only regression-test resource cleanup: query rows are closed with defer even if an assertion exits early. Production code and the root-cause conclusion are unchanged.
  • Final-head normal SQL regression: all five scenarios pass, 5.971s.
  • Final-head -race -count=3: all five scenarios pass in all repetitions, 15.725s.
  • Go vet and prealloc analysis pass. Additional SQL resource checks emit the same six diagnostics on pristine 21f19ae063 and patched source, all in unchanged test files; comparing the complete outputs produces no diff. No full-repository SCA pass is claimed.
  • Reviewed partition identity, scalar/composite fallback, RowID offset, explicit-index preservation, hook cancellation/join and resource cleanup. No unresolved in-scope blocker remains.

The existing BVT/MOTR and package evidence remains applicable because this follow-up does not modify production code, their fixtures or their oracles. Repository CI is still a separate gate, including the independent latest-main build problem #30108 / #30109 noted above.

@XuPeng-SH

Copy link
Copy Markdown
Contributor

Rebased onto main 8aaa8ae9e678 and fixed the blocking repeated-reference finding in 5d85502f7deea89b92b8caba6add8352d3fa42ce.

  • Independently reproduced the old-head write regression for HASH/RANGE sums, a square and a nested expression, plus the related PK-read error.
  • Corrected every relevant reference in the existing partition-pruning and RowID metadata owners; preserved conservative fallback and unrelated columns. Removed an unreachable duplicate shift branch.
  • The nine-scenario SQL regression now passes normally and under race; full scan, complete PK and UNIQUE reads agree. Cancellation/early-writer-failure cleanup also passes. The final review's P2 composite oracle gap is closed with exact (1,7,10,3) and id=1 AND shard=7.
  • 592 owning-package root tests and 20 directly affected consumer tests have normal/race coverage; final changed-test deltas were revalidated. Four ODKU BVT cases pass twice: 664 effective checks per round, 8 existing ignores retained, teardown verified. Incremental configured formatting/vet/lint/parsed molint checks pass.
  • Required design and final review used actual gpt-6.1-sol / xhigh sessions: design 01a13722-c013-79a2-afd2-7d18927fb420, overall/re-review 01a13737-8338-7c71-8442-d4cef7abb1e1. Final review reports no unresolved material blocker.

PR description updated to the repository template and current evidence. New-head GitHub CI remains separate from these completed local checks.

@matrix-meow matrix-meow added size/L Denotes a PR that changes [500,999] lines and removed size/M Denotes a PR that changes [100,499] lines labels Oct 13, 2026
@XuPeng-SH

XuPeng-SH commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

mo-bot review · mo-bot-local · done

Commit: 5d85502f7deea89b92b8caba6add8352d3fa42ce

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mo-bot review · mo-bot-local · review

Commit: 5d85502f7deea89b92b8caba6add8352d3fa42ce

Assessment: approve

Reviewed the complete 11-file delta and latest comments. Resolved-key routing, conservative partition fallback, and complete expression traversal close the previous correctness finding. No actionable defects remain.

Model: gpt-6-astra / xhigh

Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0

Validation:

  • Linux/amd64, Go 1.27.1: isolation preflight and native build/provenance passed; assigned source remained unchanged.

  • All 592 tests across partitionprune, partitionservice and disttae, plus 20 partition-consumer tests, passed normally and under the race detector.

  • Nine-schema concurrent ODKU regression passed normally, under race, and in two additional race repetitions, including cancellation and early-failure cleanup.

  • Exact changed BVT passed twice: 26/26 checks each, zero ignored, with teardown verified between runs. Initial harness failures occurred before SQL execution and were resolved.

  • Formatting, vet, incremental golangci-lint and molint passed.

  • Commands, source fingerprints, exit receipts and retained logs reconciled in /home/xupeng/.local/state/mo-bot/jobs/0d7257ef7c70dc4e1bea6499/attempt-1/validation-audit.json.

Limitations:

  • Full repository CI and the original production workload were not replayed; acceptance uses verified current-commit local evidence.

  • Conservative all-partition fallback can increase work; throughput was not benchmarked.

@mergify

mergify Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • ✅ Entered queue — 2026-10-13 23:26 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • 🟠 Checks running · in-place
  • 🚫 Left the queue — 2026-10-13 23:30 UTC · at c288b4c5b63e04a530096aa0ecc2af64ae806c44

This pull request spent 4 minutes 13 seconds in the queue, with no time running CI.

Reason

Pull request #30112 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.

The author needs to enable "Allow edits from maintainers" on this pull request.

Hint

You 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.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Waiting for any of
  • check-neutral = CI Required
  • check-skipped = CI Required
  • check-success = CI Required
All conditions

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request

@XuPeng-SH
XuPeng-SH merged commit b468f16 into matrixorigin:main Oct 13, 2026
18 of 21 checks passed
@mergify mergify Bot added dequeued and removed queued labels Oct 13, 2026

This branch had an error being deployed

1 failed deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dequeued kind/bug Something isn't working size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Concurrent UNIQUE-key ODKU returns duplicate visible rows on a KEY-partitioned table

3 participants