sql/pgwire: build batches by reading as much as possible without blocking

[bdarnell]

Some client drivers (including JDBC, as can be seen in the jepsen packet captures) pipeline multiple requests into one packet (most often for combining BEGIN with the first real request of a transaction). For example, this packet contains two parse/bind/execute sequences:

Transmission Control Protocol, Src Port: 60190 (60190), Dst Port: 26257 (26257), Seq: 356, Ack: 241, Len: 89
PostgreSQL
    Type: Parse
    Length: 13
    Statement: 
    Query: BEGIN
    Parameters: 0
PostgreSQL
    Type: Bind
    Length: 12
    Portal: 
    Statement: 
    Parameter formats: 0
    Parameter values: 0
    Result formats: 0
PostgreSQL
    Type: Execute
    Length: 9
    Portal: 
    Returns: all rows
PostgreSQL
    Type: Parse
    Length: 16
    Statement: 
    Query: select 1
    Parameters: 0
PostgreSQL
    Type: Bind
    Length: 12
    Portal: 
    Statement: 
    Parameter formats: 0
    Parameter values: 0
    Result formats: 0
PostgreSQL
    Type: Describe
    Length: 6
    Portal: 
PostgreSQL
    Type: Execute
    Length: 9
    Portal: 
    Returns: all rows
PostgreSQL
    Type: Sync
    Length: 4

If we consumed everything that was available on the socket before executing any statements, we could build up larger batches even though the pgwire protocol has no explicit concept of batching. This is a lower-priority followup to #4549.

Jira issue: CRDB-6198

[spencerkimball]

@bdarnell what are your thoughts on this PR, given #14188? Should we close?

[bdarnell]

This is is not resolved by #14188, although it is related. This issue would allow us to consider more queries as candidates for parallelizing because we would know that they were sent before the response to the previous query. However, it's unclear to me whether there is any real benefit to doing this. The trace above is a BEGIN and a real query, and that case was improved by #5740. I don't know that any clients will actually issue queries in a form that this proposal would benefit, so I'm just going to close it.

[bdarnell]

This has gotten more interesting now that we automatically retry the first "batch" of a transaction. Currently to count as a single batch the statements must come in as a single stream. Some clients, however, have the ability to pipeline multiple statements (in multiple Execute frames) into a single packet. Improving our ability to batch these statements would reduce the scope of retryable errors that must be returned to the client.

[andreimatei]

I believe that the answer here is that our query execution (the sql Executor) should move away as much as possible from caring about "batches". We should treat the pgwire protocol as a streaming protocol; the "batching" of statements done by clients shouldn't matter for much (except some semantics about what needs to not be run when an error occurs). Our automatic retries should be driven entirely by what results we have already delivered to the client (and so we can't "take back"). This should be controlled by a policy balancing the desire to return results quickly with the desire to leave the door open to automatic retries. Batching done by the client, among with other information about statements that are queued for execution, can influence this policy. This policy should be implemented above statement execution.

cc @knz with whom I was discussing similar pipe dreams recently. We have plans to make our SQL sessions look like cleaner state machines, and I believe this will open the door to the pgwire module interacting with them with more flexibility.

[andreimatei]

I have just discovered something interesting: while the pgwire protocol has no explicit concept of batching, libpq (the C driver) has recently gotten such explicit support: http://2ndquadrant.github.io/postgres/libpq-batch-mode.html

It is implemented how Ben saw in that jdbc trace (in fact, I think the jdbc implementation was used as an example): the client doesn't send a sync message until the whole batch has been sent, and so it relies on the server to skip pipelined statements after an error is encountered (until the sync is seen).

[andreimatei]

Since https://github.com/cockroachdb/cockroach/pull/22277 automatic retries no longer depend on using the simple vs extended protocol.

[andreimatei]

I'm folding #17289 into this one. There, @petermattis observed that there's a perf difference between an implicit and an explicit txn:

UPDATE test.rmw SET data='%s' WHERE id='%s'

is faster than

BEGIN; UPDATE test.rmw SET data='%s' WHERE id='%s'; COMMIT;

This is presumably because the former runs as 1PC. We should be smart enough to execute the latter as 1PC too.

[github-actions[bot]]

We have marked this issue as stale because it has been inactive for 18 months. If this issue is still relevant, removing the stale label or adding a comment will keep it active. Otherwise, we'll close it in 10 days to keep the issue queue tidy. Thank you for your contribution to CockroachDB!