sql: elide recursion in the row iterator

Previously, the row iterator would self-recurse to process partial
results and would sometimes blow up the stack as a result. Attempts were
made to limit the amount of recursion, but would still sometimes
explode the stack. In this patch, we rewrite the function using a retry
loop. Along the way, we simplify the logic to make the function more
readable.

Fixes: #129963
Release note: None

pkg/sql/internal.go

@@ -537,94 +537,58 @@ type rowsIterator struct {
 var _ isql.Rows = &rowsIterator{}
 var _ eval.InternalRows = &rowsIterator{}
 
-// iteratorDepthLimit is maximum allowed depth of recursion in Next(). It is set
-// to be sufficiently large to not matter under normal circumstances while
-// preventing the possibility of the stack overflow (as we've seen in #109197).
-const iteratorDepthLimit = 1000
-
-var iteratorDepthLimitExceededErr = errors.New("rowsIterator exceeded recursion depth limit")
-
-func (r *rowsIterator) Next(ctx context.Context) (_ bool, retErr error) {
-	// Due to recursive calls to Next() below, this deferred function might get
-	// executed multiple times, yet it is not a problem because Close() is
-	// idempotent and we're unsetting the error callback.
-	defer func() {
-		// If the iterator has just reached its terminal state, we'll close it
-		// automatically.
-		if r.done {
-			// We can ignore the returned error because Close() will update
-			// r.lastErr if necessary.
-			_ /* err */ = r.Close()
-		}
-		if r.errCallback != nil {
-			r.lastErr = r.errCallback(r.lastErr)
-			r.errCallback = nil
+func (r *rowsIterator) Next(ctx context.Context) (bool, error) {
+	for !r.done && r.lastErr == nil {
+		var data ieIteratorResult
+		if r.first != nil {
+			// This is the very first call to Next() and we have already buffered
+			// up the first piece of data before returning rowsIterator to the
+			// caller.
+			data = *r.first
+			r.first = nil
+		} else {
+			nextItem, done, err := r.r.nextResult(ctx)
+			if err != nil || done {
+				r.lastErr = err
+				break
+			}
+			data = nextItem
 		}
-		retErr = r.lastErr
-		r.depth--
-	}()
-
-	r.depth++
-	if r.depth > iteratorDepthLimit {
-		r.lastErr = iteratorDepthLimitExceededErr
-		r.done = true
-		return false, r.lastErr
-	}
-
-	if r.done {
-		return false, r.lastErr
-	}
 
-	// handleDataObject processes a single object read from ieResultReader and
-	// returns the result to be returned by Next. It also might call Next
-	// recursively if the object is a piece of metadata.
-	handleDataObject := func(data ieIteratorResult) (bool, error) {
 		if data.row != nil {
 			r.rowsAffected++
-			// No need to make a copy because streamingCommandResult does that
-			// for us.
 			r.lastRow = data.row
 			return true, nil
 		}
+
 		if data.rowsAffected != nil {
 			r.rowsAffected = *data.rowsAffected
-			return r.Next(ctx)
-		}
-		if data.cols != nil {
-			if r.mode == rowsAffectedIEExecutionMode {
-				// In "rows affected" execution mode we simply ignore the column
-				// schema since we always return the number of rows affected
-				// (i.e. a single integer column).
-				return r.Next(ctx)
-			}
-			// At this point we don't expect to see the columns - we should only
-			// return the rowsIterator to the caller of execInternal after the
-			// columns have been determined.
-			data.err = errors.AssertionFailedf("unexpectedly received non-nil cols in Next: %v", data)
+			continue
 		}
-		if data.err == nil {
-			data.err = errors.AssertionFailedf("unexpectedly empty ieIteratorResult object")
+
+		// In "rows affected" execution mode we simply ignore the column
+		// schema since we always return the number of rows affected
+		// (i.e. a single integer column).
+		if r.mode == rowsAffectedIEExecutionMode && data.cols != nil {
+			continue
 		}
-		r.lastErr = data.err
-		r.done = true
-		return false, r.lastErr
-	}
 
-	if r.first != nil {
-		// This is the very first call to Next() and we have already buffered
-		// up the first piece of data before returning rowsIterator to the
-		// caller.
-		first := r.first
-		r.first = nil
-		return handleDataObject(*first)
+		if data.cols != nil {
+			r.lastErr = errors.AssertionFailedf("unexpectedly receieved non-nil cols in Next: %v", data)
+		} else if data.err == nil {
+			r.lastErr = errors.AssertionFailedf("unexpectedly empty ieIteratorResult object")
+		} else {
+			r.lastErr = data.err
+		}
 	}
 
-	var next ieIteratorResult
-	next, r.done, r.lastErr = r.r.nextResult(ctx)
-	if r.done || r.lastErr != nil {
-		return false, r.lastErr
+	r.done = true
+	_ = r.Close()
+	if r.errCallback != nil {
+		r.lastErr = r.errCallback(r.lastErr)
+		r.errCallback = nil
 	}
-	return handleDataObject(next)
+	return false, r.lastErr
 }
 
 func (r *rowsIterator) Cur() tree.Datums {