Add support for retrieving multiple results. #1

petermattis · 2016-02-03T14:30:14Z

Reworked how the msg-ready command (Z) is processed. Previously
execution of a query would look for the msg-ready command before
completing the operation. Now, when executing a query, the driver places
the connection into a state where it knows there may be more results. If
another query is subsequently executed, the driver waits for the
msg-ready command to arrive, discarding any other commands, before
sending the new query. But if the special NEXT query is executed, the
driver looks for another set of results on the connection. If no such
results are found, ErrNoMoreResults is returned.

tamird · 2016-02-03T14:45:56Z

Connection pinning concerns still stand. I'd really like to see this functionality wrapped up into a function that takes care of all the shenanigans.

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

conn.go, line 624 [r1] (raw file):
along with ErrNoMoreResults, this should be an exported constant

conn.go, line 638 [r1] (raw file):
what's this change about? looks like unrelated cleanup - separate commit or back it out - we don't want to diverge more than necessary.

conn.go, line 1470 [r1] (raw file):
what's happening here?

conn.go, line 1641 [r1] (raw file):
the lack of a default here is worrying

Comments from the review on Reviewable.io

tbg · 2016-02-03T14:46:23Z

conn.go

@@ -200,7 +204,7 @@ func (c *conn) handlePgpass(o values) {
 			}
 		}
 		return append(fs, string(f))
-	}	


if you're upstreaming this (or trying to) it probably makes sense to factor out the drive-by changes in a separate commit.

tbg · 2016-02-03T14:52:36Z

conn.go

-	b.string(q)
-	cn.send(b)
+	querySent := false
+	nextResult := q == "NEXT"


pull out magic constant into named const variable?

petermattis · 2016-02-03T14:52:54Z

Moved the drive-by change to a separate commit.

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

conn.go, line 624 [r1] (raw file):
Done.

conn.go, line 638 [r1] (raw file):
Yes, just unrelated cleanup of unused structure. I'm going to push this upstream (or try to).

conn.go, line 1470 [r1] (raw file):
This is the correct way to process a result. A C or I command indicates the end of data rows. The previous behavior of looking for a Z was just completely incorrect.

conn.go, line 1641 [r1] (raw file):
As far as I can tell from the postgres docs, this is what is expected. The client "syncs" the connection by waiting for a Z.

Comments from the review on Reviewable.io

tamird · 2016-02-03T14:59:52Z

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

conn.go, line 1470 [r1] (raw file):
I see. That also seems like a standalone change that deserves its own commit.

conn.go, line 1641 [r1] (raw file):
and it's OK to discard bytes until the Z arrives?

Comments from the review on Reviewable.io

petermattis · 2016-02-03T15:02:55Z

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

conn.go, line 624 [r1] (raw file):
Done.

conn.go, line 1470 [r1] (raw file):
What is the value in moving this to its own commit? That just feels like busy work for the author of the code. And note that this isn't easily separable. We'd still need all of the readyForQuery stuff.

conn.go, line 1641 [r1] (raw file):
Yes.

conn_test.go, line 363 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

petermattis · 2016-02-03T15:06:07Z

@tamird Do you have a suggestion for how to wrap up this functionality into a function to take care of the shenanigans?

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

Comments from the review on Reviewable.io

tamird · 2016-02-03T15:10:22Z

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

conn.go, line 1470 [r1] (raw file):
I'm missing something, because if the current code is just wrong, then this change should be separable.

I don't think this change will be accepted upstream in a single PR, which is why I think it should be separated.

conn.go, line 1641 [r1] (raw file):
Can you add a comment that points to docs? That's quite surprising given the apparent strictness of similar code in this file.

Comments from the review on Reviewable.io

petermattis · 2016-02-03T15:14:06Z

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

conn.go, line 1470 [r1] (raw file):
The 5 lines of change are fix a bug when a query contains multiple statements, but they require all of the readyForQuery changes as well.

But let's focus on whether the code is correct first and worry about how to get it pushed upstream second. It looks very likely that the handling of multiple results will not be accepted upstream. That's fine, that's what forks are for.

conn.go, line 1641 [r1] (raw file):
Since a query string could contain several queries (separated by semicolons), there might be several such response sequences before the backend finishes processing the query string. ReadyForQuery is issued when the entire string has been processed and the backend is ready to accept a new query string.

Comments from the review on Reviewable.io

tbg · 2016-02-03T15:25:35Z

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

conn.go, line 1470 [r1] (raw file):

What is the value in moving this to its own commit?

Maybe no value doing it now, but had it been in its own commit from the start, you would've explained this in your commit message. Instead, Tamir and I independently tried to understand this change without explanation, which costs time.

conn.go, line 1339 [r3] (raw file):
s/has/as/

Comments from the review on Reviewable.io

