-
Notifications
You must be signed in to change notification settings - Fork 94
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
Delete rels without transaction inside storage #1075
Conversation
src/include/storage/storage_structure/lists/lists_update_store.h
Outdated
Show resolved
Hide resolved
src/include/storage/storage_structure/lists/lists_update_store.h
Outdated
Show resolved
Hide resolved
@@ -32,7 +32,10 @@ void RelsStatistics::setNumRelsForTable(table_id_t relTableID, uint64_t numRels) | |||
auto relStatistics = | |||
(RelStatistics*)tablesStatisticsContentForWriteTrx->tableStatisticPerTable[relTableID] | |||
.get(); | |||
tablesStatisticsContentForWriteTrx->nextRelID += (numRels - relStatistics->getNumTuples()); | |||
if (numRels > relStatistics->getNumTuples()) { |
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.
This looks confusing and I actually don't understand the logic. I'd rather be explicit about how tablesStatisticsContentForWriteTrx->nextRelID is incremented. Note that nextRelID should only be incremented. So I would do it in one of the updateNumRelsByValueForTable if the value is positive. I know setNumRelsForTable is also called by in_mem_rel_copier, so we need a way to set it then too. I can think of 2 things:
- Maybe in_mem_rel_copier can also just call one of the updateNumRels functions.
- If that won't work, you can have a wrapper function setNumTuples(table_id_t relTableID, uint64_t numRels) which only sets numTuples. And another function incrementNextRelID, which is called both from the updateNumRels function. and in_mem_rel_copier explicitly.
Let me know if you want to discuss this.
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.
I created a new function:
void RelsStatistics::updateNumRelsByValue(
table_id_t relTableID, table_id_t srcTableID, table_id_t dstTableID, int64_t value) {
lock_t lck{mtx};
initTableStatisticPerTableForWriteTrxIfNecessary();
auto relStatistics =
(RelStatistics*)tablesStatisticsContentForWriteTrx->tableStatisticPerTable[relTableID]
.get();
auto numRelsAfterUpdate = relStatistics->getNumTuples() + value;
relStatistics->setNumTuples(numRelsAfterUpdate);
for (auto relDirection : REL_DIRECTIONS) {
auto relStatistics =
(RelStatistics*)tablesStatisticsContentForWriteTrx->tableStatisticPerTable
.at(relTableID)
.get();
relStatistics->numRelsPerDirectionBoundTable[relDirection].at(
relDirection == FWD ? srcTableID : dstTableID) += value;
}
// Update the nextRelID only when we are inserting rels.
if (value > 0) {
tablesStatisticsContentForWriteTrx->nextRelID += value;
}
assertNumRelsIsSound(relStatistics->numRelsPerDirectionBoundTable[FWD], numRelsAfterUpdate);
assertNumRelsIsSound(relStatistics->numRelsPerDirectionBoundTable[BWD], numRelsAfterUpdate);
}
It will only update the relID if we are inserting rels.
9a12098
to
96214f5
Compare
96214f5
to
1133016
Compare
We store the deletedRelIDs in listUpdateStore, and the scanRelID will be responsible for set those deleted rels to unselected state in the valueVector.