Skip to content

[codex] fix DropDatabase SnapshotTS restore race - #24454

Merged
XuPeng-SH merged 9 commits into
matrixorigin:mainfrom
gouhongshen:codex/fix-dropdatabase-snapshotts-race
May 28, 2026
Merged

[codex] fix DropDatabase SnapshotTS restore race#24454
XuPeng-SH merged 9 commits into
matrixorigin:mainfrom
gouhongshen:codex/fix-dropdatabase-snapshotts-race

Conversation

@gouhongshen

@gouhongshen gouhongshen commented May 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #24381.

Fixes the data race between Scope.DropDatabase restoring SnapshotTS and concurrent txnOperator.GetOverview reads.

DropDatabase still temporarily advances SnapshotTS after acquiring the exclusive mo_database lock, so Relations() continues to see tables committed before the lock was granted. The restore path now goes through a locked TxnOperator.SetSnapshotTS method instead of writing through TxnRef() directly.

This PR also fixes a test-only race in TestPushClient_PartitionStateGCTicker: the test changed the package-level gcPartitionStateTicker while the ticker goroutine could still be reading it. The ticker tests now cancel and wait for their goroutines before restoring package-level intervals.

Root Cause

The restore logic introduced after the DropDatabase/CLONE orphan-record fix wrote txnOp.TxnRef().SnapshotTS = origSnapshotTS without holding txnOperator.mu. TxnRef() only protects returning the pointer; the caller's field write remains unlocked and can race with GetOverview() copying tc.mu.txn under RLock.

The logtail ticker race was independent and test-local: TestPushClient_PartitionStateGCTicker restored gcPartitionStateTicker in a defer without waiting for the spawned ticker goroutine to exit. TestPushClient_UnusedTableGCTicker had the same lifecycle bug around unsubscribeProcessTicker, so it is fixed with the same helper.

Changes

  • Add TxnOperator.SetSnapshotTS and implement it with txnOperator.mu.Lock().
  • Use SetSnapshotTS in DropDatabase and in the existing lockop test helper path.
  • Update mocks and test fakes for the expanded interface.
  • Implement StorageTxnOperator snapshot getter/setter and add focused coverage.
  • Strengthen DropDatabase tests to assert Relations() still runs under the advanced snapshot and the original snapshot is restored only on return.
  • Make logtail ticker tests wait for spawned ticker goroutines before restoring package-level ticker durations.

Validation

  • go test ./pkg/txn/client -run TestBase -count=1
  • go test ./pkg/txn/storage/memorystorage -run TestStorageTxnOperatorSnapshotTS -count=1
  • go test ./pkg/txn/storage/memorystorage -run TestStorageTxnOperatorSnapshotTS -count=1 -coverprofile=/tmp/memorystorage_snapshot.cover
  • go test ./pkg/sql/compile -run TestDropDatabase_SnapshotAdvanceAndRestore -count=1
  • go test -race ./pkg/tests/dml/ -run TestDataBranchDiffAsFile -count=1 -timeout 600s
  • go test -race ./pkg/vm/engine/disttae -run 'TestPushClient_(PartitionStateGCTicker|UnusedTableGCTicker)$' -count=10
  • git diff --check

Backport

Backport PR: #24461 targeting 4.0-dev.

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

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

Labels

size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: data race between Scope.DropDatabase SnapshotTS restore and txnOperator.GetOverview

4 participants