cli: add support for multiple results to SQL statements

[petermattis]

Use the new functionality in lib/pq to select the next set of results
when multiple statements were executed.

Switch to using the "postgres" SQL driver in the cli tests.

Remove runPrettyQueryWithFormat. It was only used in tests.

[petermattis]

This PR depends on upstream changes to lib/pq. I'm sending it out to feedback on the API to retrieve multiple results. There are not a whole lot of options here as the database/sql package does not provide access (even via interfaces) to the structures used by the driver (e.g. lib/pq).

The interface to retrieve all of the results to a multiple statement query is to execute the query NEXT and check for the error pq.ErrNoMoreResults. NEXT is not a valid SQL. The driver looks for that query and either reads another result set from the connection or returns an error. If there are additional results on the connection and another query is sent, those results are discarded.

[petermattis]

See lib/pq#425 for lib/pq changes.

[petermattis]

This is ready for a look.

[tamird]

is this whitespace? weird

[petermattis]

Appears to be just some sort of weird error in the diff computation. I don't think there are any whitespace differences. And the file was generated via glock save.

[bdarnell]

There's a space at the end of the line in the "before" version.

[tamird]

not sure this is worth it, but code LGTM

[petermattis]

I think supporting multiple results is worth the effort. An alternative to the pq.NextResults API would be to manually open a pq connection using pq.Open. This would remove the need for the DB.SetMaxConns(1) hack and open the possibility of a slightly friendlier API. The downside is that the driver.Conn API is not as friendly as the sql.DB API.

[bdarnell]

What if we introduced a new interface that extends sql.DB in the same way that io.ReaderAt extends io.Reader?

type MultiQuerier interface {
    sql.DB
    MultiQuery(query string, args ...interface{}) ([]*Rows, error)
}

