sql: stream planNode.Next() results through pgwire without collecting all intermediate results in RAM

[knz]

As discussed previously with @mjibson. Will help support work on #7572 and #7657.

[maddyblue]

For whoever does this work: I think implementing the streaming part and returning an error when needed is straightforward. I'm mostly worried about client error handling, though, and think that this deserves more research before we can ship a PR. Mostly I want someone to have thought through the following situation and be convinced that we're ok.

Let's take some app that uses this feature. It runs a SELECT and starts reading through the result rows, and for each row returned the app produces some side effect. Then after some number of rows the SELECT is aborted or errors for any reason. Those side effects have now already happened. With our current code, the error is detected first and so no side effects are produced. A user now has to figure out what to do. They can re-run the SELECT, but the results may be different, and thus have produced some erroneous side effects. How can we handle this?

However, thinking through this it's possible this exact problem is already possible. Today, the server buffers all rows in SQL, then sends them up to the pgwire layer, which sends each row off to the client. A connection or other error can also occur here, resulting in exactly the same situation. So maybe we won't be worse off than today, so the above problem isn't a real problem?

[knz]

Really I'd argue it's already a (non-)problem, because of transactions.

We already report errors before the end of a transaction. So if a transaction gets canceled, whatever side-effects on the client side that occured since the beginning of the transaction need to be undone anyway. I'd argue that any well designed MVC client code will always buffer/process data from the server (and wait for txn completion) before presenting data to the user and/or effecting irreversible side-effects.

[knz]

Ok I have investigated this more. Explaining here as requested by @petermattis.

@andreimatei you may want to chime in since the main focus here ends up revolving around autoretries and API.

The issue to solve here is not that clients may be confused to receive an error after some data has been communicated already. It is actually valid in pgwire (and other sql wire protocols) to start returning results and then fail before the statement has completed.

The main issue I found is that the API between pgwire and Executor is currently a function that drives the entire SQL transaction state, the mapping between the SQL transaction state and KV transaction state, and the retry logic. To change this to a streaming interface is not simple. It amounts to an API change that will break the abstraction boundary and cause headaches with retries.

A naive idea, not to be considered seriously but related here for contrast with the other idea below, is to change the Executor API to return a generator of planNodes to pgwire, to be driven via Start/Next etc by pgwire. However if/when an error occurs or anything like a transaction boundary in the middle of the SQL syntax, the Executor must be informed of this accordingly by pgwire (during the progress of the stream of planNodes). This would amount to a complex bi-directional interaction between pgwire and Executor, ie too complex for what we want to maintain.

The more promising direction is to have pgwire pass a closure to Executor, like a "sender function" that the Executor can use to actively send the results towards pgwire during ExecStatements.

Now the problem with this is what to do with transaction retries. The main issue is that as soon as some data has flown on the wire from the executor to the client, a retry becomes impossible. IF we naively start to stream data as soon as a statement produces data, no implicit retry becomes ever possible again for out-of-txn SELECT or INSERT/UPDATE/UPSERT ... RETURNING. So the better solution, assuming we want to keep autoretries, would be to start accumulating results locally in the server, and only stream that back to the client as soon as some local capacity limit has been reached, and limit autoretry to those statements for which the results has fit entirely in the buffer (and have not been communicated back to the client yet).

Overall all this looks pretty doable with regards to functionality but there will be a lot of corner cases for testing, namely what happens with this buffering of rows in the presence of client errors and txn errors, or when there is a transaction boundary inside a single SQL string (like "... COMMIT; BEGIN ...").

[petermattis]

Passing some sort of interface or closure to ExecuteStatements sounds reasonable to me, though I haven't looked at this code closely in a while. Sort of mirrors the approach of passing an io.Writer to some function that wants to write data rather than having that function return a []byte or a string.

[bdarnell]

This would amount to a complex bi-directional interaction between pgwire and Executor, ie too complex for what we want to maintain.

Why is this prohibitively complex? There's clear precedent for this kind of API in database/sql.Rows which uses a Start/Next/Close interface. Or anything that takes a cancelable context.Context has a similar level of bidirectional interaction.

Passing an interface or closure to handle the rows is restrictive in its own way - the connection can't signal the executor that the client has gone away except when the executor has a row available. If the query is expensive (e.g. a select that does a full table scan with a filter that discards nearly all rows), we'd like the pgwire connection to be able to tell the executor that the client has gone away instead of continuing to run the command.

[spencerkimball]

@knz, assigning you. Please re-assign, remove 1.0 milestone as you see fit.

[knz]

New milestone as discussed with @cuongdo

[cuongdo]

Now that @tristan-ohlson is on this, this will be part of the 1.1 release.

cc @knz @andreimatei @RaduBerinde @jordanlewis