memdb: retain old version nodes of ART to satisfy snapshot read

[you06]

ref pingcap/tidb#57425

Changes

The snapshot iterators always read from a snapshot of MemBuffer, but writes between Next calls can alter the structure of ART (see the "but explanation" section for details), potentially causing the snapshot iterator to read incorrect results.

This PR introduces a counter for active snapshots. When the counter is greater than 0, it indicates that old versions need to be retained for snapshot reads. In such cases, we store freed nodes in unused slices and delay the actual free operation to prevent them from being reused.

Add a counter for

Bug explanation

1. The snapshot iterator scans to node (click to expand the figure)

  │                                 │ 
  │        iterator range           │ 
  ◄─────────────────────────────────► 
  │                                 │ 
  │                                 │ 
  │            ┌────────┐           │ 
  │            │        │           │ 
  │            │  root  │           │ 
  │            │        │           │ 
  │            └────┬───┘           │ 
  │                 │               │ 
  │         ┌───────┴───────┐       │ 
  │         │               │       │ 
  │    ┌────▼───┐      ┌────▼───┐   │ 
  │    │        │      │        │   │ 
  │    │    1   │      │    2   │   │ 
  │    │        │      │        │   │ 
  │    └─────▲──┘      └────────┘   │ 
  │          │                      │ 
  │          │                      │ 
  │          │                      │ 
  │          │                      │ 
  │          │                      │ 
  │    ┌─────┴───┐                  │ 
  │    │ snapshot│                  │ 
  │    │ iterator│                  │ 
  │    └─────────┘                  │ 
  │                                 │ 

2. The node 1 grows to larger capacity (node 3) due to coming writes, and the node1 is reused (click to expand the figure)

 │                                 │                
 │        iterator range           │                
 ◄─────────────────────────────────►                
 │                                 │                
 │                                 │                
 │            ┌────────┐           │                
 │            │        │           │                
 │            │  root  │           │                
 │            │        │           │                
 │            └────┬───┘           │                
 │                 │               │                
 │         ┌───────┴───────┬───────┼─────────┐      
 │         │               │       │         │      
 │    ┌────▼───┐      ┌────▼───┐   │    ┌────▼───┐  
 │    │        │      │        │   │    │        │  
 │    │    3   │      │    2   │   │    │    1   │  
 │    │        │      │        │   │    │        │  
 │    └────────┘      └────────┘   │    └─────▲──┘  
 │                                 │          │     
 │                                 │          │     
 │                                 │          │     
 │                                 │          │     
 │                                 │          │     
 │                                 │    ┌─────┴───┐ 
 │                                 │    │ snapshot│ 
 │                                 │    │ iterator│ 
 │                                 │    └─────────┘ 
 │                                 │                

3. The iterators will return keys out of the given range in following Next call.

[cfzjywxk]

Ditto.

[ekexium]

The fix looks OK. I just have some questions:

1. In your example, which variable in the iterator is pointing to node-1? Is it nodes in baseIter?
2. Why are we only fixing for SnapshotIter? What about the ART Iterator? Is it currently safe in implementation but not guaranteed by design?
3. Are there any other inner structure change that could lead to iterator invalidation, other than the free nodes? I don't see other cases at first glance. Have you verified this as well?

[you06]

• In your example, which variable in the iterator is pointing to node-1? Is it nodes in baseIter?

Yes, nodes[len(nodes) - 1] is node1 in the example.

• Why are we only fixing for SnapshotIter? What about the ART Iterator?

The SnapshotIter can be explained always read from the snapshot, so we need to protect it against the later writes.

For Iterator, there should be no writes during the iterator, unless the result makes no sense.

Is it currently safe in implementation but not guaranteed by design?

Yes, such usage in TiDB is out of my expectation. For long-term, we may deprecate the SnapshotIter and replace it with SnapshotScan which returns all the rows in one call.

• Are there any other inner structure change that could lead to iterator invalidation, other than the free nodes? I don't see other cases at first glance. Have you verified this as well?

I don't see other cases also. GetSnapshotValue can filter out the new added keys or versions.

[cfzjywxk]

Rest LGTM

[cfzjywxk]

Better to Close the iter following the requirement usage pattern in test.

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2024-11-19 14:06:12.450469174 +0000 UTC m=+969934.641338172: ☑️ agreed by cfzjywxk.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk

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:

• OWNERS [cfzjywxk]

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

[cfzjywxk]

@you06
Please also ensure the read-with-write test cases are covered in the PR for tidb repo.