sql/rowexec: check MustBeStreaming on instantiated generators

[stevendanna]

In projectSetProcessor, the gen array is not populated until
nextInputRow() is called in Next. But, we expected that these
generators had been populated in MustBeStreaming which is called
before Next() is called.

As a result, rows returned streaming value generators were still
buffered.

I believe this is the main cause of #85630 and other odd behaviour
we've seen in the tenant replication tests.

Release note: None

[cockroach-teamcity]

This change is 

[stevendanna]

It's a bummer to do this here, because I think that constructing this generator actually does work in some cases.

[stevendanna]

Unfortunately, this Close() might assume that Start() has run. Calling Start() here would be an even bigger bummer.

So, I think this isn't how we want to initialize these.

[yuzefovich]

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna and @yuzefovich)

---

pkg/sql/rowexec/project_set.go line 132 at r1 (raw file):

Previously, stevendanna (Steven Danna) wrote…

It's a bummer to do this here, because I think that constructing this generator actually does work in some cases.

Instead of doing this, let's rework how we are propagating the fact whether a value generator is streaming. We can introduce new property to tree.FunctionProperties indicating whether the func expr must be streaming, and then streamPartition would mark it as true. We can then examine the FuncExpr's properties in newProjectSetProcessor.

---

pkg/sql/rowexec/project_set.go line 136 at r1 (raw file):

Previously, stevendanna (Steven Danna) wrote…

Unfortunately, this Close() might assume that Start() has run. Calling Start() here would be an even bigger bummer.

So, I think this isn't how we want to initialize these.

This call to Close doesn't seem necessary - at least the contract says that Close doesn't need to be called if Start isn't called.

[yuzefovich]

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

---

pkg/sql/rowexec/project_set.go line 132 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Instead of doing this, let's rework how we are propagating the fact whether a value generator is streaming. We can introduce new property to tree.FunctionProperties indicating whether the func expr must be streaming, and then streamPartition would mark it as true. We can then examine the FuncExpr's properties in newProjectSetProcessor.

Typed up that idea in #85866 since I had some free cycles.

[stevendanna]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/rowexec/project_set.go line 132 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Typed up that idea in #85866 since I had some free cycles.

Nice, thanks! I'll close this PR out.

---

pkg/sql/rowexec/project_set.go line 136 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This call to Close doesn't seem necessary - at least the contract says that Close doesn't need to be called if Start isn't called.

Yeah, this close was added after some test failures led me to https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sem/builtins/generator_builtins.go#L2095-L2104. The iterator creator there would end up creating logic test failures related to unfinished trace spans without the close. But, I think the correct fix for that would actually be to fix up that generator to do that work in start rather than in the constructor.