sql/distsqlrun: refactor tableReader to implement RowSource

[petermattis]

Refactor tableReader to implement the RowSource interface. Refactor
tableReader.Run() to be implemented in terms of
tableReader.Next() (i.e. the RowSource interface).
See #20550

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

I will reiterate that I believe we should first work on row batching before going down this path because 1) it is definitely necessary regardless of other improvements and 2) it could reduce the benefit of this kind of change. Overall the change itself seems fine but if we make more changes to remove a row channel here and there, things could easily become very messy.

---

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[andreimatei]

+1 to batching first :)

---

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[andreimatei]

And there's another thing - there's been a series of triparty talks between Radu, Rafa and myself about supporting synchronous scheduling of co-located processors. You'd probably end up with something similar to what you have here, but hopefully done in a more general way. I was hoping that line of thought would marinate as we work towers unifying the execution engines. BTW, I applaud the development of benchmarks like the one that lead to this PR.

…

On Dec 8, 2017 6:11 PM, "Andrei Matei" ***@***.***> wrote: +1 to batching first :) ------------------------------ Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/20584#-:-L-sOAg0sMjNNc1mfPW6:b-n6wl6h>* — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#20584 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXBcWhUw0482VhZl7-NtxD7npCfM5Ujks5s-cIIgaJpZM4Q7tAB> .

[petermattis]

Note that I hacked this up to see what could be accomplished. I'm not in anyway proposing this as the right approach. Passing batches of rows will lessen the need for removing inter-processor communication speed, but not eliminate it. And this PR gives us an indication of the possible speed-up from row batching (assuming we don't change processor internals at the same time).

@andreimatei How would synchronous scheduling of co-located processors work? Would it be any different from pushing this PR to its conclusion and eliding RowChannel between co-located processors whenever possible? Probably better to discuss on #20550.

[rjnn]

Another thing to note is that making the underlying MVCCScan go much faster probably exacerbates the difference in performance between the two approaches, making the need for eliding RowChannels where possible more critical.

---

Comments from Reviewable

[petermattis]

This is prep work for #20550, without actually taking the step of eliding RowChannels. The plan is to follow-up with similar refactorings of the other processors. Note the first commit is all context plumbing work. PTAL.

---

Review status: 0 of 26 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/sql/distsqlrun/tablereader.go, line 374 at r2 (raw file):

func (tr *tableReader) ConsumerDone() {
	// TODO(peter): what to do here?

@RaduBerinde Any insight on what to do here? The various RowSource implementations which do something for this method only utilize the effects of that something in RowReceiver.Push. So it seems fine to do nothing here.

---

Comments from Reviewable

[RaduBerinde]

Review status: 0 of 26 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/distsqlrun/tablereader.go, line 314 at r2 (raw file):

}

