Skip to content

[fix](be) Avoid rowset map race in backend tablets scanner#65288

Open
zclllyybb wants to merge 1 commit into
apache:masterfrom
zclllyybb:codex/doris-26859-schema-tablets-lock
Open

[fix](be) Avoid rowset map race in backend tablets scanner#65288
zclllyybb wants to merge 1 commit into
apache:masterfrom
zclllyybb:codex/doris-26859-schema-tablets-lock

Conversation

@zclllyybb

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: N/A

Related PR: N/A

Problem Summary:

information_schema.backend_tablets reads tablet rowset metadata while compaction can rewrite the rowset maps under the tablet header lock. The scanner also derives fields such as tablet size, version count, segment count, compaction score, update time, and overlap state from those rowset maps or the max-version rowset.

This PR takes a short shared tablet header lock per tablet, snapshots the rowset metadata pointers, row-binlog metadata pointers, max-version rowset pointer, and scalar tablet fields, then releases the lock before doing aggregation and filling the result block. This keeps the scanner synchronized with compaction without holding the lock during block materialization.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
      • git diff --check
      • GIT_ALLOW_PROTOCOL=file ./build.sh --be -j 90
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@zclllyybb zclllyybb marked this pull request as ready for review July 7, 2026 02:16
Root cause:\nThe backend tablets schema scanner reads tablet rowset maps while compaction can rewrite the same maps under the tablet header lock. Some columns also depend on the max-version rowset and rowset-derived compaction metadata, so reading them without the lock can race with compaction.\n\nFix:\nTake a short shared header lock per tablet, snapshot the rowset metadata pointers, row-binlog metadata pointers, max-version rowset pointer, and scalar tablet fields, then release the lock before aggregating sizes, counts, compaction score, overlap state, and filling the result block. This uses the same synchronization boundary as compaction while keeping the lock hold time small.\n\nVerification:\n- git diff --check\n- GIT_ALLOW_PROTOCOL=file ./build.sh --be -j 90
@zclllyybb zclllyybb force-pushed the codex/doris-26859-schema-tablets-lock branch from 6a4f528 to 7b451b2 Compare July 7, 2026 02:36
@zclllyybb

Copy link
Copy Markdown
Contributor Author

run buildall

@gavinchou

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Jul 7, 2026
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@github-actions github-actions Bot 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.

Automated review summary for PR 65288.

I reviewed the single changed file, be/src/information_schema/schema_tablets_scanner.cpp, plus the surrounding tablet, rowset metadata, local tablet, cloud tablet, FE schema, and regression-test context. I did not find a substantiated issue that needs an inline review comment.

Critical checkpoint conclusions:

  • Goal and tests: the PR snapshots backend-tablets scanner rowset metadata under the tablet header lock to avoid races with compaction. The implementation does that and preserves the prior backend_tablets output formulas. Existing coverage includes backend-tablets shape and cloud tablet size semantics; I did not run tests in this checkout.
  • Scope: the change is focused to the backend tablets scanner and does not introduce unrelated behavior.
  • Concurrency and lifecycle: the relevant concurrency is schema scan reads versus compaction/publish/sync mutation of tablet rowset maps. The scanner now copies rowset meta shared pointers and the max-version rowset while holding the same header lock used by those mutations, then releases it before aggregation and block filling.
  • Config, compatibility, and protocol: no new configs, persisted formats, FE-BE thrift fields, or schema columns are introduced.
  • Parallel paths: local and cloud tablet paths remain covered. Cloud IS_USED still returns true as before, and local mode still uses Tablet::is_used().
  • Data writes and persistence: this is a read-only information-schema scanner path; transaction and persistence write paths are not changed.
  • Performance and observability: the per-tablet lock scope is limited to snapshot copying. Heavy aggregation and block writes happen after unlock. No new observability looked necessary for this race fix.
  • Validation: static review only. .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc are missing in this runner, so I did not run BE build or regression tests. git show --check for the changed file reported no whitespace errors.

User focus: no additional user-provided review focus was supplied.

Subagent conclusions:

  • optimizer-rewrite: no candidates became inline comments; convergence round 1 returned NO_NEW_VALUABLE_FINDINGS.
  • tests-session-config: no candidates became inline comments; convergence round 1 returned NO_NEW_VALUABLE_FINDINGS.
  • No duplicates were merged, and the final proposed inline comment set was empty.

@zclllyybb

Copy link
Copy Markdown
Contributor Author

Branch-4.0 backport PR: #65301

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 0.00% (0/91) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 56.54% (23182/40998)
Line Coverage 40.11% (224897/560766)
Region Coverage 36.12% (177770/492148)
Branch Coverage 37.13% (78941/212583)

@gavinchou

Copy link
Copy Markdown
Contributor

skip check_coverage

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

Labels

approved Indicates a PR has been approved by one committer. dev/4.0.7-merged dev/4.1.x reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants