Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Buffer transaction statements to disk to prevent replay deadlock #505
Buffer transaction statements to disk to prevent replay deadlock #505
Changes from all commits
21463c7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we have the LSN value for that consistent point available in the context here? If yes, I believe we should be using it to make sure we are skipping transactions as intended. If no, then we should add to the comment why not, so that another PR later can go and fix the situation if we feel like it should be fixed.
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.
@dimitri This is a great point ! I think the current ignore logic implemented in this PR might cause inconsistency where it might ignore the partial transaction, but applies the next transaction because it has valid BEGIN/COMMIT block.
I need to have a another deep look now.
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.
@dimitri I was wrong in my previous comment.
While switching to replay mode, follow ensures that the pending LSNs in transform queue has been processed and applied to the target. This ensures everything except the last the partial transaction has been successfully committed into the target. After switching to replay mode, stream_transform_resume again streams the last partial file(which is already applied by the previous step) to the replay process which leads to this scenario. I think streaming last partial .sql from stream_transform_resume is redundant and shall be removed and this shouldn't cause any issue. WDYT?
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'm wondering are these PRs #348, #277 trying to do the same in different ways?
I no longer see partial txn when I remove part of the code from stream_transform_resume which streams partial sql., I didn't see any data loss either.
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.
IIRC the transform parts need to parse/load the latest file to build up the context in case we stopped in the middle of a transaction (we then need to have the start of the transaction in-memory); but I agree we don't need to send that to the replay process, which is only going to skip again.
That said the skip logic is sound and I wonder why in your PR we can't benefit from it?
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.
Please rewrite the message so that it ends with
": %s", line
. This makes it easier to process the logs later.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.
Please rewrite the message so that it ends with
": %s", line
. This makes it easier to process the logs later.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.
Please rewrite the message so that it ends with
": %s", line
. This makes it easier to process the logs later.