-
Notifications
You must be signed in to change notification settings - Fork 128
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
Process sequence of pending packets #580
Merged
Lagovas
merged 19 commits into
master
from
lagovas/T2663-process-sequence-pending-packets
Sep 20, 2022
Merged
Process sequence of pending packets #580
Lagovas
merged 19 commits into
master
from
lagovas/T2663-process-sequence-pending-packets
Sep 20, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
it fixes issue
This reverts commit b7b686d.
remove forgetting BindPacket until ReadyForQuery received
Zhaars
reviewed
Sep 19, 2022
Zhaars
reviewed
Sep 19, 2022
Lagovas
commented
Sep 20, 2022
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.
@Zhaars , added nil checks and replaced panic with returning error
Zhaars
approved these changes
Sep 20, 2022
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.
Cool, thanks.
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What was the problem? DB drivers can send set of packets in one TCP packet. And usually, they are related to each other and look like Parse + Bind + Describe + Execute + Sync. Driver registers query in Parse, then binds data, then asks to describe types, executes query and commit state. Acra registers PreparedStatement on ParseComplete packet that DB responds to driver after validation and acceptance. Bind packet relates to Parse packet because it should specify statement name for which it sends values.
In this flow acra do next: remembers Parse packet and save it as
pendingParse
. Then handle Bind packet, loads findspendignParse
and link them together.Previously we expected that one set of packets in TCP packet contains only one packet with a query like SimpleQuery or Parse packet. And when Acra receives ParsePacket, it resets state, forgets about Bind and other kind of packets - https://github.com/cossacklabs/acra/blob/master/decryptor/postgresql/protocol.go#L121
But drivers not limited in the number of query packets. They can send several of them in one set. And they should be processed in same order. If acra got 2 Describe packets from driver to database and then receives 2 RowDescription responses from the database, it should link first RowDescription to the first Describe, second one to the second.
So Acra should not store only last packet, but should all of them in same order.
In this PR was added new component that stores all pendingPackets and store them separately according to the type, works as a queue. And was refactored logic with working with pending packets. Now it removes item from the queue, not nilify a field.
Due to limitations of python's drivers that don't allow to compile such set of packets to write regression test, were added unit-test with dumped packets that reproduces the problem.
Checklist
with new changes