WritePrepared: release snapshot equal to max #4944
Conversation
There was a problem hiding this comment.
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| if (snap_seq < max_evicted_seq_.load(std::memory_order_relaxed)) { | ||
| // TODO(myabandeh): relax should enough since the synchronizatin is already | ||
| // done by snapshots_mutex_ under which this function is called. | ||
| if (snap_seq <= max_evicted_seq_.load(std::memory_order_acquire)) { |
There was a problem hiding this comment.
I actually don't follow why we needed to change from relaxed => acquire?
There was a problem hiding this comment.
The algorithms were initially designed to allow concurrent call to AdvanceMaxEvictedSeq which has added too much unnecessary complexity. We should get rid of all that in a future refactoring.
For the moment my concern was that that if max is already advanced by another thread, and our cpu cache is not updated with that, then with memory_order_relaxed we might fail to properly cleanup the old_commit_map_ for the released snapshot. I do not think this would happen in the current code but it is easier to prove the code is correct with acquire than with relaxed.
WritePrepared maintains a list of snapshots that are <= max_evicted_seq_. Based on this list, old_commit_map_ is updated if an evicted commit entry overlaps with such snapshot. Such lists are garbage collected when the release of snapshot is reported to WritePreparedTxnDB, which is the next time max_evicted_seq_ is updated and yet the snapshot is not found is the list returned from DB. This logic was broken since ReleaseSnapshotInternal was using "< max_evicted_seq_" to cleanup old_commit_map_, which would leave a snapshot uncleaned if it "= max_evicted_seq_". The patch fixes that and adds a unit test to check for the bug.