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

sql/pgwire: send CommandComplete for DDLs #3853

Merged
merged 1 commit into from
Jan 22, 2016
Merged

sql/pgwire: send CommandComplete for DDLs #3853

merged 1 commit into from
Jan 22, 2016

Conversation

dt
Copy link
Member

@dt dt commented Jan 14, 2016

Fixes #3849

Review on Reviewable

@dt dt added the SQL label Jan 14, 2016
tamird
tamird reviewed Jan 14, 2016
View reviewed changes
sql/pgwire/v3.go
@@ -677,6 +672,7 @@ func (c *v3Conn) sendResponse(resp driver.Response, formatCodes []formatCode, se

// Ack messages do not have a corresponding protobuf field, so handle those
// with a default.
// This also includes DDLs which want CommandComcplete as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@tamird
Copy link
Contributor

tamird commented Jan 14, 2016

Sigh, CI fails because serverMsgEmptyQuery is unused now. Perhaps you could fix #3852 while you're here.

@dt dt mentioned this pull request Jan 15, 2016
Closed
@dt dt force-pushed the cmdcomplete branch 2 times, most recently from 7a926fe to 7e0f5f4 Compare January 15, 2016 22:52
@dt
Copy link
Member Author

dt commented Jan 15, 2016

As discussed on #3852, a "real" fix three is going go involve plumbing the parser's opinion on emptiness though sql/driver. Once that's done the naive check added here might go, or stay around as it avoid work in the simple case.

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

sql/pgwire/v3.go, line 675 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Jan 16, 2016

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

sql/pgwire/v3.go, line 548 [r2] (raw file):
nit: don't you usually use dt? also, for greppability, no space between TODO and the paren

sql/pgwire/v3.go, line 550 [r2] (raw file):
unless execute?

sql/pgwire/v3.go, line 680 [r2] (raw file):
put your name on this TODO

Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Jan 16, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.

sql/pgwire/v3.go, line 548 [r2] (raw file):
oh, yeah, old habits. Done.

sql/pgwire/v3.go, line 550 [r2] (raw file):
no-op. clarified.

sql/pgwire/v3.go, line 680 [r2] (raw file):
bah, no good deed goes unpunished.

Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Jan 16, 2016

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Jan 16, 2016

This needs a test, if that's feasible.

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Jan 16, 2016

I'll take another look atlib/pq but it might not be easily within our existing tests.

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Jan 21, 2016

After digging through lib/pq and database/sql, figured out a test but it is gross.

Review status: 1 of 2 files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Jan 21, 2016

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

sql/pgwire_test.go, line 333 [r4] (raw file):
s/PGWire/CmdCompleteVsEmptyStatements/

sql/pgwire_test.go, line 346 [r4] (raw file):
there needs to be a comment here explaning what is actually being tested. in other words, the reader needs to know that schema changes should not return empty query responses.

also, why is creating the table necessary? seems like the CREATE DATABASE above would suffice.

Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Jan 21, 2016

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

sql/pgwire_test.go, line 333 [r4] (raw file):
Done.

sql/pgwire_test.go, line 346 [r4] (raw file):
Done.

Comments from the review on Reviewable.io

@dt
sql/pgwire: send CommandComplete for DDLs
93d48cd 
also explicitly send EmptyQuery than execute empty query strings.

Fixes cockroachdb#3849

Starts to address cockroachdb#3852 but, as noted in TODO and on the issue,
correctly handling more complex empty queries (e.g. "; ; ;") is
better left to the actual parser, which will require plumbing that
through the driver too.
@tamird
Copy link
Contributor

tamird commented Jan 22, 2016

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

sql/pgwire_test.go, line 365 [r5] (raw file):
if you comment this line out - does the test fail?

Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Jan 22, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.

sql/pgwire_test.go, line 365 [r5] (raw file):
Yep.

Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Jan 22, 2016

Review status: all files reviewed at latest revision, all discussions resolved.

sql/pgwire_test.go, line 365 [r5] (raw file):
Really? I'm surprised the defered recover() doesn't swallow the Fatal below. LGTM then.

Comments from the review on Reviewable.io

dt added a commit that referenced this pull request Jan 22, 2016
@dt
Merge pull request #3853 from dt/cmdcomplete
2fce854 
sql/pgwire: send CommandComplete for DDLs
@dt dt merged commit 2fce854 into cockroachdb:master Jan 22, 2016
@dt dt deleted the cmdcomplete branch January 22, 2016 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@maddyblue maddyblue

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dt @tamird @maddyblue