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.

Sign up for GitHub

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

Jump to bottom

feat: Add Copy command parsing in QueryMessage and basic psql e2e test #43

Merged
merged 21 commits into from
Feb 24, 2022
Merged

feat: Add Copy command parsing in QueryMessage and basic psql e2e test #43

merged 21 commits into from
Feb 24, 2022

Conversation

Vizerai
Copy link
Collaborator

@Vizerai Vizerai commented Feb 18, 2022

No description provided.

tinaspark
tinaspark reviewed Feb 18, 2022
View reviewed changes
src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/ControlMessage.java Show resolved Hide resolved
src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/ControlMessage.java Outdated
return new SyncMessage(connection);
case TerminateMessage.IDENTIFIER:
return new TerminateMessage(connection);
case CopyDoneMessage.IDENTIFIER:
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we allowed to receive CopyDone/CopyData/CopyFail if we're not in COPY_IN mode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it should not.

olavloite
olavloite reviewed Feb 19, 2022
View reviewed changes
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

The build fails because of a compile error, I think because ConnectionStatus is not imported by CopyDoneMessage.

src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/CopyDoneMessage.java Show resolved Hide resolved
src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/CopyDoneMessage.java Show resolved Hide resolved
@Vizerai
Copy link
Collaborator Author

Vizerai commented Feb 22, 2022

The e2e failures seem to be due to a change on the backend. There a new pg_catalog entries returned from the information schema.

@olavloite olavloite changed the title Adding Copy command parsing in QueryMessage and basic e2e test. feat: Add Copy command parsing in QueryMessage and basic psql e2e test Feb 22, 2022
@olavloite
Copy link
Collaborator

The current e2e test failure should be fixed once #40 has been merged (it's caused by the "?" that is fixed in #40)

tinaspark
tinaspark reviewed Feb 22, 2022
View reviewed changes
Copy link
Collaborator

@tinaspark tinaspark left a comment

Choose a reason for hiding this comment

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

LGTM once the test failures are fixed by #40

Vizerai and others added 10 commits February 22, 2022 14:43
@Vizerai
Merge branch 'postgresql-dialect' into copyparse
a67add4
@Vizerai
Merge branch 'postgresql-dialect' of https://github.com/GoogleCloudPl…
086d299 
…atform/pgadapter into copyparse
@Vizerai
Merge branch 'copyparse' of https://github.com/GoogleCloudPlatform/pg…
4f77682 
…adapter into copyparse
@Vizerai
Update
169f2bb
@olavloite
fix: do not return Ready response for Copy messages
f3d3c67 
Copy messages should not respond with a Ready response, only with its
own specific Copy responses. Failure to do so can cause the calling
client to think that the operation has already finished, which again can
cause the entire copy operation to fail if the connection is closed
directly after receiving the Ready response.

Also the current Copy operation should not execute a rollback if
something fails, as it is operating in auto-commit mode. Trying to
rollback when in auto-commit mode will cause an error.
@olavloite
test: update test values to avoid duplicates
4b1b5d1
@olavloite
fix: remove non-existing rows from expected file
774d367
@olavloite
fix: add missing COPY to response
26174dd
@olavloite
fix: copy comes before the select
d99c3c5
@olavloite
fix: COPY is on stdin, the expected result has an extra empty line
2a7f121
@olavloite
Copy link
Collaborator

@Vizerai I was wondering why the e2e-tests were failing, so I had a look at this PR and ended up doing a couple of small changes. It might be good if you go through those to check that you are OK with them before merging.

@Vizerai
Copy link
Collaborator Author

Vizerai commented Feb 23, 2022

@Vizerai I was wondering why the e2e-tests were failing, so I had a look at this PR and ended up doing a couple of small changes. It might be good if you go through those to check that you are OK with them before merging.

The changes look good. Thanks for the update.

@Vizerai Vizerai merged commit 184c00e into postgresql-dialect Feb 24, 2022
@olavloite olavloite deleted the copyparse branch March 6, 2022 16:05
@release-please release-please bot mentioned this pull request Mar 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tinaspark tinaspark tinaspark approved these changes

@olavloite olavloite olavloite approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vizerai @olavloite @tinaspark