Skip to content
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

Merged
merged 1 commit into from
Dec 4, 2022
Merged

Conversation

acquamarin
Copy link
Collaborator

@acquamarin acquamarin commented Nov 29, 2022

  1. Adds the storage support for rel-deletion (without transaction):
    We store the deletedRelIDs in listUpdateStore, and the scanRelID will be responsible for set those deleted rels to unselected state in the valueVector.
  2. Moves the relID property to the first property of the relTable (We used to put it at the end of the relTable's property list).
  3. Solved issue Relstatistics refactor #891

src/storage/in_mem_csv_copier/in_mem_rel_csv_copier.cpp Outdated Show resolved Hide resolved
src/storage/in_mem_csv_copier/in_mem_rel_csv_copier.cpp Outdated Show resolved Hide resolved
src/storage/storage_structure/lists/lists_update_store.cpp Outdated Show resolved Hide resolved
src/storage/store/rels_statistics.cpp 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()) {
Copy link
Contributor

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:

  1. Maybe in_mem_rel_copier can also just call one of the updateNumRels functions.
  2. 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.

Copy link
Collaborator Author

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.

test/runner/e2e_delete_rel_test.cpp Outdated Show resolved Hide resolved
src/include/catalog/catalog_structs.h Outdated Show resolved Hide resolved
src/storage/in_mem_csv_copier/in_mem_rel_csv_copier.cpp Outdated Show resolved Hide resolved
src/processor/result/factorized_table.cpp Outdated Show resolved Hide resolved
src/processor/result/factorized_table.cpp Outdated Show resolved Hide resolved
src/include/storage/storage_structure/lists/lists.h Outdated Show resolved Hide resolved
src/storage/storage_structure/lists/lists_update_store.cpp Outdated Show resolved Hide resolved
test/runner/e2e_delete_rel_test.cpp Outdated Show resolved Hide resolved
@acquamarin acquamarin mentioned this pull request Dec 4, 2022
@acquamarin acquamarin merged commit a9d568b into master Dec 4, 2022
@acquamarin acquamarin deleted the rel-deletion branch December 4, 2022 04:06
@ray6080 ray6080 changed the title add rel deletion Delete rels without transaction inside storage Jan 29, 2023
@ray6080 ray6080 added the feature New features or missing components of existing features label Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New features or missing components of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants