sql: make internal executor streaming

[yuzefovich]

This commit updates the internal executor to operate in a streaming
fashion by refactoring its internal logic to implement an iterator
pattern. A new method QueryInternalEx (and its counterpart
QueryInternal) is introduced (both not used currently) while all
existing methods of InternalExecutor interface are implemented
using the new iterator logic.

The communication between the iterator goroutine (the receiver) and the
connExecutor goroutine (the sender) is done via a buffered (of 32 size
in non-test setting) channel. The channel is closed when the
connExecutor goroutine exits its run() loop.

Care needs to be taken when closing the iterator - we need to make sure
to close the stmtBuf (so that there are no more commands for the
connExecutor goroutine to execute) and then we need to unblockingly
drain the channel (since the connExecutor goroutine might be blocked on
adding a row to the channel). After that we have to wait for the
connExecutor goroutine to exit so that we can finish the tracing span.
For convenience purposes, if the iterator is fully exhausted, it will
get closed automatically.

Addresses: #48595.

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

bors r-

I think we can improve the semantics around closing the channel.

[craig]

Canceled.

[yuzefovich]

@jordanlewis could please take another quick look at the change on when the channel is closed?

[yuzefovich]

Added another commit that improves the short-circuiting behavior. PTAL.

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @jordanlewis)

---

pkg/sql/internal.go, line 343 at r3 (raw file):

	// We also need to exhaust the channel since the connExecutor goroutine
	// might be blocked on sending the row in AddRow().
	// TODO(yuzefovich): at the moment, the connExecutor goroutine will not stop

Had to remove the second commit that tried to address it via deriving a child context before instantiating the connExecutor and canceling it here, in Close(). The problem is that the context cancellation error might be sent on the channel, and I don't think it is possible to distinguish the error between "external" (the user of the iterator API cancels the context) which should be returned and "internal" (caused by us canceling the context). Any ideas on how to address this TODO?

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei, @jordanlewis, and @yuzefovich)

---

pkg/sql/internal.go, line 766 at r1 (raw file):

But I think it's strictly worse than what we have here in terms of potential for true streaming usage, isn't it?

It's about the latency-to-first-result vs automatic retries tradeoff. Letting the caller specify the buffer size would put it in control of this tradeoff.

Unless we want to expose some kind of contract that forces the users of the internal executor to choose their portal limit, which feels wrong (too complex).

The interface we should be shooting for, in my opinion, should be either the go sql package driver interface, or pgwire. Implementing the streaming in the form of portals would keep the door open to using this internal executor through libpq or go sql. I think this channel-based results communication moves us away from that, but I'm not sure.

[jordanlewis]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @yuzefovich)

---

pkg/sql/internal.go, line 766 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

But I think it's strictly worse than what we have here in terms of potential for true streaming usage, isn't it?

It's about the latency-to-first-result vs automatic retries tradeoff. Letting the caller specify the buffer size would put it in control of this tradeoff.

Unless we want to expose some kind of contract that forces the users of the internal executor to choose their portal limit, which feels wrong (too complex).

The interface we should be shooting for, in my opinion, should be either the go sql package driver interface, or pgwire. Implementing the streaming in the form of portals would keep the door open to using this internal executor through libpq or go sql. I think this channel-based results communication moves us away from that, but I'm not sure.

I think your opinion is reasonable. But the burden to do this again is quite high. The proximal goal, now solved, of correcting the unbounded memory usage is by far the worst problem that we've had with the internal executor domain, IMO. I think from a strict priority POV, these other reasonable suggestions can come later.

---

pkg/sql/internal.go, line 343 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Had to remove the second commit that tried to address it via deriving a child context before instantiating the connExecutor and canceling it here, in Close(). The problem is that the context cancellation error might be sent on the channel, and I don't think it is possible to distinguish the error between "external" (the user of the iterator API cancels the context) which should be returned and "internal" (caused by us canceling the context). Any ideas on how to address this TODO?

Hmm... I guess you could try to wrap the internal version and detect it specially, or the other way around? I don't know. What are the consequences of this TODO? Leaked goroutines are quite bad, will we leak goroutines? Or will execution just take a while to finish in some cases?

I think in general it's not great to have operations that don't cancel when top level contexts are canceled, because users expect that their cancellations trickle all the way down.

I'm okay merging this for now to get this work done. But you should make an issue and plan to do it soon, I think.

[yuzefovich]

TFTRs!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @jordanlewis)

---

pkg/sql/internal.go, line 343 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Hmm... I guess you could try to wrap the internal version and detect it specially, or the other way around? I don't know. What are the consequences of this TODO? Leaked goroutines are quite bad, will we leak goroutines? Or will execution just take a while to finish in some cases?

I think in general it's not great to have operations that don't cancel when top level contexts are canceled, because users expect that their cancellations trickle all the way down.

I'm okay merging this for now to get this work done. But you should make an issue and plan to do it soon, I think.

The consequences are only performance-related.

Imagine that we have a long-running query like SELECT * FROM t executed via the iterator API, but we are only interested in the first few rows. Once the caller satisfies its limit, it'll call iterator.Close() to finish early. However, the query execution (i.e. of ExecStmt command currently being executed by the connExecutor) will not stop right away, still all of the remaining rows will be pushed onto the channel, and the iterator will drop them on the floor here, in Close(). This TODO is about improving this situation so that the connExecutor short-circuited the execution of the current command.

To be clear, this is not about leaked goroutines; and if the caller cancels the context, that will stop the connExecutor goroutine too, so we are listening for top level context cancelation.

I'll file an issue to track addressing this, but I currently don't see an easy way to address this TODO.

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• GitHub CI (Cockroach)