-
Notifications
You must be signed in to change notification settings - Fork 623
[15721] TileGroup Compaction and GC Fixes #1349
base: master
Are you sure you want to change the base?
Changes from all commits
53d21cf
5cf3a0f
5a9b0ef
8800842
729f96e
0d93313
ded93a7
469ae2c
39e0727
1be98a5
0750a84
3f592d5
1242ec3
22087ac
84efd9e
8f9f25a
c07fb50
2eb08c2
a1dbd5a
118008b
cd01623
b81817c
d6d90cc
04353ad
21c7874
01db881
ad604a2
e35d818
7b3218b
a2baf6a
c45adff
f79660c
925e3c1
62521cb
a3c2d56
112ec8c
87281e3
3da3d05
4777dc8
8f1c840
f5e9ed0
da2879b
6331fd4
f83be68
238abed
e77979d
9f1dd1d
18f9de2
4de37d0
a710ed0
04a03ab
3d812b1
cb00215
6508ddb
901fd61
88c4ac2
f743bd7
7d67573
b78607a
652d200
4bb69e9
94d6245
1373bb2
624195a
1135d45
f68e182
f32b079
5663fac
680b30d
ce044fa
a8459c5
c2a0564
d69c302
ad00934
405f32f
35b856d
5210f0f
ed222a0
35ded84
698e811
92f53fa
9ca16c0
14327c8
d058b17
c962de5
aadb61e
a1f2f2f
d714595
b43951b
bfd38ac
10255fe
32088a0
26b830a
5ea8fa5
5cbece2
557364a
e9ca79e
4d879da
775518e
982f8a8
448e307
1988155
77d49a9
7b6edbe
4e54284
785f2e9
d284122
c09e684
6014375
a7e99c2
a3708d9
4a921f6
aaefb0f
e18919d
5a45684
fbb27de
9e1d943
7797be2
ac86a4e
9a9477a
002881b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ void Deleter::Delete(uint32_t tile_group_id, uint32_t tuple_offset) { | |
|
||
auto *txn = executor_context_->GetTransaction(); | ||
auto tile_group = table_->GetTileGroupById(tile_group_id); | ||
PELOTON_ASSERT(tile_group != nullptr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this line added? Is this necessary? Or was it for debugging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GetTileGroupById can now return a nullptr if the TileGroup has been dropped, this is just a check that we don't hit that during make check and other Debug builds. That's what most of the ASSERTs added to the execution layer are looking for. If you're suggesting that we make it impossible to return a nullptr to the execution level, then we need to revisit how we access TileGroups and go only through an iterator that will always be valid. That may in fact be the correct solution, but is a larger design decision that I'd have to loop in people more familiar with what the execution layer (particularly codegen going forward) would want out of it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need to touch the execution layer. In a correct implementation, the execution layer should be completely oblivious to the GC. GetTileGroupById can now return a nullptr if the TileGroup has been dropped. -> That is my biggest concern. So this thing is checked in debug which is great. But what happens in release mode? If this function gets a nullptr, it hits a segfault and peloton crashes? That's not expected behavior. Like I said, in my overall comment, the whole point of the epoch based garbage collection is that the I would suggest that we can either do a hangouts call over the weekend. Or you talk to @apavlo about this. |
||
auto *tile_group_header = tile_group->GetHeader(); | ||
|
||
auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,8 @@ void Inserter::Init(storage::DataTable *table, | |
char *Inserter::AllocateTupleStorage() { | ||
location_ = table_->GetEmptyTupleSlot(nullptr); | ||
|
||
PELOTON_ASSERT(location_.IsNull() == false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above. Why was this added? Ideally, I believe these checks should be handled at a higher layer, probably the |
||
|
||
// Get the tile offset assuming that it is a row store | ||
auto tile_group = table_->GetTileGroupById(location_.block); | ||
auto layout = tile_group->GetLayout(); | ||
|
@@ -54,8 +56,11 @@ void Inserter::Insert() { | |
auto *txn = executor_context_->GetTransaction(); | ||
auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance(); | ||
|
||
ContainerTuple<storage::TileGroup> tuple( | ||
table_->GetTileGroupById(location_.block).get(), location_.offset); | ||
auto tile_group = table_->GetTileGroupById(location_.block).get(); | ||
PELOTON_ASSERT(tile_group != nullptr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question. Why was this added? |
||
|
||
ContainerTuple<storage::TileGroup> tuple(tile_group, location_.offset); | ||
|
||
ItemPointer *index_entry_ptr = nullptr; | ||
bool result = table_->InsertTuple(&tuple, location_, txn, &index_entry_ptr); | ||
if (result == false) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,9 @@ | |
#include "common/item_pointer.h" | ||
#include "common/logger.h" | ||
#include "common/macros.h" | ||
#include "common/container/lock_free_queue.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need to include this file and |
||
#include "gc/recycle_stack.h" | ||
#include "storage/data_table.h" | ||
|
||
namespace peloton { | ||
|
||
|
@@ -125,4 +128,7 @@ template class CuckooMap<std::shared_ptr<oid_t>, std::shared_ptr<oid_t>>; | |
// Used in StatementCacheManager | ||
template class CuckooMap<StatementCache *, StatementCache *>; | ||
|
||
// Used in TransactionLevelGCManager | ||
template class CuckooMap<oid_t, std::shared_ptr<gc::RecycleStack>>; | ||
|
||
} // namespace peloton |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,7 @@ template <typename ValueType> | |
void LOCK_FREE_ARRAY_TYPE::Erase(const std::size_t &offset, | ||
const ValueType &invalid_value) { | ||
LOG_TRACE("Erase at %lu", offset); | ||
PELOTON_ASSERT(lock_free_array.size() > offset); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, was this for debugging? Or have you introduced some new logic that could cause this condition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No new logic. .at() will throw an exception that we never catch if it's out of bounds. Seems more correct to me to have a PELOTON_ASSERT first. Also, see Find() and Update() which already have that logic. Erase() not having it just looks like an oversight. |
||
lock_free_array.at(offset) = invalid_value; | ||
} | ||
|
||
|
@@ -107,6 +108,20 @@ bool LOCK_FREE_ARRAY_TYPE::Contains(const ValueType &value) const { | |
return exists; | ||
} | ||
|
||
template <typename ValueType> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two main concerns:
|
||
ssize_t LOCK_FREE_ARRAY_TYPE::Lookup(const ValueType &value) { | ||
for (std::size_t array_itr = 0; array_itr < lock_free_array.size(); | ||
array_itr++) { | ||
auto array_value = lock_free_array.at(array_itr); | ||
// Check array value | ||
if (array_value == value) { | ||
return array_itr; | ||
} | ||
} | ||
|
||
return -1; | ||
} | ||
|
||
// Explicit template instantiation | ||
template class LockFreeArray<std::shared_ptr<oid_t>>; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,7 +52,7 @@ void PelotonInit::Initialize() { | |
threadpool::MonoQueuePool::GetExecutionInstance().Startup(); | ||
|
||
int parallelism = (CONNECTION_THREAD_COUNT + 3) / 4; | ||
storage::DataTable::SetActiveTileGroupCount(parallelism); | ||
storage::DataTable::SetDefaultActiveTileGroupCount(parallelism); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice job! |
||
storage::DataTable::SetActiveIndirectionArrayCount(parallelism); | ||
|
||
// start epoch. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -290,9 +290,9 @@ bool TimestampOrderingTransactionManager::PerformRead(TransactionContext *const | |
////////////////////////////////////////////////////////// | ||
else { | ||
PELOTON_ASSERT(current_txn->GetIsolationLevel() == | ||
IsolationLevelType::SERIALIZABLE || | ||
current_txn->GetIsolationLevel() == | ||
IsolationLevelType::REPEATABLE_READS); | ||
IsolationLevelType::SERIALIZABLE || | ||
current_txn->GetIsolationLevel() == | ||
IsolationLevelType::REPEATABLE_READS); | ||
|
||
oid_t tuple_id = location.offset; | ||
|
||
|
@@ -580,6 +580,9 @@ void TimestampOrderingTransactionManager::PerformDelete( | |
current_txn->RecordDelete(old_location); | ||
} | ||
|
||
// Performs Delete on a tuple that was created by the current transaction, and | ||
// never | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Combine line 584 and 585. |
||
// installed into the database | ||
void TimestampOrderingTransactionManager::PerformDelete( | ||
TransactionContext *const current_txn, const ItemPointer &location) { | ||
PELOTON_ASSERT(!current_txn->IsReadOnly()); | ||
|
@@ -737,6 +740,9 @@ ResultType TimestampOrderingTransactionManager::CommitTransaction( | |
gc_set->operator[](tile_group_id)[tuple_slot] = | ||
GCVersionType::COMMIT_DELETE; | ||
|
||
gc_set->operator[](new_version.block)[new_version.offset] = | ||
GCVersionType::TOMBSTONE; | ||
|
||
log_manager.LogDelete(ItemPointer(tile_group_id, tuple_slot)); | ||
|
||
} else if (tuple_entry.second == RWType::INSERT) { | ||
|
@@ -855,9 +861,11 @@ ResultType TimestampOrderingTransactionManager::AbortTransaction( | |
// before we unlink the aborted version from version list | ||
ItemPointer *index_entry_ptr = | ||
tile_group_header->GetIndirection(tuple_slot); | ||
UNUSED_ATTRIBUTE auto res = AtomicUpdateItemPointer( | ||
index_entry_ptr, ItemPointer(tile_group_id, tuple_slot)); | ||
PELOTON_ASSERT(res == true); | ||
if (index_entry_ptr) { | ||
UNUSED_ATTRIBUTE auto res = AtomicUpdateItemPointer( | ||
index_entry_ptr, ItemPointer(tile_group_id, tuple_slot)); | ||
PELOTON_ASSERT(res == true); | ||
} | ||
////////////////////////////////////////////////// | ||
|
||
// we should set the version before releasing the lock. | ||
|
@@ -903,9 +911,11 @@ ResultType TimestampOrderingTransactionManager::AbortTransaction( | |
// before we unlink the aborted version from version list | ||
ItemPointer *index_entry_ptr = | ||
tile_group_header->GetIndirection(tuple_slot); | ||
UNUSED_ATTRIBUTE auto res = AtomicUpdateItemPointer( | ||
index_entry_ptr, ItemPointer(tile_group_id, tuple_slot)); | ||
PELOTON_ASSERT(res == true); | ||
if (index_entry_ptr) { | ||
UNUSED_ATTRIBUTE auto res = AtomicUpdateItemPointer( | ||
index_entry_ptr, ItemPointer(tile_group_id, tuple_slot)); | ||
PELOTON_ASSERT(res == true); | ||
} | ||
////////////////////////////////////////////////// | ||
|
||
// we should set the version before releasing the lock. | ||
|
@@ -923,7 +933,7 @@ ResultType TimestampOrderingTransactionManager::AbortTransaction( | |
|
||
// add the version to gc set. | ||
gc_set->operator[](new_version.block)[new_version.offset] = | ||
GCVersionType::ABORT_DELETE; | ||
GCVersionType::TOMBSTONE; | ||
|
||
} else if (tuple_entry.second == RWType::INSERT) { | ||
tile_group_header->SetBeginCommitId(tuple_slot, MAX_CID); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,6 +154,12 @@ bool SeqScanExecutor::DExecute() { | |
while (current_tile_group_offset_ < table_tile_group_count_) { | ||
auto tile_group = | ||
target_table_->GetTileGroup(current_tile_group_offset_++); | ||
|
||
if (tile_group == nullptr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am confused that tile_group can be nullptr in this seq scan executor, but be asserted not be null in the previous index and hybrid scan executor ? |
||
// tile group was freed so, continue to next tile group | ||
continue; | ||
} | ||
|
||
auto tile_group_header = tile_group->GetHeader(); | ||
|
||
oid_t active_tuple_count = tile_group->GetNextTupleSlot(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style change, please remove it.