tamird · 2016-02-03T15:45:06Z

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

conn.go, line 1470 [r1] (raw file):
I can't tell if this code is correct without this change separated out. With this change (but without support for NEXT) do the tests pass?

Comments from the review on Reviewable.io

Reworked how the msg-ready command (`Z`) is processed. Previously execution of a query would look for the msg-ready command before completing the operation. Now, when executing a query, the driver places the connection into a state where it knows there may be more results. If another query is subsequently executed, the driver waits for the msg-ready command to arrive, discarding any other commands, before sending the new query.

If the special `NEXT` query is executed, the driver looks for another set of results on the connection. If no such results are found, ErrNoMoreResults is returned.

petermattis · 2016-02-03T15:56:16Z

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

conn.go, line 1470 [r1] (raw file):
Yes. The support for NEXT is purely in conn.SimpleQuery. Refactored the commits to increase reviewer happiness.

Comments from the review on Reviewable.io

tbg · 2016-02-03T16:06:18Z

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

Comments from the review on Reviewable.io

petermattis · 2016-02-03T17:00:38Z

Anything else to do here?

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

conn.go, line 1339 [r3] (raw file):
Done.

Comments from the review on Reviewable.io

tamird · 2016-02-03T19:12:57Z

LGTM

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

Comments from the review on Reviewable.io

Add support for retrieving multiple results.

petermattis assigned tamird Feb 3, 2016

tbg reviewed Feb 3, 2016
View reviewed changes

Remove unnecessary allocation.

3e033bd

tbg reviewed Feb 3, 2016
View reviewed changes

conn.go

b.string(q)

cn.send(b)

querySent := false

nextResult := q == "NEXT"

Copy link

Member

tbg Feb 3, 2016

Choose a reason for hiding this comment

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

pull out magic constant into named const variable?

petermattis force-pushed the pmattis/multiple-results branch from 8b0d5d6 to 943e575 Compare February 3, 2016 14:52

petermattis force-pushed the pmattis/multiple-results branch from 943e575 to 07298f2 Compare February 3, 2016 15:00

petermattis force-pushed the pmattis/multiple-results branch from 07298f2 to 73fc92e Compare February 3, 2016 15:07

petermattis force-pushed the pmattis/multiple-results branch from 73fc92e to 95bc43c Compare February 3, 2016 15:18

petermattis force-pushed the pmattis/multiple-results branch from 95bc43c to cca0657 Compare February 3, 2016 15:55

Add support for retrieving multiple results.

4bc2416

If the special `NEXT` query is executed, the driver looks for another set of results on the connection. If no such results are found, ErrNoMoreResults is returned.

petermattis force-pushed the pmattis/multiple-results branch from cca0657 to 4bc2416 Compare February 3, 2016 15:56

petermattis added a commit that referenced this pull request Feb 3, 2016

Merge pull request #1 from cockroachdb/pmattis/multiple-results

77bd855

Add support for retrieving multiple results.

petermattis merged commit 77bd855 into master Feb 3, 2016

petermattis mentioned this pull request Feb 5, 2016

cli: add support for multiple results to SQL statements cockroachdb/cockroach#4081

Merged

tamird mentioned this pull request Feb 22, 2016

Add support for retrieving multiple results. #2

Merged

petermattis deleted the pmattis/multiple-results branch January 25, 2017 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for retrieving multiple results. #1

Add support for retrieving multiple results. #1

petermattis commented Feb 3, 2016

tamird commented Feb 3, 2016

tbg Feb 3, 2016

tbg Feb 3, 2016

petermattis commented Feb 3, 2016

tamird commented Feb 3, 2016

petermattis commented Feb 3, 2016

petermattis commented Feb 3, 2016

tamird commented Feb 3, 2016

petermattis commented Feb 3, 2016

tbg commented Feb 3, 2016

tamird commented Feb 3, 2016

petermattis commented Feb 3, 2016

tbg commented Feb 3, 2016

petermattis commented Feb 3, 2016

tamird commented Feb 3, 2016

Add support for retrieving multiple results. #1

Add support for retrieving multiple results. #1

Conversation

petermattis commented Feb 3, 2016

tamird commented Feb 3, 2016

tbg Feb 3, 2016

Choose a reason for hiding this comment

tbg Feb 3, 2016

Choose a reason for hiding this comment

petermattis commented Feb 3, 2016

tamird commented Feb 3, 2016

petermattis commented Feb 3, 2016

petermattis commented Feb 3, 2016

tamird commented Feb 3, 2016

petermattis commented Feb 3, 2016

tbg commented Feb 3, 2016

tamird commented Feb 3, 2016

petermattis commented Feb 3, 2016

tbg commented Feb 3, 2016

petermattis commented Feb 3, 2016

tamird commented Feb 3, 2016