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

[master < T0806-MG] Fix nested transaction error #339

Merged
merged 3 commits into from Feb 3, 2022

Conversation

antaljanosbenjamin
Copy link
Contributor

@antaljanosbenjamin antaljanosbenjamin commented Jan 31, 2022

[master < Task] PR

  • Check, and update documentation if necessary
  • Update changelog

Docs PR

@antaljanosbenjamin
Copy link
Contributor Author

I will update the docs soon.

@@ -680,8 +680,7 @@ TransformationResult Streams::Check(const std::string &stream_name, std::optiona
CallCustomTransformation(transformation_name, messages, result, accessor, *memory_resource, stream_name);

for (auto &row : result.rows) {
auto [query, parameters] =
ExtractTransformationResult(std::move(row.values), transformation_name, stream_name);
auto [query, parameters] = ExtractTransformationResult(row.values, transformation_name, stream_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this solve the nested transaction error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't solve it. The solution is on line 486 (aborting the transaction in case of serialization error).

This is just a bugfix that was hidden by the original bug: if the data is moved out here, then in the retry theere is no data, so we cannot retry the transaction. That's why I changed this to not move.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha, I see now it so if the transaction that has a serialization error it aborts. So why do we have both abort every time and throw after a certain amount of retries, should it not abort when it has tried several times? Does this make the counter redundant?

Copy link
Contributor

@kostasrim kostasrim Feb 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbajic That's a good question. If you call abort() it means that the whole transaction gets aborted and a new one must start by calling begin(). This is satisfied on line 462 (if you see this is wrapped in a while(true) loop. If there is a serialization error the whole transaction will be replayed - see the inner for-loop). Therefore the counter is for the total amount of retries the whole transaction must replay either by completing successfully or throwing. After all, a transaction is a log of changes done to the storage engine, therefore you either commit it or abort but nothing inbetween

Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shiny and clean. Thank you for taking care of this -- I introduced this small bug ;)

Copy link
Contributor

@jbajic jbajic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! 🎉

@antaljanosbenjamin antaljanosbenjamin self-assigned this Feb 2, 2022
@antonio2368 antonio2368 merged commit 661e518 into master Feb 3, 2022
@antonio2368 antonio2368 deleted the T0806-MG-fix-nested-transaction-error branch February 3, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants