sql: internal executor iterator pattern makes it easy to create invalid uses of txn

[knz]

This issue describes the more general problem around #99171.

The following code pattern is both very easy to introduce/use (because it's convenient) and also extremely invalid because it violates the Txn API contract:

it, _ := internalExecutor.QueryIteratorEx(... txn1 ...)
defer it.Close()

for hasNext, err = it.Next(ctx); hasNext; hasNext, err = it.Next(ctx) {
    // Here the iterator is active, the plan above is running
    ...
    /* EITHER: */
    result := internalExecutor.Exec( ... txn1 /* same txn as above */ ... )
    /* OR:  */
    /* do something else with the txn object that issues KV requests */
    /* THIS INCLUDES INDIRECT CALLS e.g. via APIs for user authorization / privilege checks */
    ...
}

The following is also problematic because the query plan may have started to execute before the first Next call:

it, _ := internalExecutor.QueryIteratorEx(... txn1 ...)
defer it.Close()

/* EITHER: */
result := internalExecutor.Exec( ... txn1 /* same txn as above */ ... )
/* OR:  */
/* do something else with the txn object that issues KV requests */
/* THIS INCLUDES INDIRECT CALLS e.g. via APIs for user authorization / privilege checks */

for hasNext, err = it.Next(ctx); hasNext; hasNext, err = it.Next(ctx) {
   // ... 
}

The issue is that while the iterator is active, the kv.Txn may have some LeafTxn objects active and issuing KV requests. During that time, it is not valid to concurrently use the RootTxn object associated with the kv.Txn -- in particular not via another concurrent use of the internal executor, but also not via other KV interfaces.

This problem already existed in the past but through the v23.1 cycle we have introduced more uses of the iterator pattern, so we have become more sensitive to it.

Jira issue: CRDB-25758

[knz]

@stevendanna has an insight:

The error is easy to miss in tests. Some auth calls go to the database. Others read from caches that are often [already] populated.

[knz]

xref #42766

[michae2]

See also #41992

[michae2]

The following code pattern is both very easy to introduce/use (because it's convenient) and also extremely invalid because it violates the Txn API contract:

Is this pattern still invalid if only the RootTxn is used?

[knz]

No then it's not invalid, but beware that a single RootTxn cannot be used from concurrent goroutines either.