sql: send ParameterStatus over pgwire using pgwireconn.ClientBuffer

[otan]

We were previously sending ParameterStatus using an async listener. We
eventually want to send NoticeResponse
(https://www.postgresql.org/docs/9.2/protocol-error-fields.html) over
pgwire, so it makes sense to give sql.EvalContext the ability to talk to
pgwire directly using dependency injection.

This code refactors the code to allow sql to talk to pgwire using
pgwireconn.ClientBuffer.

A number of notes:

• This needs to be in a new package as pgwirebase would otherwise have a
  cyclic dependency on the tree package.
• Refactored the pgwire code out of SessionMutator and made it
  explicitly only made calls for parameter status update on the set var
  and set_config builtin. This separates the SessionMutator from doing
  too much responsibility.

No release note as there is no functional difference.

Release note: None

[cockroach-teamcity]

This change is 

[knz]

What is it with some folk that you always seem to want put everything under the sun in a "God class"?

tree.EvalContext is dedicated to scalar expression evaluation. Does scalar eval has anything to do with pushing a message to a pgwire client? No? Then don't put it in EvalContext.

(Ditto SemaContext. That's for type checking, not for talking through pgwire. So it won't go to SemaContext either.)

Now maybe we have a candidate in the extendedEvalContext. That might fit the bill, it's only used for the interaction between gateway-local statements (that matches all the DDL + SET and other things) and the conn executor. But I'm not sure.

If we're missing a struct with a particular scope, maybe it's possible to create it.

Reviewed 12 of 13 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)

[otan]

What is it with some folk that you always seem to want put everything under the sun in a "God class"?

simile aside, it's because I thought the set_config pg builtin needed direct access to notices (or in fact, any other builtin) -- but turns out when i got it working in the end I didn't. I will move this to extendedEvalCtx for now.

[andreimatei]

If possible, I think going through the CommandResult would be much better than creating a new interface into pgwire. Can't you put a CommandResult in the extendedEvalContext?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @otan)

---

pkg/sql/conn_io.go, line 569 at r2 (raw file):

// ClientComm is implemented by the pgwire connection.
type ClientComm interface {
	pgwireconn.ClientBuffer

I think it'd be better if this interface was part of a CommandResult, no ClientComm, even if you have to overwrite SessionDataMutator.<foo> at the beginning of each statement.
The idea is that I'd like to scope these updates that need to be sent to the client to a single result - basically by observing the result, you should be able to observe all the bytes that get sent to the client for the execution of a statement. Besides being conceptually cleaner, this kind of scoping would also be useful if we need to make any of the session variable updates transactional, for example.

---

pkg/sql/internal.go, line 635 at r2 (raw file):

// silentClientBuffer is an implementation of pgwire.ClientBuffer
// that does not do anything. This is intended for internal executors.
type silentClientBuffer struct{}

nit: noopClientBuffer

---

pkg/sql/planner.go, line 52 at r2 (raw file):

	// ClientBuffer is used for communication between possible pgwire clients.
	ClientBuffer pgwireconn.ClientBuffer

How come the planner and the extendedEvalContext need to know anything about this? Isn't the reference in SessionDataMutator enough?

---

pkg/sql/vars.go, line 60 at r2 (raw file):

	Get func(evalCtx *extendedEvalContext) string

	// BufferClientConn is optionally set.

Why do we need a new method for this / why can't Set() do this?

[otan]

Originally I didn't put it in CommandResult as I couldn't find a way to thread that object through to either the planner or the eval ctx in https://sourcegraph.com/github.com/cockroachdb/cockroach@3dff825/-/blob/pkg/sql/conn_executor.go#L564:15

For reference I wanted to use it somewhere such that I could re-use this for notices, e.g. #44884.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @knz, and @otan)

---

pkg/sql/conn_io.go, line 569 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think it'd be better if this interface was part of a CommandResult, no ClientComm, even if you have to overwrite SessionDataMutator.<foo> at the beginning of each statement.
The idea is that I'd like to scope these updates that need to be sent to the client to a single result - basically by observing the result, you should be able to observe all the bytes that get sent to the client for the execution of a statement. Besides being conceptually cleaner, this kind of scoping would also be useful if we need to make any of the session variable updates transactional, for example.

I want to use this in other places that aren't necessarily session data mutators tho, e.g. #44884.

---

pkg/sql/vars.go, line 60 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Why do we need a new method for this / why can't Set() do this?

I originally did this because at init time, we send the session data already in a separate loop elsewhere (https://sourcegraph.com/github.com/cockroachdb/cockroach@cd28ab7553484120304e939208a28d6352460ee3/-/blob/pkg/sql/pgwire/conn.go#L604:15), and putting it in Set would buffer and output it twice.

[otan]

I could change the approach entirely and get this to work in different ways for setting session vars and notices [where you can call a notice from anywhere] and was trying to kill two birds with one stone in this approach but now that you mention transactions and the ilk maybe it's worth changing tacks.

[otan]

alright, going to use CommandResult to do this. separate PR coming out shortly.