feat(p4): refactor table heap + tuple for project 4 (#542)

skyzh · web-flow · commit 5d7a9ac19ea5 · 2023-03-19T23:58:57.000-04:00
* feat(p4): refactor table heap + tuple for project 4

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* verify p3

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* fix

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* fmt

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* impl hack

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* fix

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* fix

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* fix race

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* fix race

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

* fix race

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;

---------

Signed-off-by: Alex Chi &lt;iskyzh@gmail.com&gt;
diff --git a/src/catalog/table_generator.cpp b/src/catalog/table_generator.cpp
@@ -3,6 +3,7 @@
 #include <algorithm>
 #include <random>
 #include <vector>
+#include "common/config.h"
 
 namespace bustub {
 
@@ -76,7 +77,8 @@ void TableGenerator::FillTable(TableInfo *info, TableInsertMeta *table_meta) {
       for (const auto &col : values) {
         entry.emplace_back(col[i]);
       }
-      auto rid = info->table_->InsertTuple(TupleMeta{0, 0, false}, Tuple(entry, &info->schema_));
+      auto rid =
+          info->table_->InsertTuple(TupleMeta{INVALID_TXN_ID, INVALID_TXN_ID, false}, Tuple(entry, &info->schema_));
       BUSTUB_ENSURE(rid != std::nullopt, "Sequential insertion cannot fail");
       num_inserted++;
     }
diff --git a/src/concurrency/lock_manager.cpp b/src/concurrency/lock_manager.cpp
@@ -26,7 +26,9 @@ auto LockManager::LockRow(Transaction *txn, LockMode lock_mode, const table_oid_
   return true;
 }
 
-auto LockManager::UnlockRow(Transaction *txn, const table_oid_t &oid, const RID &rid) -> bool { return true; }
+auto LockManager::UnlockRow(Transaction *txn, const table_oid_t &oid, const RID &rid, bool force) -> bool {
+  return true;
+}
 
 void LockManager::AddEdge(txn_id_t t1, txn_id_t t2) {}
 
diff --git a/src/include/concurrency/lock_manager.h b/src/include/concurrency/lock_manager.h
@@ -260,9 +260,10 @@ class LockManager {
    * @param rid the RID that is locked by the transaction
    * @param oid the table_oid_t of the table the row belongs to
    * @param rid the RID of the row to be unlocked
+   * @param force unlock the tuple regardless of isolation level, not changing the transaction state
    * @return true if the unlock is successful, false otherwise
    */
-  auto UnlockRow(Transaction *txn, const table_oid_t &oid, const RID &rid) -> bool;
+  auto UnlockRow(Transaction *txn, const table_oid_t &oid, const RID &rid, bool force = false) -> bool;
 
   /*** Graph API ***/
 
diff --git a/src/include/concurrency/transaction.h b/src/include/concurrency/transaction.h
@@ -65,15 +65,15 @@ using index_oid_t = uint32_t;
 class TableWriteRecord {
  public:
   // NOLINTNEXTLINE
-  TableWriteRecord(RID rid, WType wtype, const Tuple &tuple, TableHeap *table)
-      : rid_(rid), wtype_(wtype), tuple_(tuple), table_(table) {}
+  TableWriteRecord(table_oid_t tid, RID rid, TableHeap *table_heap) : tid_(tid), rid_(rid), table_heap_(table_heap) {}
 
+  table_oid_t tid_;
   RID rid_;
+  TableHeap *table_heap_;
+
+  // Recording write type might be useful if you want to implement in-place update for leaderboard
+  // optimization. You don't need it for the basic implementation.
   WType wtype_;
-  /** The tuple is only used for the update operation. */
-  Tuple tuple_;
-  /** The table heap specifies which table this write record is for. */
-  TableHeap *table_;
 };
 
 /**
@@ -86,6 +86,11 @@ class IndexWriteRecord {
                    Catalog *catalog)
       : rid_(rid), table_oid_(table_oid), wtype_(wtype), tuple_(tuple), index_oid_(index_oid), catalog_(catalog) {}
 
+  /**
+   * Note(spring2023): I don't know what are these for. If you are implementing leaderboard optimizations, you can
+   * figure out how to use this structure to store what you need.
+   */
+
   /** The rid is the value stored in the index. */
   RID rid_;
   /** Table oid. */
diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
@@ -12,8 +12,6 @@
 #include "execution/expressions/abstract_expression.h"
 #include "execution/plans/abstract_plan.h"
 
-#define BUSTUB_OPTIMIZER_HACK_REMOVE_AFTER_2022_FALL
-
 namespace bustub {
 
 /**
diff --git a/src/include/storage/page/table_page.h b/src/include/storage/page/table_page.h
@@ -85,6 +85,11 @@ class TablePage {
    */
   auto GetTuple(const RID &rid) const -> std::pair<TupleMeta, Tuple>;
 
+  /**
+   * Read a tuple meta from a table.
+   */
+  auto GetTupleMeta(const RID &rid) const -> TupleMeta;
+
   /**
    * Update a tuple in place.
    */
diff --git a/src/include/storage/table/table_heap.h b/src/include/storage/table/table_heap.h
@@ -60,20 +60,26 @@ class TableHeap {
   /**
    * Read a tuple from the table.
    * @param rid rid of the tuple to read
-   * @param tuple output variable for the tuple
-   * @return the tuple
+   * @return the meta and tuple
    */
   auto GetTuple(RID rid) -> std::pair<TupleMeta, Tuple>;
 
+  /**
+   * Read a tuple meta from the table. Note: if you want to get tuple and meta together, use `GetTuple` insead
+   * to ensure atomicity.
+   * @param rid rid of the tuple to read
+   * @return the meta
+   */
+  auto GetTupleMeta(RID rid) -> TupleMeta;
+
   /** @return the iterator of this table */
   auto MakeIterator() -> TableIterator;
 
   /** @return the id of the first page of this table */
   inline auto GetFirstPageId() const -> page_id_t { return first_page_id_; }
 
   /**
-   * Update a tuple in place. SHOULD BE ONLY USED IN PROJECT 3. Only tuple of the same
-   * size is allowed.
+   * Update a tuple in place. SHOULD NOT BE USED UNLESS YOU WANT TO OPTIMIZE FOR PROJECT 4.
    * @param meta new tuple meta
    * @param tuple  new tuple
    * @param[out] rid the rid of the tuple to be updated
diff --git a/src/include/storage/table/table_iterator.h b/src/include/storage/table/table_iterator.h
@@ -34,7 +34,7 @@ class TableIterator {
  public:
   DISALLOW_COPY(TableIterator);
 
-  TableIterator(TableHeap *table_heap, RID rid);
+  TableIterator(TableHeap *table_heap, RID rid, RID stop_at_rid);
   TableIterator(TableIterator &&) = default;
 
   ~TableIterator() = default;
@@ -50,6 +50,11 @@ class TableIterator {
  private:
   TableHeap *table_heap_;
   RID rid_;
+
+  // When creating table iterator, we will record the maximum RID that we should scan.
+  // Otherwise we will have dead loops when updating while scanning. (In project 4, update should be implemented as
+  // deletion + insertion.)
+  RID stop_at_rid_;
 };
 
 }  // namespace bustub
diff --git a/src/optimizer/eliminate_true_filter.cpp b/src/optimizer/eliminate_true_filter.cpp
@@ -8,8 +8,6 @@
 
 namespace bustub {
 
-#ifdef BUSTUB_OPTIMIZER_HACK_REMOVE_AFTER_2022_FALL
-
 auto Optimizer::OptimizeEliminateTrueFilter(const AbstractPlanNodeRef &plan) -> AbstractPlanNodeRef {
   std::vector<AbstractPlanNodeRef> children;
   for (const auto &child : plan->GetChildren()) {
@@ -29,6 +27,4 @@ auto Optimizer::OptimizeEliminateTrueFilter(const AbstractPlanNodeRef &plan) ->
   return optimized_plan;
 }
 
-#endif
-
 }  // namespace bustub
diff --git a/src/optimizer/merge_filter_scan.cpp b/src/optimizer/merge_filter_scan.cpp
@@ -10,8 +10,6 @@
 
 namespace bustub {
 
-#ifdef BUSTUB_OPTIMIZER_HACK_REMOVE_AFTER_2022_FALL
-
 auto Optimizer::OptimizeMergeFilterScan(const AbstractPlanNodeRef &plan) -> AbstractPlanNodeRef {
   std::vector<AbstractPlanNodeRef> children;
   for (const auto &child : plan->GetChildren()) {
@@ -36,6 +34,4 @@ auto Optimizer::OptimizeMergeFilterScan(const AbstractPlanNodeRef &plan) -> Abst
   return optimized_plan;
 }
 
-#endif
-
 }  // namespace bustub
diff --git a/src/storage/page/table_page.cpp b/src/storage/page/table_page.cpp
@@ -81,6 +81,15 @@ auto TablePage::GetTuple(const RID &rid) const -> std::pair<TupleMeta, Tuple> {
   return std::make_pair(meta, std::move(tuple));
 }
 
+auto TablePage::GetTupleMeta(const RID &rid) const -> TupleMeta {
+  auto tuple_id = rid.GetSlotNum();
+  if (tuple_id >= num_tuples_) {
+    throw bustub::Exception("Tuple ID out of range");
+  }
+  auto &[_1, _2, meta] = tuple_info_[tuple_id];
+  return meta;
+}
+
 void TablePage::UpdateTupleInPlaceUnsafe(const TupleMeta &meta, const Tuple &tuple, RID rid) {
   auto tuple_id = rid.GetSlotNum();
   if (tuple_id >= num_tuples_) {
diff --git a/src/storage/table/table_heap.cpp b/src/storage/table/table_heap.cpp
@@ -50,19 +50,29 @@ auto TableHeap::InsertTuple(const TupleMeta &meta, const Tuple &tuple) -> std::o
     page_id_t next_page_id = INVALID_PAGE_ID;
     auto npg = bpm_->NewPage(&next_page_id);
     BUSTUB_ENSURE(next_page_id != INVALID_PAGE_ID, "cannot allocate page");
+
+    // Don't do lock crabbing here: TSAN reports, also as last_page_id_ is only updated
+    // later, this page won't be accessed.
+    page->SetNextPageId(next_page_id);
+    page_guard.Drop();
+
     npg->WLatch();
     auto next_page_guard = WritePageGuard{bpm_, npg};
     auto next_page = next_page_guard.AsMut<TablePage>();
     next_page->Init();
-    page->SetNextPageId(next_page_id);
+
     last_page_id_ = next_page_id;
     page_guard = std::move(next_page_guard);
   }
+  auto last_page_id = last_page_id_;
   guard.unlock();
 
   auto page = page_guard.AsMut<TablePage>();
   auto slot_id = *page->InsertTuple(meta, tuple);
-  return RID(last_page_id_, slot_id);
+
+  page_guard.Drop();
+
+  return RID(last_page_id, slot_id);
 }
 
 void TableHeap::UpdateTupleMeta(const TupleMeta &meta, RID rid) {
@@ -79,7 +89,21 @@ auto TableHeap::GetTuple(RID rid) -> std::pair<TupleMeta, Tuple> {
   return std::make_pair(meta, std::move(tuple));
 }
 
-auto TableHeap::MakeIterator() -> TableIterator { return {this, {first_page_id_, 0}}; }
+auto TableHeap::GetTupleMeta(RID rid) -> TupleMeta {
+  auto page_guard = bpm_->FetchPageRead(rid.GetPageId());
+  auto page = page_guard.As<TablePage>();
+  return page->GetTupleMeta(rid);
+}
+
+auto TableHeap::MakeIterator() -> TableIterator {
+  std::unique_lock<std::mutex> guard(latch_);
+  auto last_page_id = last_page_id_;
+  guard.unlock();
+
+  auto page_guard = bpm_->FetchPageRead(last_page_id);
+  auto page = page_guard.As<TablePage>();
+  return {this, {first_page_id_, 0}, {last_page_id, page->GetNumTuples()}};
+}
 
 void TableHeap::UpdateTupleInPlaceUnsafe(const TupleMeta &meta, const Tuple &tuple, RID rid) {
   auto page_guard = bpm_->FetchPageWrite(rid.GetPageId());
diff --git a/src/storage/table/table_iterator.cpp b/src/storage/table/table_iterator.cpp
@@ -20,7 +20,8 @@
 
 namespace bustub {
 
-TableIterator::TableIterator(TableHeap *table_heap, RID rid) : table_heap_(table_heap), rid_(rid) {
+TableIterator::TableIterator(TableHeap *table_heap, RID rid, RID stop_at_rid)
+    : table_heap_(table_heap), rid_(rid), stop_at_rid_(stop_at_rid) {
   // If the rid doesn't correspond to a tuple (i.e., the table has just been initialized), then
   // we set rid_ to invalid.
   auto page_guard = table_heap_->bpm_->FetchPageRead(rid_.GetPageId());
@@ -40,13 +41,27 @@ auto TableIterator::operator++() -> TableIterator & {
   auto page_guard = table_heap_->bpm_->FetchPageRead(rid_.GetPageId());
   auto page = page_guard.As<TablePage>();
   auto next_tuple_id = rid_.GetSlotNum() + 1;
-  if (next_tuple_id < page->GetNumTuples()) {
-    rid_ = RID{rid_.GetPageId(), next_tuple_id};
+
+  BUSTUB_ASSERT(
+      /* case 1: cursor before the page of the stop tuple */ rid_.GetPageId() < stop_at_rid_.GetPageId() ||
+          /* case 2: cursor at the page before the tuple */
+          (rid_.GetPageId() == stop_at_rid_.GetPageId() && next_tuple_id <= stop_at_rid_.GetSlotNum()),
+      "iterate out of bound");
+
+  rid_ = RID{rid_.GetPageId(), next_tuple_id};
+
+  if (rid_ == stop_at_rid_) {
+    rid_ = RID{INVALID_PAGE_ID, 0};
+  } else if (next_tuple_id < page->GetNumTuples()) {
+    // that's fine
   } else {
     auto next_page_id = page->GetNextPageId();
     // if next page is invalid, RID is set to invalid page; otherwise, it's the first tuple in that page.
     rid_ = RID{next_page_id, 0};
   }
+
+  page_guard.Drop();
+
   return *this;
 }
 
diff --git a/test/sql/p3.03-update.slt b/test/sql/p3.03-update.slt
@@ -60,7 +60,7 @@ update t1 set v3 = 645 where v1 >= 3;
 ----
 4
 
-query
+query rowsort
 select * from t1;
 ----
 0 🥰 10
@@ -80,7 +80,7 @@ update t1 set v3 = 645 where v1 >= 5;
 ----
 0
 
-query
+query rowsort
 select * from t1;
 ----
 0 🥰 10
@@ -99,7 +99,7 @@ update t1 set v3 = v3 + v1;
 ----
 10
 
-query
+query rowsort
 select * from t1;
 ----
 0 🥰 10
diff --git a/test/table/tuple_test.cpp b/test/table/tuple_test.cpp
@@ -43,7 +43,7 @@ TEST(TupleTest, DISABLED_TableHeapTest) {
 
   std::vector<RID> rid_v;
   for (int i = 0; i < 5000; ++i) {
-    auto rid = table->InsertTuple(TupleMeta{0, 0, false}, tuple);
+    auto rid = table->InsertTuple(TupleMeta{INVALID_TXN_ID, INVALID_TXN_ID, false}, tuple);
     rid_v.push_back(*rid);
   }
 

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,9 @@ auto LockManager::LockRow(Transaction *txn, LockMode lock_mode, const table_oid_`
`26`	`26`	`return true;`
`27`	`27`	`}`
`28`	`28`
`29`		`-auto LockManager::UnlockRow(Transaction *txn, const table_oid_t &oid, const RID &rid) -> bool { return true; }`
	`29`	`+auto LockManager::UnlockRow(Transaction *txn, const table_oid_t &oid, const RID &rid, bool force) -> bool {`
	`30`	`+ return true;`
	`31`	`+}`
`30`	`32`
`31`	`33`	`void LockManager::AddEdge(txn_id_t t1, txn_id_t t2) {}`
`32`	`34`