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

pgwire: empty query should result in emptyresponse, not error/commandcomplete #9093

Closed
knz opened this issue Sep 4, 2016 · 9 comments
Closed
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@knz
Copy link
Contributor

knz commented Sep 4, 2016

Found by @fishcakez in #5582 (comment):

An empty query will fail with an error, postgresql handles this with an EmptyQueryResponse message, instead of CommandComplete. This may not matter to cockroach.

Note that this is may appear to be a dup of #3852 but the fix to that issue didn't change what happens for empty queries.

@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Sep 4, 2016
@knz
Copy link
Contributor Author

knz commented Sep 4, 2016

cc @mjibson @dt

@dt
Copy link
Member

dt commented Sep 5, 2016

Huh, I thought I fixed this a long time ago, and we even have a test for it: TestCmdCompleteVsEmptyStatements

@dt
Copy link
Member

dt commented Sep 5, 2016

@knz can you clarify "the fix to that issue [#3852] didn't change what happens for empty queries."... I thought that that was exactly what that changed.

@knz
Copy link
Contributor Author

knz commented Sep 5, 2016

@fishcakez could you give more details about how you isolated this specific issue in your use case?

@fishcakez
Copy link

The reference issue seems to refer to using the simple Query messages to send multiple queries (of which some are empty). The situation we encountered (running the test suite for a postgres client against cockroach) is doing a single empty (prepared) query using Parse, Describe, Execute. I did not check for which message of the 3 the error occurs. However postgres will return the EmptyQueryResponse and so the test for our client fails.

@maddyblue maddyblue self-assigned this Sep 12, 2016
@tlvenn
Copy link
Contributor

tlvenn commented Sep 16, 2016

@mjibson any chance to pull that fix soon ?

@maddyblue
Copy link
Contributor

I'm waiting on lib/pq#500 to get merged (which I don't control), otherwise we can't write a test for this. Sorry for the delay. @tamird do you have write access to lib/pq? Would you be willing to review + merge that PR?

@tamird
Copy link
Contributor

tamird commented Sep 20, 2016

Sadly I do not have commit over there.

On Tue, Sep 20, 2016 at 2:03 PM, Matt Jibson notifications@github.com
wrote:

I'm waiting on lib/pq#500 lib/pq#500 to get
merged (which I don't control), otherwise we can't write a test for this.
Sorry for the delay. @tamird https://github.com/tamird do you have
write access to lib/pq? Would you be willing to review + merge that PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9093 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPLiVB1q8bQlFe9f5-VxKRH9JHX5xks5qsB_VgaJpZM4J0gST
.

@tlvenn
Copy link
Contributor

tlvenn commented Oct 10, 2016

Hope this can finally be fixed now the changes have been merged in lib/pq. Thanks for your work @mjibson and @tamird !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

6 participants