This might be more palatable upstream than the "NEXT" hack, and puts everything behind one method call so there is no question about connections being swapped out from under you (requiring SetMaxConns(1) seems like a high price to pay, although it's fine for our sql shell). I have no idea how hard it would be to make this work with the layers of database/sql and database/sql/driver abstractions, though.

The needs of our SQL shell are sufficiently different from a normal application that it might make sense for it to use a lower-level API than sql.DB.

Or another crazy idea: we could support a special query on the server PARSE STATEMENTS '...' which would return each statement contained within the string as a separate row. The client could call this whenever it has a string containing ;, and then it would know where the statement boundaries are without having to link in the parser.

[petermattis]

The pgwire protocol doesn't tell you how many result sets there are upfront. I'm not seeing how I would implement the MultiQuery interface.

If we don't mind avoiding the sql.DB interface, then going directly to a pq.Conn would be my suggestion.

[petermattis]

Thoughts on how to proceed? The approach taken here is very contained, and while not a particularly pretty interface, it isn't fragile either since we limit the number of DB connections from the cli to 1.

[bdarnell]

I think this should continue to recommend lib/pq instead of our fork; most clients would be better off sticking with the standard version instead of this modified version given its limitations. Use of cockroachdb/pq should be limited to the cli package.

[bdarnell]

What's the impetus for supporting multiple queries in the CLI? We support them on the server side so it will work for (non-go) programmatic clients. I can see that it's a good thing to support in the abstract, but I was unaware until now that this was supported in other CLIs and the cost seems high relative to the benefit.

If we can do this with a pq.Conn without making significant changes to lib/pq, then I'd do that. Otherwise, I'd probably do without the feature unless there's a compelling reason I'm not seeing.

[petermattis]

The impetus for supporting multiple queries in the CLI is that we can't easily prohibit the user from entering them and once such a query is executed it is surprising to not get back all of the results.

We can't support multiple queries without a change to lib/pq. The change was not huge, but so far there does not seem to be appetite to merge it upstream. See cockroachdb/pq#1. That PR not only provides an interface for retrieving multiple results, but fixes the handling of any query which generates multiple results.

Maintaining a fork of lib/pq doesn't seem particularly onerous. That package is not undergoing any significant development.

[bdarnell]

But we also can't prevent people from entering two update statements (which should return two counts of affected rows to satisfy #3993) or a query and an update. The sql.DB interface is a poor fit for use in an interactive console because it requires you to programmatically know whether you're dealing with a query or other statement. Hacking in a "NEXT" command to Query() only solves part of the problem.

We'll need to either use a lower-level interface that can give us either a result set or a row count without us knowing in advance which to expect (can pq.Conn do this?) or parse the query to determine both how many statements are in it and whether those statements need to be passed to Query() or Execute().

[petermattis]

Yes, we'd need to do something additional to handle #3993. It would require some straightforward changes to cockroachdb/pq and for us to move away from the sql.DB interface to direct usage of pq.Conn and pq.rows. Both straightforward and doable if we want to do it.

[bdarnell]

I'd be in favor of moving to direct usage of pq.Conn; it would solve multiple issues and the necessary changes to lib/pq seem like they'd be more palatable to upstream than cockroachdb/pq#1.

[petermattis]

Cc @cuongdo.

[tamird]

What's the status of this?

---

Reviewed 9 of 14 files at r1, 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[petermattis]

On hold for now. @cuongdo or I need to pick it up again. Maybe next week.

---

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

---

Comments from the review on Reviewable.io

[cuongdo]

Will take a look next week.

On Thu, Feb 18, 2016 at 8:20 AM Peter Mattis notifications@github.com
wrote:

On hold for now. @cuongdo https://github.com/cuongdo or I need to pick

it up again. Maybe next week.

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

discussions, some commit checks failed.

Comments from the review on Reviewable.io
https://reviewable.io:443/reviews/cockroachdb/cockroach/4081#-:-KAoXOHeO-C86cl2Yceu:1989209636

—
Reply to this email directly or view it on GitHub
#4081 (comment)
.

[petermattis]

This is ready for another look. I'm planning to address #3993 in a follow-on PR.

[bdarnell]

LGTM

---

Review status: 1 of 14 files reviewed at latest revision, 4 unresolved discussions.

---

cli/sql_util.go, line 35 [r3] (raw file):
If we're going to actually use this string then we should export it from pq. But I thought it was going to use the Next() method now.

---

sql/pgwire_test.go, line 37 [r3] (raw file):
Do we anticipate other changes than those motivated by the unique needs of the CLI shell? If not, I think I'd rather use lib/pq in most cases and only use our fork in the cli package. In any case, we should have at least one test that verifies that we work with the standard lib/pq driver (probably in acceptance)

---

Comments from the review on Reviewable.io

[petermattis]

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

---

cli/sql_util.go, line 35 [r3] (raw file):
We don't send this string to pq, but are using this internally. See the changes to runQueryWithFormat. Any suggestions on how to pass something to that function to indicate we want the next set of results?

---

sql/pgwire_test.go, line 37 [r3] (raw file):
Well, we can have cockroach/pq expose the command tags as mentioned in one of the comments below. Agreed that we need some tests that lib/pq works. Let me see what I can do.

---

Comments from the review on Reviewable.io

[bdarnell]

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

---

cli/sql_util.go, line 35 [r3] (raw file):
Ah, I see. I'd rather add a new boolean argument to runQueryWithFormat than have a magic value ofquerythat changes its behavior (unless perhaps if the magic value is an empty string). Or we could replace theconn,query, andparametersarguments with a closure which would encapsulate the call toQueryorNext:func runQueryWithFormat(format fmtMap, query func() (driver.Rows, error))`

---

sql/pgwire_test.go, line 37 [r3] (raw file):
Hmm, exposing more implementation details for testing seems like a good enough reason.

Should cockroachdb/pq register itself under another name than postgres so it can be linked into the same binary as lib/pq? Otherwise we'll need to be careful about how we test this.

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 1 of 16 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

cli/sql_util.go, line 35 [r3] (raw file):
I could certainly make the magic value the empty string. Let me try the closure approach.

---

sql/pgwire_test.go, line 37 [r3] (raw file):
I switched back the acceptance tests to use lib/pq so we will get some testing there. Yeah, perhaps we should change the name cockroach/pq registers itself under. We could use cockroach as we were with sql/driver.

---

Comments from the review on Reviewable.io

[petermattis]

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

---

cli/sql_util.go, line 35 [r3] (raw file):
Pushed another commit containing the closure approach. PTAL.

---

client/doc.go, line 19 [r2] (raw file):
Done.

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 1 of 15 files reviewed at latest revision, 4 unresolved discussions.

---

cli/cli_test.go, line 509 [r5] (raw file):
This format is up for discussion. It would be easy to switch to something like 1 row, but I found that less descriptive than including the command tag like this. In particular, the tag is useful for statements like CREATE TABLE (see below) which do not affect any rows.

---

Comments from the review on Reviewable.io

[petermattis]

I think this PR has already received sufficient LGTMs, but it underwent a few minor tweaks. I'll be merging in a little bit, but am happy to address any issues in a follow-on PR.