Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memdb: prevent iterator invalidation #1563

Merged
merged 15 commits into from
Feb 19, 2025

Conversation

ekexium
Copy link
Contributor

@ekexium ekexium commented Jan 23, 2025

ref pingcap/tidb#59153

To prevent potential misuse and iterator invalidation, modify the iterators provided by ART memdb as follows:

  1. Iter and IterReverse now comes with an extra check: it is invalidated immediately by any write operation to the memdb after the creation of the iterator. Attempting to use such an invalidated iterator will result in a panic.
  2. SnapshotIter and SnapshotIterReverse will be replaced by BatchedSnapshotIter.
    2.1. SnapshotIter is different from Iter that it can be valid after write operations, but only becomes invalid if a write operation modifies the "snapshot".
    2.2. We need to introduce BatchedSnapshotIter instead of directly modifying SnapshotIter because SnapshotIter maintains internal states and pointers. Consider a situation where a write operation causes changes to the internal data structure making the pointers invalid, while the snapshot should remain valid.
    2.3. SnapshotIter and SnapshotIterReverse are not removed now for compatibility.

RBT is unchanged as it is no longer used.
Pipelined MemDB still doesn't support iterators as it was.

Performance

Iterator microbenchmark

go test -run=^$ -bench=BenchmarkSnapshotIter -benchtime=3s
goos: linux
goarch: amd64
pkg: github.com/tikv/client-go/v2/internal/unionstore
cpu: AMD Ryzen 9 9900X 12-Core Processor            
BenchmarkSnapshotIter/RBT-SnapshotIter-24                     1783           1826587 ns/op             144 B/op               2 allocs/op
BenchmarkSnapshotIter/ART-SnapshotIter-24                     1870           1718771 ns/op             496 B/op              11 allocs/op
BenchmarkSnapshotIter/ART-BatchedSnapshotIter-24              1120           2964170 ns/op          417461 B/op             370 allocs/op
BenchmarkSnapshotIter/ART-ForEachInSnapshot-24                1771           1832084 ns/op             496 B/op              11 allocs/op
PASS
ok          github.com/tikv/client-go/v2/internal/unionstore        14.598s

TiDB union scan executor
BatchedSnapshotIter

go test -run=^$ -bench=BenchmarkUnionScanRead -benchtime=10s
    3999           3063035 ns/op          793197 B/op           17578 allocs/op

SnapshotIter

go test -run=^$ -bench=BenchmarkUnionScanRead -benchtime=10s
    3862           2990516 ns/op          386942 B/op           17434 allocs/op

@ti-chi-bot ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has signed the dco. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 23, 2025
@ekexium ekexium force-pushed the memdb-prevent-iter-invalidation branch from 8ac9c7a to 1a6d100 Compare January 23, 2025 08:02
@ekexium ekexium force-pushed the memdb-prevent-iter-invalidation branch from faf4853 to 4c16a14 Compare January 23, 2025 11:30
Signed-off-by: ekexium <[email protected]>
@ekexium ekexium force-pushed the memdb-prevent-iter-invalidation branch from 916238e to e1a3b5a Compare January 23, 2025 12:27
Signed-off-by: ekexium <[email protected]>
@ekexium ekexium force-pushed the memdb-prevent-iter-invalidation branch from 74a0617 to 29fc98e Compare January 24, 2025 05:54
@ekexium ekexium marked this pull request as ready for review January 24, 2025 05:54
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2025
@ekexium ekexium requested review from you06 and cfzjywxk January 24, 2025 05:54
Signed-off-by: ekexium <[email protected]>
internal/unionstore/arena/arena.go Show resolved Hide resolved
internal/unionstore/art/art.go Outdated Show resolved Hide resolved
internal/unionstore/memdb_art.go Show resolved Hide resolved
internal/unionstore/arena/arena.go Outdated Show resolved Hide resolved
internal/unionstore/memdb_art.go Outdated Show resolved Hide resolved
internal/unionstore/union_store.go Show resolved Hide resolved
@ekexium ekexium force-pushed the memdb-prevent-iter-invalidation branch from 7faf0e1 to 941d94b Compare February 10, 2025 06:56
internal/unionstore/memdb_rbt.go Outdated Show resolved Hide resolved
internal/unionstore/memdb_test.go Show resolved Hide resolved
internal/unionstore/memdb_art.go Outdated Show resolved Hide resolved
internal/unionstore/memdb_art.go Outdated Show resolved Hide resolved
@ekexium ekexium force-pushed the memdb-prevent-iter-invalidation branch from a5e313d to b15c57a Compare February 12, 2025 08:14
Signed-off-by: ekexium <[email protected]>
@ti-chi-bot ti-chi-bot bot requested a review from you06 February 12, 2025 08:50
Copy link
Contributor

@you06 you06 left a comment

Choose a reason for hiding this comment

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

rest LGTM

internal/unionstore/art/art.go Show resolved Hide resolved
internal/unionstore/art/art.go Show resolved Hide resolved
internal/unionstore/art/art_iterator.go Outdated Show resolved Hide resolved
internal/unionstore/art/art_iterator.go Outdated Show resolved Hide resolved
internal/unionstore/memdb_art.go Outdated Show resolved Hide resolved
)
}

it.db.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we move the RLock operation before seqno check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's discussed above. SnapshotSeqNo is not supposed to be accessed concurrently. This is the property that SnapshotSeqNo must have. Should this case happen we'd better expose this race

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I get you, SnapshotSeqNo is changed in very low frequency.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ekexium
Better to add some comments to explain the usage of RLock here, others may not be familiar with the upper MemDB and underlying ART implementation and usages.

Copy link
Contributor

@cfzjywxk cfzjywxk left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Feb 19, 2025
@ti-chi-bot ti-chi-bot bot added the lgtm label Feb 19, 2025
Copy link

ti-chi-bot bot commented Feb 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, you06

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Feb 19, 2025
Copy link

ti-chi-bot bot commented Feb 19, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-02-19 06:38:18.325182833 +0000 UTC m=+1029740.721404894: ☑️ agreed by cfzjywxk.
  • 2025-02-19 07:59:29.789850576 +0000 UTC m=+1034612.186072637: ☑️ agreed by you06.

@ti-chi-bot ti-chi-bot bot merged commit ddec823 into tikv:master Feb 19, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants