Review result

dbms/src/Storages/Page/PageEntries.h

@@ -2,6 +2,7 @@
 
 #include <cassert>
 #include <optional>
+// REVIEW: mutex unused
 #include <shared_mutex>
 #include <unordered_map>
 #include <unordered_set>
@@ -232,6 +233,7 @@ void PageEntriesMixin<T>::put(PageId page_id, const PageEntry & entry)
     max_page_id = std::max(max_page_id, page_id);
 }
 
+// REVIEW: the name `move` confused me for a while, until I see this code
 template <typename T>
 void PageEntriesMixin<T>::move_normal_page(PageId normal_page_id, PageEntry entry)
 {

dbms/src/Storages/Page/PageFile.h

@@ -1,5 +1,6 @@
 #pragma once
 
+// REVIEW: mutex unused
 #include <mutex>
 #include <unordered_map>
 #include <vector>

dbms/src/Storages/Page/VersionSet/PageEntriesEdit.h

@@ -55,6 +55,10 @@ class PageEntriesEdit
     struct EditRecord
     {
         WriteBatch::WriteType type;
+        // REVIEW: small opinion: I think this is a bit over engineering, cause this struct doesn't need to serialize,
+        //   even if it need to serialize, we could use pragma packing,
+        //   or use union{char, uint32}, union can be more reliable (if we change the type of WriteType, it don't cause problem)
+        //   manually padding seems to be the worst way to do it.
         char                  _padding[7]; // 7 bytes unused since type is only 1 byte.
         PageId                page_id;
         PageId                ori_page_id;

dbms/src/Storages/Page/mvcc/VersionSet.h

@@ -24,6 +24,7 @@ template <typename T>
 struct MultiVersionCountable
 {
 public:
+    // REVIEW: thit two status: <this is valid, ref count> are related, and should be under a same mutex
     std::atomic<uint32_t> ref_count;
     T *                   next;
     T *                   prev;
@@ -41,9 +42,11 @@ struct MultiVersionCountable
 
     void incrRefCount() { ++ref_count; }
 
+    // REVIEW: MS used the method names of increase/release, is a stander name, we could use it too.
     void decrRefCount(std::shared_mutex & mutex)
     {
         assert(ref_count >= 1);
+        // REVIEW: potential bug: 2 threads detected that ref_count == 1, then both execute delete (execute one by one cause by mutex)
         if (--ref_count == 0)
         {
             // in case two neighbor nodes remove from linked list
@@ -105,6 +108,7 @@ class VersionSet
 
     virtual ~VersionSet()
     {
+        // REVIEW: why this doesn't need the mutex?
         current->decrRefCount();
         assert(placeholder_node.next == &placeholder_node); // List must be empty
     }
@@ -140,6 +144,7 @@ class VersionSet
         return sz;
     }
 
+    // REVIEW: I read the tests, and I think lock it up should be better
     std::string toDebugStringUnlocked() const
     {
         std::string s;
@@ -183,9 +188,11 @@ class VersionSet
     }
 
 protected:
+    // REVIEW: why can't we just use std::list?
     VersionType placeholder_node; // Head of circular double-linked list of all versions
     VersionPtr  current;          // current version; current == placeholder_node.prev
 
+    // REVIEW: this name is not accurate, cause this mutex also be used for editing
     mutable std::shared_mutex read_mutex;
 
 protected:

dbms/src/Storages/Page/mvcc/VersionSetWithDelta.h

@@ -131,6 +131,7 @@ class VersionSetWithDelta
             CurrentMetrics::add(CurrentMetrics::PSMVCCNumSnapshots);
         }
 
+        // REVIEW: make sure Snapshot is uncopyable, or this will go wrong
         ~Snapshot()
         {
             vset->compactOnDeltaRelease(view.transferTailVersionOwn());
@@ -292,6 +293,7 @@ class VersionSetWithDelta
         tail.reset();
     }
 
+    // REVIEW: maybe relocate this two methods to VersionType?
     virtual VersionPtr compactDeltas(const VersionPtr & tail) const = 0;
 
     virtual VersionPtr compactDeltaAndBase(const VersionPtr & old_base, const VersionPtr & delta) const = 0;
@@ -341,6 +343,7 @@ class VersionSetWithDelta
     }
 
 protected:
+    // REVIEW: this name is not accurate, cause this mutex also be used for editing
     mutable std::shared_mutex    read_mutex;
     VersionPtr                   current;
     SnapshotPtr                  snapshots;