sql + pgwire: provide ways for clients to discover the CockroachDB version

[knz]

Fixes #14142.

sql/pgwire: report the CockroachDB version in the pgwire parameter message.

Discussed in MagicStack/asyncpg#87 (comment)

It is useful for clients to know that they are actually talking to a
CockroachDB SQL node, as opposed to a real PostgreSQL server. At this
point there is no way to determine a client is connected to
CockroachDB using a SQL statement/query that would not otherwise
fail in PostgreSQL.

This patch addresses this limitation by returning a new crdb_version
parameter in the ParameterStatus
message. (https://www.postgresql.org/docs/9.5/static/protocol-message-formats.html)

This is safe because as explained in
https://www.postgresql.org/docs/9.5/static/protocol-flow.html#PROTOCOL-ASYNC
"This set might change in the future, or even become
configurable. Accordingly, a frontend should simply ignore
ParameterStatus for parameters that it does not understand or care
about."

sql: report the CockroachDB information details also in a virtual table.

The changes in the previous commit have enabled pgwire clients to
inspect the CockroachDB version from the parameter status message.
However this is only reported during connection start-up, and client
drivers may not be able to save this information (e.g. Go's lib/pq
doesn't).

This patch complements the previous change by reporting the version
details also in a new virtual table crdb_internal.local_version. A
client that wants to disambiguate between pg and CockroachDB can
select into that table. To avoid this query to fail on pg, the client
may first check using information_schema.schemata whether this table
exists.

---

This change is 

[tamird]

nit: second commit message should not reference the first; just describe the commit in isolation.

---

Reviewed 2 of 2 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/acceptance/c_test.go, line 491 at r1 (raw file):

}

const cVersion = `

I'd probably include this in the other test rather than adding a new one.

---

pkg/sql/pgwire/v3.go, line 258 at r1 (raw file):

	// The current CockroachDB version string.
	"crdb_version": func() string {
		info := build.GetInfo()

why not get this at init?

---

Comments from Reviewable

[knz]

Review status: 2 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/acceptance/c_test.go, line 491 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I'd probably include this in the other test rather than adding a new one.

The other test is ran two times (one with a good query, one in a failing query) -- it didn't seem to be a good fit.

---

pkg/sql/pgwire/v3.go, line 258 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why not get this at init?

You mean var statusReportParams map[string]string; func init() { statusReportParams = ... }
You were the one actually who taught me the wisdom of doing the initialization work for global variables in their initializer... Is that not the right choice here?
(pgwire does not yet have an initializer)

---

Comments from Reviewable

[jordanlewis]

---

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

---

pkg/acceptance/c_test.go, line 491 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I'd probably include this in the other test rather than adding a new one.

I was going to suggest this as well originally, but that test is getting pretty big and gnarly. I would be a fan of a refactor to this test (and others like it) that pull out the inlined programs into their own files, with a corresponding test harness update. I'm not suggesting we do that in this PR though.

---

pkg/sql/crdb_internal.go, line 46 at r4 (raw file):

var crdbInternalVersionTable = virtualSchemaTable{
	schema: `
CREATE TABLE crdb_internal.local_version (

bikeshed: I'm not sure the name local_version matches the contents. Maybe build_info?

---

Comments from Reviewable

[tamird]

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

---

pkg/acceptance/c_test.go, line 491 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I was going to suggest this as well originally, but that test is getting pretty big and gnarly. I would be a fan of a refactor to this test (and others like it) that pull out the inlined programs into their own files, with a corresponding test harness update. I'm not suggesting we do that in this PR though.

Well there's a reason for that; we are testing (there) that we can actually detect a failing test. Currently, this test does not include that machinery, so for all we know, it could be exiting zero regardless of the implementation.

---

pkg/sql/pgwire/v3.go, line 258 at r1 (raw file):

Previously, knz (kena) wrote…

You mean var statusReportParams map[string]string; func init() { statusReportParams = ... }
You were the one actually who taught me the wisdom of doing the initialization work for global variables in their initializer... Is that not the right choice here?
(pgwire does not yet have an initializer)

Ah, I misread; this is good as-is.

---

Comments from Reviewable

[knz]

Review status: 3 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

pkg/acceptance/c_test.go, line 491 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Well there's a reason for that; we are testing (there) that we can actually detect a failing test. Currently, this test does not include that machinery, so for all we know, it could be exiting zero regardless of the implementation.

I'm not sure I'm following the conversation here.
In TestDockerC there is currently conceptually a single test: executing a single query via the C driver.
The test is ran two times, once with a query that succeeds given its parameters, and one that tests the driver rejects execution of the query if parameters are set while the SQL doesn't contain placeholders.
I do not believe TestDockerC can be further split up.

To come back to the issue at hand, what do you suggest I do in this PR?

---

pkg/sql/crdb_internal.go, line 46 at r4 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

bikeshed: I'm not sure the name local_version matches the contents. Maybe build_info?

Done.

---

Comments from Reviewable

[tamird]

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

---

pkg/acceptance/c_test.go, line 491 at r1 (raw file):

Previously, knz (kena) wrote…

I'm not sure I'm following the conversation here.
In TestDockerC there is currently conceptually a single test: executing a single query via the C driver.
The test is ran two times, once with a query that succeeds given its parameters, and one that tests the driver rejects execution of the query if parameters are set while the SQL doesn't contain placeholders.
I do not believe TestDockerC can be further split up.

To come back to the issue at hand, what do you suggest I do in this PR?

Two options:

• Modify the existing test to include this check
• Modify this new test to include a second variant so we can test that it fails when we expect it to fail (by interopolating a different "expected string", say).

---

Comments from Reviewable

[knz]

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

---

pkg/acceptance/c_test.go, line 491 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Two options:

• Modify the existing test to include this check
• Modify this new test to include a second variant so we can test that it fails when we expect it to fail (by interopolating a different "expected string", say).

Thanks for explaining. Done.

---

Comments from Reviewable

[tamird]

---

Reviewed 1 of 4 files at r3, 1 of 4 files at r5, 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[knz]

TFYRs!