Garbage collection fixes

mbutrovich commented 6 years ago

This PR resolves most of the issues identified in issue #1325. The summary of changes are:

Added a function to GCManager to allow unused ItemPointers to be returned without going through the entire Unlink and Reclaim process.
Modified TOTransactionManager to pass tombstones created by deletes to the GCManager.
Modified DataTable's Insert to return the ItemPointer to the GCManager in the case of a failed insert.
Modified DataTable's InsertIntoIndexes to iterate through indexes and remove inserted keys in the event of a failure.
Modified GCManager's Unlink function to clean indexes from garbage created by COMMIT_DELETE, COMMIT_UPDATE, and ABORT_UPDATE.
Added 14 tests to transaction_level_gc_manager_test.cpp to handle more complex GC scenarios. Currently 4 of these tests still fail for polluting indexes with old keys, but we believe this will require more significant changes at the execution layer to resolve. We believe we have resolved all of the tuple-level GC bugs and most of the index bugs. We have disabled the 4 check that fail and will open a new issue describing those scenarios only once this is merged.

Note: significant changes to the GCManager are coming as part of our TileGroup-freeing branch, but we wanted to get these bug changes merge in based on the master branch first.

coveralls commented 6 years ago

Coverage decreased (-77.3%) to 0.0% when pulling b4ce7e4d56e797ed5d9c0b0c3d742d956758d541 on mbutrovich:gc_fixes into 512b31d67809699dbac83206192bbf7e338d41d1 on cmu-db:master.

poojanilangekar commented 6 years ago

@mbutrovich Can you please fix the transaction_level_gc_manager_test?

mbutrovich commented 6 years ago

@poojanilangekar After speaking with Andy, I disabled those 4 checks so we can get this PR to merge, and will open a new issue to reflect the failing scenario in those 4 tests.

mbutrovich commented 6 years ago

@poojanilangekar I feel like the formatting should be done. Restricting it to our lines pushes the same problem and extra work to the next PR I'm going to submit on these files soon. I made a single commit just for formatting at the end, so if you checkout the commit before that it might make reviewing easier.

poojanilangekar commented 6 years ago

@mbutrovich I have reverted your style changes for now. We shouldn't get other changes in the PR because that way we'd keep modifying the same lines depending on the OS of the committer. Also, you won't be the only team modifying some of those files, so everyone is going to have these issues and the final merge would be a nightmare. I have asked @tcm-marcel about this. We should look into doing something about this.

tcm-marcel commented 6 years ago

@poojanilangekar take a look at this for the formatting stuff: https://github.com/cmu-db/peloton/pull/1329

mbutrovich commented 6 years ago

Updating this to Do Not Merge. We will incorporate the feedback and changes into our TileGroup compaction branch and submit all of that together as on PR. Since some of the requested changes here conflict with what we'll be doing with TG compaction, we'll collect everything together and request feedback on that.

mbutrovich commented 6 years ago

Closing since we are bringing these changes over to our tilegroup compaction PR for more eyes on it.

cmu-db / peloton

Garbage collection fixes #1326