Fix for #1336

[pmenon]

It looks like the deletion logic in codegen was incomplete.

Problem: Under SI, as an optimization, the transaction manager doesn't update the last reader field when reading a version created by the current transaction. Then, in the version of TimestampOrderingTransactionManager::PerformDelete(...) that is called in codegen::Deleter, there is a sanity check to ensure that the transaction actually read the version. This will be false because of the aforementioned optimization.

Solution: There is an existing TimestampOrderingTransactionManager::PerformDelete(...) that doesn't require insertion of an empty version. This is the API we should be using in codgen::Deleter only in the case where we delete a version we've created. This is the same logic as in the interpreted engine.

[coveralls]

Coverage increased (+0.008%) to 77.454% when pulling bff435f on pmenon:fix-delete-1336 into d68ab71 on cmu-db:master.

[yingjunwu]

hi @pmenon, I have reviewed your code and it looks great to me. The original codegen code for delete executor is incomplete: it only contains a simplified logic for deletion. Your code basically matches the logic in DeleteExecutor and checks both is_owner and is_written. I don't have any question on this part.

However, there are two questions.

1. You mentioned that Under SI, as an optimization, the transaction manager doesn't update the last reader field when reading a version created by the current transaction. This is not precise, as this is not an optimization for SI. If the current transaction has already acquired the ownership of a tuple, then it must has already updated the last reader field of the tuple, hence there's no need to update it again. However, it seems that the codegen version of delete executor does not check the isolation level for a transaction, am I right? We also did not check the isolation level in codegen executor.

2. Assertion fail when insert and delete in the same transaction #1336 mentioned that the test cases failed because of the codegen version of delete executor. It seems that when running test cases, we always set the codegen flag to true. If so, I wonder whether there's still need to maintain two versions of delete executor (the codegen version and the original version).

[pmenon]

@yingjunwu Great. Do you see any any required changes? Other answers below:

1. Yes, you're correct. The problem is that codegen::Deleter was not calling the correct PerformDelete() function. There seem to be two versions of the function: one when the txn is deleting a version written by itself, and one otherwise (both versions require the txn to own the version). Is this correct?
2. Yes, there is duplication. The plan is to slowly phase out the executor stuff as we get more functionality into codegen. Hopefully this summer.

[yingjunwu]

I reviewed the code a second time and didn't see any necessity to change the code, as the logic is the same as the old non-codegen code.

[yingjunwu]

@pmenon yes you are right. Regarding to the isolation levels, we don't want to check the isolation level in the codegen version of update/delete executor, right?

I think the PR is ready to go.

[poojanilangekar]

LGTM