-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix RDF and Rel Group Entries in WAL Recovery #3912
Conversation
Benchmark ResultMaster commit hash:
|
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.
Thanks!
#include <utility> | ||
|
||
#include "binder/binder.h" | ||
#include "catalog/catalog.h" |
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.
Double check if this include catalog/catalog.h
is necessary.
src/catalog/catalog_set.cpp
Outdated
@@ -51,11 +54,12 @@ CatalogEntry* CatalogSet::getEntryNoLock(Transaction* transaction, const std::st | |||
} | |||
|
|||
void CatalogSet::createEntry(Transaction* transaction, std::unique_ptr<CatalogEntry> entry) { | |||
std::lock_guard lck{mtx}; | |||
createEntryNoLock(transaction, std::move(entry)); | |||
std::unique_lock lck{mtx}; |
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.
See if you can reuse UniqLock
under common/uniq_lock.h
.
src/catalog/catalog_set.cpp
Outdated
} | ||
|
||
void CatalogSet::createEntryNoLock(Transaction* transaction, std::unique_ptr<CatalogEntry> entry) { | ||
void CatalogSet::createEntryNoLock(Transaction* transaction, std::unique_ptr<CatalogEntry> entry, |
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.
Rename this back to createEntry
. I think passing lock as param in is quite evident itself.
src/catalog/catalog_set.cpp
Outdated
@@ -75,6 +79,7 @@ void CatalogSet::createEntryNoLock(Transaction* transaction, std::unique_ptr<Cat | |||
entries.emplace(entry->getName(), std::move(dummyEntry)); | |||
auto entryPtr = entry.get(); | |||
emplaceNoLock(std::move(entry)); | |||
lck.unlock(); |
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.
Do not unlock here. Instead you can move the part of push undo buffer out of this function.
It's not a good practice to unlock a passed in lock. Always better to lock and unlock at the same level.
src/catalog/catalog_set.cpp
Outdated
@@ -142,7 +147,7 @@ void CatalogSet::dropEntryNoLock(Transaction* transaction, const std::string& na | |||
} | |||
|
|||
void CatalogSet::alterEntry(Transaction* transaction, const binder::BoundAlterInfo& alterInfo) { | |||
std::lock_guard lck{mtx}; | |||
std::unique_lock lck{mtx}; |
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.
Ditto here on UniqLock.
@@ -54,12 +54,14 @@ class Deserializer { | |||
void deserializeVector(std::vector<T>& values) { | |||
uint64_t vectorSize; | |||
deserializeValue(vectorSize); | |||
values.resize(vectorSize); | |||
for (auto& value : values) { | |||
values.reserve(vectorSize); |
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.
Why change this function?
Don't do reserve and emplace_back. This assumes the input values
is empty before deserializing. but it can be violated. either change to resize
or at least assert that values
is empty before deserializing.
@@ -36,6 +37,7 @@ class WAL { | |||
|
|||
~WAL(); | |||
|
|||
void logCreateTableEntryRecord(binder::BoundCreateTableInfo tableInfo); |
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.
Add a comment here to explain logCreateTableEntryRecord
and logCreateCatalogEntryRecord
, as their names are not evident enough.
Alternatively you can rename them to logCreateTableEntryRecord
and logCreateNonTableEntryRecord
, which is not perfect but somewhat makes sense.
@@ -65,10 +70,16 @@ void Transaction::pushCatalogEntry(CatalogSet& catalogSet, CatalogEntry& catalog | |||
case CatalogEntryType::TYPE_ENTRY: { | |||
KU_ASSERT( | |||
catalogEntry.getType() == CatalogEntryType::DUMMY_ENTRY && catalogEntry.isDeleted()); | |||
if (newCatalogEntry->hasParent()) { |
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.
Is this for sequence? I'm not following sequence for now. Maybe you can just walk me through this line of change tomorrow.
Can u unskip and double check tests related to
|
src/catalog/catalog_set.cpp
Outdated
@@ -50,12 +50,24 @@ CatalogEntry* CatalogSet::getEntryNoLock(Transaction* transaction, const std::st | |||
return entry; | |||
} | |||
|
|||
static void LogEntryForTrx(Transaction* transaction, CatalogSet& set, CatalogEntry& entry) { |
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.
LogEntryForTrx -> logEntryForTrx.
Function names always stay as the first letter lower case regardless of static or non-static
2e33352
to
784f973
Compare
Benchmark ResultMaster commit hash:
|
e1e0b48
to
65d3ce0
Compare
Benchmark ResultMaster commit hash:
|
* initial reimpl * Run clang-format * add tests and fix locking * Run clang-format * address comments * Run clang-format * fix small mistake * bump versions * change name casing * fix logging * export symbol for extension * change default constructor * make change for dllexport * fix sequence log to wal during recovery * bump version again --------- Co-authored-by: CI Bot <yiyun-sj@users.noreply.github.com> Co-authored-by: Guodong Jin <guod.jin@gmail.com>
No description provided.