sql: fix cleanup of not exhausted pausable portals in some cases

[yuzefovich]

execinfra: relax RowSource.ConsumerClosed contract

This commit adjusts RowSource.ConsumerClosed contract so that it's
possible for this method to be called multiple times. Previously, vast
majority of implementations already supported that (and some actually
assumed that), but there were a couple that would result in a panic or
an error if called multiple times - those have been relaxed.

This is needed for the follow-up commit which will introduce an
unconditional call of this method on the "head" processor of the flow
(needed for pausable portals execution model).

Release note: None

sql: fix cleanup of not exhausted pausable portals in some cases

Previously, if we executed a flow (either vectorized or row-based) that
contains a row-by-row processor in "pausable portal" model and didn't
fully exhaust that flow (meaning that it could still produce more rows),
this could result in an error when closing the portal because that
processor might not be closed properly. Vectorized operators aren't
affected by this, but row-by-row processors effectively require
ConsumerClosed method to be called on them, and this might not happen.
In particular, it was assumed that this method would get called by
execinfra.Run loop, but for pausable portal model we needed to adjust
it so that the loop could be exited (and then re-entered) when switching
to another portal; thus, we lost the guarantee that ConsumerClosed is
always called.

This commit fixes this problem by teaching both vectorized and row-based
flows to always call ConsumerClosed on their "head" processor on
Cleanup (which is called when closing the portal). Now, Cleanup
calls ConsumerClosed unconditionally (when the flow might be running
in the pausable portal mode), and this is safe given the interface
adjustment in the previous commit.

Fixes: #110486

Release note (bug fix): Previously, when evaluating some queries (most
likely with a lookup join) via "multiple active portals" execution
mode (preview feature that can be enabled via
multiple_active_portals_enabled session variable) CockroachDB could
encounter an internal error like unexpected 40960 leftover bytes if
that portal wasn't fully consumed. This is now fixed.

[cockroach-teamcity]

This change is 

[ZhouXing19]

LGTM, thanks for the detailed explanation in the commit message! Just left a nit comment

Reviewed 5 of 5 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2, @rafiss, and @yuzefovich)

---

pkg/ccl/backupccl/split_and_scatter_processor.go line 198 at r1 (raw file):

	scatterer splitAndScatterer
	// cancelScatterAndWaitForWorker cancels the scatter goroutine and waits for
	// it to finish. It can be called multiples times.

Curious why we add that it can be called multiple times now -- is this something new, or do we just want to emphasize it? For I can't see changes about cancelScatterAndWaitForWorker.

[yuzefovich]

TFTR!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2, @rafiss, and @ZhouXing19)

---

pkg/ccl/backupccl/split_and_scatter_processor.go line 198 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Curious why we add that it can be called multiple times now -- is this something new, or do we just want to emphasize it? For I can't see changes about cancelScatterAndWaitForWorker.

It's not new - it's simply highlighting the existing implementation detail which makes it easier to see that splitAndScatterProcessor.ConsumerClosed is such that it can be safely called multiple times. (We already had the similar comment on backupDataProcessor.cancelAndWaitForWorker, so I added this callout in two other places.) While working on the first commit I audited all implementations of ConsumerClosed, and in most we short-circuit if .Closed is true or only do work if InternalClose returns true, but in a few places we don't do that, so I added a bit more commentary.

[craig]

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[yuzefovich]

CLA bot hiccup.

bors r+

[craig]

Stopped waiting for PR status (GitHub check) without running due to duplicate requests to run. You may check Bors to see that this PR is included in a batch by one of the other requests.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error setting reviewers, but backport branch blathers/backport-release-23.1-110625 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/110666/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch 23.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}