func (tr *tableReader) Next(ctx context.Context) (sqlbase.EncDatumRow, ProducerMetadata) {

Currently the table reader calls tr.sendMisplannedRangesMetadata and sendTraceData before finishing. Without something similar here (returning that metadata after the last row) we wouldn't get traces or (more importantly) misplanned ranges info from the table reader.

---

pkg/sql/distsqlrun/tablereader.go, line 374 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

@RaduBerinde Any insight on what to do here? The various RowSource implementations which do something for this method only utilize the effects of that something in RowReceiver.Push. So it seems fine to do nothing here.

Currently, even if a consumer signals that it is done, it still "drains" the input (e.g. to get any remaining metadata). If that carries over, the consumer will keep running Next, so the right thing to do would be to set a flag and have the next Next call say it's done.

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/distsqlrun/tablereader.go, line 314 at r2 (raw file):

Previously, RaduBerinde wrote…

Currently the table reader calls tr.sendMisplannedRangesMetadata and sendTraceData before finishing. Without something similar here (returning that metadata after the last row) we wouldn't get traces or (more importantly) misplanned ranges info from the table reader.

Ah, I missed that. Is the contract that we'll send the misplanned ranges metadata and tracedata even if there is an error? Is there a reason this extra metadata isn't bundled into the same ProducerMetadata struct. Currently it looks like we send 2 separate ProducerMetadata.

PTAL at the logic both in Next() and Run(). There is lots of logic here which feels like it could be shared across processors.

---

Comments from Reviewable

[RaduBerinde]

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

---

pkg/sql/distsqlrun/tablereader.go, line 314 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ah, I missed that. Is the contract that we'll send the misplanned ranges metadata and tracedata even if there is an error? Is there a reason this extra metadata isn't bundled into the same ProducerMetadata struct. Currently it looks like we send 2 separate ProducerMetadata.

PTAL at the logic both in Next() and Run(). There is lots of logic here which feels like it could be shared across processors.

I think they can probably be packaged in the same ProducerMetadata.

I think we want the tracing data even if there's an error. Note that finishing early (through ConsumerDone) is not necessarily an error, it can happen because of a LIMIT (and we definitely want the metadata in that case).

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/distsqlrun/tablereader.go, line 314 at r2 (raw file):

Previously, RaduBerinde wrote…

I think they can probably be packaged in the same ProducerMetadata.

I think we want the tracing data even if there's an error. Note that finishing early (through ConsumerDone) is not necessarily an error, it can happen because of a LIMIT (and we definitely want the metadata in that case).

Yeah, packaging everything into a single ProducerMetadata seems to work. See tableReader.finalProducerMeta.

The changes here have gotten complex enough that I'm not at all comfortable with them without additional testing. I'd like to add some testing which verifies both the Processor and RowSource interface behaviors.

---

Comments from Reviewable

[RaduBerinde]

Just a meta-comment: I can easily see this series of changes turning into a can of worms and potentially causing a lot of conflicts with other ongoing work (row batching). Consider putting things on a separate branch for now (you can still post PRs for changes that go on that branch).

[petermattis]

Just a meta-comment: I can easily see this series of changes turning into a can of worms and potentially causing a lot of conflicts with other ongoing work (row batching). Consider putting things on a separate branch for now (you can still post PRs for changes that go on that branch).

That's a good point. I was hoping to do this processor-by-processor to avoid some uber merge nightmare. I think the row batching could take a similar approach.

[petermattis]

Ok, I think I have all of the semantics of RowSource implemented correctly now. The restructuring is now much less pleasant to look at, but I think the final code is reasonable. The PR now takes @andreimatei's suggestion and embeds a context in tableReader for use by Next(). When used as a RowSource, tableReader creates a span on the first call to Next() and finishes the span either when it is done sending rows or when ConsumerClosed() is called.

At this point, it looks feasible to implement tableReader.Run as:

func runProcessor(p Processor, out RowReceiver, wg *sync.WaitGroup)

---

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

---

pkg/sql/distsqlrun/flow.go, line 57 at r3 (raw file):

	// Context used for all execution within the flow.
	// Created in Start(), canceled in Cleanup().
	ctx context.Context

This was moved from Flow.ctx so that all processors have access to the flow's context. A follow-on change would be to remove the ctx parameter from Processor.Run.

---

pkg/sql/distsqlrun/tablereader.go, line 374 at r2 (raw file):

Previously, RaduBerinde wrote…

Currently, even if a consumer signals that it is done, it still "drains" the input (e.g. to get any remaining metadata). If that carries over, the consumer will keep running Next, so the right thing to do would be to set a flag and have the next Next call say it's done.

Done.

---

Comments from Reviewable

[petermattis]

Ping. I'd like to get this reviewed and merged piecemeal rather than changing all processors at once. This PR changes 3 of the 15 existing processors to implement RowSource, but does not otherwise change the interaction between processors (i.e. it does not elide RowChannel). Changing the remaining processors should not be difficult.

---

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

---

Comments from Reviewable

[RaduBerinde]

LGTM

@andreimatei should take a look too. And folks who are working on row batching should at least take a look at the new interface. CC @arjunravinarayan @asubiotto

---

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/tablereader.go, line 326 at r5 (raw file):

		}

		if tr.ctx == nil {

How can tr.ctx be anything but nil here? (given that !tr.started)

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/distsqlrun/tablereader.go, line 326 at r5 (raw file):

Previously, RaduBerinde wrote…

How can tr.ctx be anything but nil here? (given that !tr.started)

It can't. I think this was detritus from earlier versions of this change (before I added tr.started).

---

Comments from Reviewable

[andreimatei]

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/tablereader.go, line 57 at r6 (raw file):

	processorBase

	ctx     context.Context

please comment how ctx should be used instead of flowCtx.Ctx.

---

pkg/sql/distsqlrun/tablereader.go, line 201 at r6 (raw file):

func (tr *tableReader) Run(ctx context.Context, wg *sync.WaitGroup) {
	if tr.out.output == nil {
		panic("output RowReceiver not initialized for emitting rows")

this message is OK in ProcOutputHelper, but it's not great here. Just say "output not configured". Or, better yet, move the assertion to newTableReader().

---

pkg/sql/distsqlrun/tablereader.go, line 204 at r6 (raw file):

	}
	if ctx != tr.flowCtx.ctx {
		panic("unexpected context")

please add a TODO somewhere around here to remove the ctx arg. Or ideally do it now. Otherwise I don't really know what's going on here.

---

pkg/sql/distsqlrun/tablereader.go, line 229 at r6 (raw file):

	if tr.consumerStatus != ConsumerClosed {
		if meta := tr.producerMeta(nil); !meta.Empty() {

pls comment nils (some of them were commented before :) )

---

pkg/sql/distsqlrun/tablereader.go, line 312 at r6 (raw file):

	if tr.ctx != nil {
		if log.V(1) {
			log.Infof(tr.ctx, "exiting")

VEventf() instead of if log.v(1) please. I know it was bad before too.

---

pkg/sql/distsqlrun/tablereader.go, line 315 at r6 (raw file):

		}
		tracing.FinishSpan(tr.span)
		tr.ctx, tr.span = nil, nil

not sure resetting these is a good idea (it forces you to not use tr.ctx in other places although I think you could even with the finished span).

---

pkg/sql/distsqlrun/tablereader.go, line 324 at r6 (raw file):

// terminated, either due to being indicated by the consumer, or because the
// tableReader ran out of rows or encountered an error.
func (tr *tableReader) producerMeta(err error) ProducerMetadata {

pls comment that err can be nil

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

	var meta ProducerMetadata
	if tr.ctx != nil {
		meta = ProducerMetadata{

ProducerMetadata is specified to have a single fields set and has the following message:
// Only one of these fields will be set. If this ever changes, note that
// there's consumers out there that extract the error and, if there is one,
// forward it in isolation and drop the rest of the record.

---

pkg/sql/distsqlrun/tablereader.go, line 339 at r6 (raw file):

}

func (tr *tableReader) Types() []sqlbase.ColumnType {

how come the linter is not yelling at you?
Please comment that these methods are "part of the RowSource interface".

---

pkg/sql/distsqlrun/tablereader.go, line 343 at r6 (raw file):

}

func (tr *tableReader) Next() (sqlbase.EncDatumRow, ProducerMetadata) {

With processors implementing the RowSource iface, the comment on the Next method is no longer appropriate - it talks about rows pushed by a producer; it should be expanded.
And it should also say something about post-processing - that the processors have to do their post-processing before returning a row.

---

pkg/sql/distsqlrun/tablereader.go, line 418 at r6 (raw file):

func (tr *tableReader) ConsumerDone() {
	if tr.consumerStatus != NeedMoreRows {
		log.Fatalf(context.Background(), "tableReader already done or closed: %d", tr.consumerStatus)

does it not work if you use tr.ctx (even if its span has been closed already)? I seem to remember that we made it logging to finished spans "work" (not panic).

---

pkg/sql/distsqlrun/tablereader_test.go, line 233 at r6 (raw file):

				out = tr
			} else {
				tr.Run(context.Background(), nil)

nit: you've done a superfluous change to this line, but you missed the opportunity to comment the nil :P

---

Comments from Reviewable

[petermattis]

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/tablereader.go, line 57 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

please comment how ctx should be used instead of flowCtx.Ctx.

Done.

---

pkg/sql/distsqlrun/tablereader.go, line 201 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

this message is OK in ProcOutputHelper, but it's not great here. Just say "output not configured". Or, better yet, move the assertion to newTableReader().

Fixed the message. tablereader_test.go creates table readers with a nil output. Agreed this should be cleaned up, but we're not yet at that point.

---

pkg/sql/distsqlrun/tablereader.go, line 204 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

please add a TODO somewhere around here to remove the ctx arg. Or ideally do it now. Otherwise I don't really know what's going on here.

I'll remove it in a follow-on PR. I pinky promise.

---

pkg/sql/distsqlrun/tablereader.go, line 229 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls comment nils (some of them were commented before :) )

Done.

---

pkg/sql/distsqlrun/tablereader.go, line 312 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

VEventf() instead of if log.v(1) please. I know it was bad before too.

Done.

---

pkg/sql/distsqlrun/tablereader.go, line 315 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

not sure resetting these is a good idea (it forces you to not use tr.ctx in other places although I think you could even with the finished span).

Well, we use tr.started && tr.ctx == nil to indicate closed. I could certainly do something else here, though I'm not sure if it would be an improvement. Do you have a more specific reason why you're concerned about setting tr.{ctx,span} to nil?

---

pkg/sql/distsqlrun/tablereader.go, line 324 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls comment that err can be nil

Done.

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

ProducerMetadata is specified to have a single fields set and has the following message:
// Only one of these fields will be set. If this ever changes, note that
// there's consumers out there that extract the error and, if there is one,
// forward it in isolation and drop the rest of the record.

Hmm, let me investigate this again. I think I took a look while developing this PR and determined that the comment was out of date.

---

pkg/sql/distsqlrun/tablereader.go, line 339 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

how come the linter is not yelling at you?
Please comment that these methods are "part of the RowSource interface".

Done. The linter only cares about exported types.

---

pkg/sql/distsqlrun/tablereader.go, line 343 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

With processors implementing the RowSource iface, the comment on the Next method is no longer appropriate - it talks about rows pushed by a producer; it should be expanded.
And it should also say something about post-processing - that the processors have to do their post-processing before returning a row.

I think I already updated the comment on RowSource.Next. Did I miss something that should be adjusted?

---

pkg/sql/distsqlrun/tablereader.go, line 418 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

does it not work if you use tr.ctx (even if its span has been closed already)? I seem to remember that we made it logging to finished spans "work" (not panic).

Well, if we set tr.ctx to nil this won't work. I don't see a problem with using context.Background() here given that this is a fatal error.

---

pkg/sql/distsqlrun/tablereader_test.go, line 233 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: you've done a superfluous change to this line, but you missed the opportunity to comment the nil :P

Done.

---

Comments from Reviewable

[petermattis]

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions, some commit checks pending.

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Hmm, let me investigate this again. I think I took a look while developing this PR and determined that the comment was out of date.

I was wrong. There are several places in the code which extract the error and drop the rest of the metadata. NoMetadataRowSource.NextRow is the primary one (used by several processors) and other processors do this manually. Fixing NoMetadataRowSource is rather straightforward, but would it be better to leave the invariant in place and produce multiple bits of metadata here? That appears awkward, though perhaps I just need to stare at the code longer.

---

Comments from Reviewable

[RaduBerinde]

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions, some commit checks pending.

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I was wrong. There are several places in the code which extract the error and drop the rest of the metadata. NoMetadataRowSource.NextRow is the primary one (used by several processors) and other processors do this manually. Fixing NoMetadataRowSource is rather straightforward, but would it be better to leave the invariant in place and produce multiple bits of metadata here? That appears awkward, though perhaps I just need to stare at the code longer.

Note that the over-the-wire proto is RemoteProducerMetadata which uses oneof. So if we support multiple things per ProducerMetadata, we will likely need to adjust the stream_encoder code to split it up into multiple protos.

---

Comments from Reviewable

[andreimatei]

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions, some commit checks pending.

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

Previously, RaduBerinde wrote…

Note that the over-the-wire proto is RemoteProducerMetadata which uses oneof. So if we support multiple things per ProducerMetadata, we will likely need to adjust the stream_encoder code to split it up into multiple protos.

I am inclined to believe that "one at a time, and nothing comes after an error" is an easy to work with convention, but I don't have strong feelings.

---

Comments from Reviewable

[petermattis]

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I am inclined to believe that "one at a time, and nothing comes after an error" is an easy to work with convention, but I don't have strong feelings.

Ok, so if an error is returned we don't need to return anything else. And if no error is returned I have to return a separate ProducerMetadata for each field set? Would be convenient to send the trace and misplanned ranges data at the same time, but I think that can be handled.

---

Comments from Reviewable

[petermattis]

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ok, so if an error is returned we don't need to return anything else. And if no error is returned I have to return a separate ProducerMetadata for each field set? Would be convenient to send the trace and misplanned ranges data at the same time, but I think that can be handled.

Ok, I took a crack at this. It mostly works, though causes TestTrace to fail deterministically. I need to investigate, but won't be able to do so until later tonight or tomorrow. PTAL and let me know if this looks better.

---

Comments from Reviewable

[andreimatei]

Review status: 0 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/base.go, line 139 at r7 (raw file):

	ConsumerDone()

	// ConsumerClosed informs the source that the consumer it is done and will

s/the consumer it is done/the consumer, well, it is done

---

pkg/sql/distsqlrun/tablereader.go, line 315 at r6 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Well, we use tr.started && tr.ctx == nil to indicate closed. I could certainly do something else here, though I'm not sure if it would be an improvement. Do you have a more specific reason why you're concerned about setting tr.{ctx,span} to nil?

Well, if tr.ctx == nil means something, then please comment it.
I don't like it very much because it's weird. Dealing with nil ctxs is not something we do. And that use of context.Background, consequential or not, also bothered me. At the very least because I had to figure out why we're doing it - and I guess I misunderstood too; I thought you were concerned about a finished span.

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ok, I took a crack at this. It mostly works, though causes TestTrace to fail deterministically. I need to investigate, but won't be able to do so until later tonight or tomorrow. PTAL and let me know if this looks better.

I think I might have spoken nonsense before. A producer can push other metadata after an error, and so can a RowSource return metadata after an error. Sorry. And it's quite useful, e.g. for sending the trace after an error in this processor.
So I think the tr.close() call here should go away.
Perhaps that's also why that test is failing.

---

pkg/sql/distsqlrun/tablereader.go, line 339 at r6 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Done. The linter only cares about exported types.

ignore: I personally have moved to the "Types is part of the RowSource interface" phrasing for interfaces with more than one method. The current phrasing doesn't quite compute.

---

pkg/sql/distsqlrun/tablereader.go, line 343 at r6 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think I already updated the comment on RowSource.Next. Did I miss something that should be adjusted?

Ah, you did change something indeed, that's good.
The thing that made me say something here is that I was confused for a bit about why we're not using EmitRow() any more. EmitRow() did two things - post-processing and sending to an output. Post-processing is now done in Next(). So I was thinking that a comment somewhere should say that. At first I thought the comment should be on tableReader.Next(); then I thought maybe a nod to post-processing belongs in RowSource.Next(). I dunno. You can also ignore.

---

pkg/sql/distsqlrun/tablereader.go, line 73 at r8 (raw file):

	started    bool
	halfClosed bool

If this stays, please put some comment on this field.
Feel free to ignore, but I find the way you've structured this draining a bit confusing. I would have created a buffer of metadata (say, trailingMeta) to send, and then, if consumerStatus != NeedsMoreRows, I would have made Next() return from that buffer until it's exhausted. I'm pretty sure there's precedent for such a "pattern" in various other places in DistSQL too.

---

pkg/sql/distsqlrun/tablereader.go, line 292 at r8 (raw file):

	tr.halfClosed = true
	// This prevents Next() from returning more rows.
	tr.out.rowIdx = tr.out.maxRowIdx

Ummm seems like someone is abusing something here, and also breaking the ProcOutputHelper's encapsulation.
Can we not look at tr.halfClosed instead of tr.out.rowIdx in Next()?
Or better yet I hope halfClosed goes away and we look at whether the trailingMeta buffer that I was suggesting elsewhere is non-nil.

---

Comments from Reviewable

[petermattis]

TFTR! PTAL.

---

Review status: 0 of 9 files reviewed at latest revision, 8 unresolved discussions.

---

pkg/sql/distsqlrun/base.go, line 139 at r7 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

s/the consumer it is done/the consumer, well, it is done

Done, though I just removed the extraneous it.

---

pkg/sql/distsqlrun/tablereader.go, line 315 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Well, if tr.ctx == nil means something, then please comment it.
I don't like it very much because it's weird. Dealing with nil ctxs is not something we do. And that use of context.Background, consequential or not, also bothered me. At the very least because I had to figure out why we're doing it - and I guess I misunderstood too; I thought you were concerned about a finished span.

I've added an explicit tr.closed field and left tr.ctx alone. I'm still using context.Background() for the log fatals because that code is essentially untested (we never hit that code path in practice) and I'd prefer not to have surprises about using an invalid context.

---

pkg/sql/distsqlrun/tablereader.go, line 327 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think I might have spoken nonsense before. A producer can push other metadata after an error, and so can a RowSource return metadata after an error. Sorry. And it's quite useful, e.g. for sending the trace after an error in this processor.
So I think the tr.close() call here should go away.
Perhaps that's also why that test is failing.

I believe the test was failing because there were no misplanned ranges and thus we were sending an empty ProducerMetadata before sending the trace data.

---

pkg/sql/distsqlrun/tablereader.go, line 339 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

ignore: I personally have moved to the "Types is part of the RowSource interface" phrasing for interfaces with more than one method. The current phrasing doesn't quite compute.

Done.

---

pkg/sql/distsqlrun/tablereader.go, line 343 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Ah, you did change something indeed, that's good.
The thing that made me say something here is that I was confused for a bit about why we're not using EmitRow() any more. EmitRow() did two things - post-processing and sending to an output. Post-processing is now done in Next(). So I was thinking that a comment somewhere should say that. At first I thought the comment should be on tableReader.Next(); then I thought maybe a nod to post-processing belongs in RowSource.Next(). I dunno. You can also ignore.

I'm not sure if anything needs to be commented here. Clearly post-processing needs to be done. Failure to do so will break tests dramatically.

---

pkg/sql/distsqlrun/tablereader.go, line 73 at r8 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

If this stays, please put some comment on this field.
Feel free to ignore, but I find the way you've structured this draining a bit confusing. I would have created a buffer of metadata (say, trailingMeta) to send, and then, if consumerStatus != NeedsMoreRows, I would have made Next() return from that buffer until it's exhausted. I'm pretty sure there's precedent for such a "pattern" in various other places in DistSQL too.

Done. halfClosed has been replaced with trailingMetadata. Note that I didn't do this for distinct and valuesProcessor because their handling of trailing metadata is simpler.

---

pkg/sql/distsqlrun/tablereader.go, line 292 at r8 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Ummm seems like someone is abusing something here, and also breaking the ProcOutputHelper's encapsulation.
Can we not look at tr.halfClosed instead of tr.out.rowIdx in Next()?
Or better yet I hope halfClosed goes away and we look at whether the trailingMeta buffer that I was suggesting elsewhere is non-nil.

Yes, there is a bit of encapsulation breakage here, though I'm hoping it is temporary. Once all processors implement RowSource, we can start trimming stuff from ProcOutputHelper and the encapsulation breakage can be fixed. I made this mildly cleaner by adding a method to ProcOutputHelper.

---

Comments from Reviewable

[andreimatei]

---

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/base.go, line 139 at r7 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Done, though I just removed the extraneous it.

I kid.

---

pkg/sql/distsqlrun/tablereader.go, line 315 at r6 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I've added an explicit tr.closed field and left tr.ctx alone. I'm still using context.Background() for the log fatals because that code is essentially untested (we never hit that code path in practice) and I'd prefer not to have surprises about using an invalid context.

Thanks. SGTM about about Background(); I now think it might not work with a closed span if we have lightstep enabled.

---

pkg/sql/distsqlrun/tablereader.go, line 73 at r8 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Done. halfClosed has been replaced with trailingMetadata. Note that I didn't do this for distinct and valuesProcessor because their handling of trailing metadata is simpler.

danke

---

Comments from Reviewable

[petermattis]

@arjunravinarayan, @asubiotto I'm going to merge this now. Feel free to leave additional comments which I'll address in follow-on PRs. I don't think this fundamentally affects row batching, but if it does I'm happy to deal with the fallout.

[rjnn]

Reviewed 1 of 26 files at r4, 4 of 4 files at r9, 3 of 3 files at r10, 2 of 2 files at r11, 2 of 2 files at r12.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

Comments from Reviewable