distsql: add NewFinalIterator to the rowIterator interface

[asubiotto]

Some implementations of the rowIterator interface would destroy rows as
they were iterated over to free memory eagerly. NewFinalIterator is
introduced in this change to provide non-reusable behavior and
NewIterator is explicitly described as reusable.

A reusable iterator has been added to the memRowContainer to satisfy
these new interface semantics.

Release note: None

[cockroach-teamcity]

This change is 

[rjnn]

Good interface design!

Might be worth adding a boolean to the implementors that track whether a final iterator was created, which errors out on any further iteration creations, just to eagerly ensure callers of final iterator are not messing up. this proactive checking would avoid a problem where we actually make destructive final iterator implementations in the future, and only then discover the bugs lurking underneath.

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[asubiotto]

@arjunravinarayan I'm not sure whether we should do that because it is technically valid to call NewFinalIterator a second time, you just won't read any rows unless you added to the rowContainer. I think it won't be that likely to run into any surprise issues because NewIterator would probably be the default that most people use (just like {Unsafe}Key in RocksDB).

bors r+

[jordanlewis]

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

pkg/sql/distsqlrun/row_container.go, line 38 at r1 (raw file):

	// NewFinalIterator returns a rowIterator that can be used to iterate over the
	// rows. The use of this method indicates to the implementer that the row will
	// never be read again. Consult implementations for exact behavior.

I find this phrasing a little confusing - I would probably say something like "NewFinalIterator returns a rowIterator that can be used to iterate over the rows, possibly freeing resources along the way. Once you call NewFinalIterator, subsequent calls to NewIterator or NewFinalIterator are not guaranteed to return any rows."

---

Comments from Reviewable

[jordanlewis]

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from Reviewable

[asubiotto]

bors r-

[craig]

Canceled

[asubiotto]

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/distsqlrun/row_container.go, line 38 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I find this phrasing a little confusing - I would probably say something like "NewFinalIterator returns a rowIterator that can be used to iterate over the rows, possibly freeing resources along the way. Once you call NewFinalIterator, subsequent calls to NewIterator or NewFinalIterator are not guaranteed to return any rows."

Done.

---

Comments from Reviewable

[asubiotto]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)