database/sql: add cursor cancelation test, document some cursor issues

A row can contain a "cursor", which is essentially a nested Rows.

Add a test for calling Rows.Close at various points of iterating over
nested Rows.

The implementation of cursors has some infelicities.
Add some comments documenting these.
Don't try to fix them at this time.

For #79407

Change-Id: I0010f89ccca352ca4eb54056f57cf5656a6a6964
Reviewed-on: https://go-review.googlesource.com/c/go/+/777960
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

src/database/sql/convert.go

@@ -361,12 +361,29 @@ func convertAssignRows(dest, src any, rows *Rows) error {
 			if rows == nil {
 				return errors.New("invalid context to convert cursor rows, missing parent *Rows")
 			}
+			// This is hazardous and not really correct: If the user provides us
+			// with the same *Rows for each Scan (which they very likely will),
+			// then this overwrites the previously-used Rows, including its mutexes.
+			// The chained row cancel function below will also repeatedly reference
+			// the same *Rows.
 			*d = Rows{
 				dc:          rows.dc,
 				releaseConn: func(error) {},
 				rowsi:       s,
 			}
 			// Chain the cancel function.
+			//
+			// This has problems:
+			//   - Repeatedly wrapping the cancel func is inefficient compared to
+			//     just storing a []*Rows of children.
+			//   - The cancel func is wrapped for each cursor read. If we scan N
+			//     rows, each with a child cursor, we end up with N chained cancel
+			//     funcs. (Also, if the user is reusing a Rows--see above--the cancel
+			//     funcs might all be referencing the same underlying Rows cursor.)
+			//   - It seems like it would be reasonable to invalidate a cursor
+			//     after advancing to the next parent row (the row which contains
+			//     the cursor). We don't do that now, and it isn't clear that we can
+			//     change this.
 			parentCancel := rows.cancel
 			rows.cancel = func() {
 				// When Rows.cancel is called, the closemu will be locked as well.

src/database/sql/sql_test.go

@@ -1671,6 +1671,100 @@ func testCursorFake(t *testing.T, db *DB) {
 	}
 }
 
+// TestCursorCancel exercises calling Rows.Close at various places,
+// including canceling a cursor (child Rows).
+func TestCursorCancel(t *testing.T) {
+	for _, test := range []struct {
+		name     string
+		cancelOn string
+		want     []string
+	}{{
+		// don't cancel
+		name: "no cancel",
+		want: []string{
+			"table1",
+			"1.1",
+			"1.2",
+			"table2",
+			"2.1",
+			"2.2",
+		},
+	}, {
+		name:     "outer cancel",
+		cancelOn: "table2",
+		want: []string{
+			"table1",
+			"1.1",
+			"1.2",
+			"table2",
+		},
+	}, {
+		name:     "inner cancel",
+		cancelOn: "1.1",
+		want: []string{
+			"table1",
+			"1.1",
+			"table2",
+			"2.1",
+			"2.2",
+		},
+	}} {
+		t.Run(test.name, func(t *testing.T) {
+			testDatabase(t, func(t *testing.T, db *DB) {
+				testCursorCancel(t, db, test.cancelOn, test.want)
+			})
+		})
+	}
+}
+func testCursorCancel(t *testing.T, db *DB, cancelOn string, want []string) {
+	exec(t, db, "CREATE|table1|col=string")
+	exec(t, db, "INSERT|table1|col=1.1")
+	exec(t, db, "INSERT|table1|col=1.2")
+	exec(t, db, "CREATE|table2|col=string")
+	exec(t, db, "INSERT|table2|col=2.1")
+	exec(t, db, "INSERT|table2|col=2.2")
+
+	exec(t, db, "CREATE|cursor|name=string,list=table")
+	exec(t, db, "INSERT|cursor|name=table1,list=table1!col")
+	exec(t, db, "INSERT|cursor|name=table2,list=table2!col")
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*30)
+	defer cancel()
+
+	rows, err := db.QueryContext(ctx, `SELECT|cursor|name,list|`)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rows.Close()
+
+	var got []string
+	for rows.Next() {
+		var name string
+		cursor := &Rows{}
+		if err := rows.Scan(&name, cursor); err != nil {
+			t.Fatal(err)
+		}
+		got = append(got, name)
+		if name == cancelOn {
+			rows.Close()
+		}
+		for cursor.Next() {
+			var col string
+			if err := cursor.Scan(&col); err != nil {
+				t.Fatal(err)
+			}
+			got = append(got, col)
+			if col == cancelOn {
+				cursor.Close()
+			}
+		}
+	}
+
+	if !slices.Equal(got, want) {
+		t.Errorf("cancel after reading %q:\ngot:  %v\nwant: %v", cancelOn, got, want)
+	}
+}
+
 func TestInvalidNilValues(t *testing.T) {
 	var date1 time.Time
 	var date2 int