jobs: Incorrect re-entrant use of isql.Executor

[miretskiy]

Exposed by flake https://teamcity.cockroachdb.com/repository/download/Cockroach_Nightlies_StressBazel/9121862:id/tmp/_tmp/905465f697dc04c39f7d8a82cb057bfe/logTestChangefeedJobControl3004067371/changefeedccltest.log

The recent changes to the jobs system #93643 inadvertently introduced an incorrect usage of internal executor query.
In particular, it is incorrect to use internalEx.QueryIterator to populate crdb_internal.jobs table, and, while using
the same transaction, to issue re-entrant call to internal executor (via querying crdb_internal.system_jobs table).

This problem is also applicable to the code that uses higher level APIs, for example, to perform authorization checks,
and those APIs in turn may issue incorrect calls to the internal executor.

This issue is specific to the changes introduced by the above PR, and is not going to address potential issues with other uses
of internal executor.
In particular, the tentative plan, is to make functions responsible for populating virtual tables crdb_internal.jobs and crdb_internal.system_jobs use their own transactions.

Jira issue: CRDB-25732

[blathers-crl]

Hi @miretskiy, please add branch-* labels to identify which branch(es) this release-blocker affects.

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

[miretskiy]

@adityamaru -- should this be moved to @yuzefovich ? Not sure this is jobs related anymore.

[adityamaru]

I think it should and maybe it can be closed by virtue of #99325? I'll let Yahor close it out.

[yuzefovich]

I don't think it's incorrect to have queries issued by the internal executor to result in other IE-issued queries, i.e. such "nested" queries should be allowed. What was incorrect is that we need to be careful to either:

• use RootTxns in all queries and avoid any concurrency
• use LeafTxns in all queries.

Currently, we use the former option, and #99325 fixed one case where that approach was violated (the "outer" query used the LeafTxn while the "inner" query used the RootTxn).

One scenario that I'm still concerned about is the usage of DistSQL in the "inner" queries. If the "outer" query uses the RootTxn, then issues the "inner" query via QueryIterator API (i.e. the query doesn't complete before returning the control back to the "outer" query), and that "inner" query chooses to use DistSQL, then we might be in violation too. However, this is an old pre-existing scenario (unlike #99325 which was introduced in 22.2).

At the same time @ajwerner mentioned that as of #93218 we might be using the "outer" query's session defaults more. Andrew, do you think it's more likely that as of #93218 that the internal queries will use DistSQL more? Are those cases only where we use isql.WithSessionData option?

[yuzefovich]

With Raphael explicitly disabling DistSQL for jobs system in #100579, perhaps this part

One scenario that I'm still concerned about is the usage of DistSQL in the "inner" queries. If the "outer" query uses the RootTxn, then issues the "inner" query via QueryIterator API (i.e. the query doesn't complete before returning the control back to the "outer" query), and that "inner" query chooses to use DistSQL, then we might be in violation too. However, this is an old pre-existing scenario (unlike #99325 which was introduced in 22.2).

is less of a concern now. Closing.