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

Add support for retrieving multiple results. #425

Closed
wants to merge 3 commits into from

Conversation

petermattis
Copy link
Contributor

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.

@msakrejda
Copy link
Contributor

This is very clever, and I can see the utility, but I'm really skeptical of adding hacks like this... I'd like to hear opinions from other committers or the community.

@petermattis
Copy link
Contributor Author

Besides adding new functionality, this fixes the processing of results in queries that contain multiple statements. Consider the query SELECT 1; SELECT 2, 3;. The SELECT 1 statement will return a single row (D) followed by a command complete (C). The second statement (SELECT 2, 3) will then send a row description (T) which will cause the processing of rows in rows.Next to error (unexpected message after execute) and leave the connection in a bad state.

I'm not terribly thrilled with the usage of NEXT as an interface to retrieve multiple results, but I'm not seeing any other options. The database/sql package doesn't give any access to driver data structures AFAICT.

@johto
Copy link
Contributor

johto commented Feb 3, 2016

I'm not terribly thrilled with the usage of NEXT as an interface to retrieve multiple results, but I'm not seeing any other options. The database/sql package doesn't give any access to driver data structures AFAICT.

I really would rather not see hacks like this. If database/sql doesn't support something, why not suggest improvements over there?

@petermattis
Copy link
Contributor Author

There is an issue filed upstream about this: golang/go#12382

@johto
Copy link
Contributor

johto commented Feb 3, 2016

Besides adding new functionality, this fixes the processing of results in queries that contain multiple statements.

Now that might be worth fixing. I'm not sure whether we should throw away result sets after the first one (the opposite of what e.g. libpq does), or whether to simply clean up correctly and return an error.

Has anyone checked what the behavior of libpq with the streaming interface is?

@petermattis petermattis force-pushed the pmattis/multiple-results branch 4 times, most recently from 73fc92e to 95bc43c Compare February 3, 2016 15:18
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.
@petermattis petermattis force-pushed the pmattis/multiple-results branch from 95bc43c to cca0657 Compare February 3, 2016 15:55
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 petermattis force-pushed the pmattis/multiple-results branch from cca0657 to 4bc2416 Compare February 3, 2016 15:56
@kardianos
Copy link
Contributor

Support for multiple result sets has been merged into database/sql now.

@tamird
Copy link
Collaborator

tamird commented Oct 17, 2016

Note that such support will need to live behind an appropriate build tag, and has no business being merged before 1.8 is out. It'd be cool to see it working, though.

@kardianos
Copy link
Contributor

@tamird On the contrary, it may be implemented as additional methods and the sql package will detect support. No build tags are required.

Also, I have yet to make this general request, but I'll start with the request here:

There are many new features in database/sql. Please oh PLEASE start adding support for them November 1 when the go1.8 freeze starts and have some idea of your implementation before the first 1.8 pre-release comes out. Once go1.8 is released, this stuff is frozen solid. If a design mistake was made I want to know before the first pre-release. Thanks.

@tamird
Copy link
Collaborator

tamird commented Oct 17, 2016

Ah, of course you're right. It would be great if there was somewhere to look at a live list of those updates.

Here's the few that I've found:
golang/go@e13df02
golang/go@86b2f29
golang/go@707a833

@kardianos
Copy link
Contributor

@tamird I'll get a list together within a week or two and include it in a more general announcement. Maybe I'll start with opening a single issue on each database drivers that link to a living google docs that contains a list to all the different interfaces / descriptions.

@freeformz
Copy link

That would be awesome.

On Mon, Oct 17, 2016 at 11:48 AM Daniel Theophanes notifications@github.com
wrote:

@tamird https://github.com/tamird I'll get a list together within a
week or two and include it in a more general announcement. Maybe I'll start
with opening a single issue on each database drivers that link to a living
google docs that contains a list to all the different interfaces /
descriptions.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#425 (comment), or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAAAZ0gGd2om_u2_Cwhc74-r2cMtQzeYks5q08MTgaJpZM4HRqaH
.

@c4milo
Copy link

c4milo commented Jan 25, 2017

Is this PR still relevant with the upcoming Go 1.8 release supporting multiple result sets?

@maddyblue
Copy link
Collaborator

No, this PR isn't needed. We previously merged this support with another implementation: #532

@petermattis petermattis deleted the pmattis/multiple-results branch January 25, 2017 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants