release-23.2: sql: don't parallelize *any* FK and unique checks containing locking

[michae2]

Backport 3/3 commits from #118722.

/cc @cockroachdb/release

---

sql, execbuilder: move plan flags from planTop to planComponents

Execbuild produces a sql.planComponents containing the main query
plan, plus plans for any subqueries, checks, and cascades. In the normal
SQL code path (sql.(*optPlanningCtx).runExecBuilder) we copy this
planComponents into a sql.planTop, which then is decorated with extra
information gathered from the Builder.

However, there are other users of execbuild that only work with the
planComponents, and don't have a planTop. In the next commit, one such
user, PlanAndRunCascadesAndChecks, needs to see some of the plan flags
set when building FK check plans, but doesn't have access to the Builder
which gathered those plans.

To solve this, this commit moves plan flags from the planTop to
planComponents, and then refactors the execbuilder and exec factory to
set some of these flags when creating the planComponents.

Informs: #118720

Epic: None

Release note: None

---

sql: don't parallelize any FK and unique checks containing locking

In #111896 we disallowed parallel execution of FK and unique checks that
contain locking, to avoid usage of LeafTxns. But #111896 only considered
checks built during planning of the main query, and overlooked checks
built during planning of FK cascades.

This commit also considers checks built during planning of FK cascades.

Fixes: #118720

Epic: None

Release note (bug fix): This commit fixes an internal error with message
like: "LeafTxn ... incompatible with locking request" that occurs when
performing an update under read committed isolation which cascades to a
table with multiple other foreign keys.

---

sql: rename *ContainsNonDefaultKeyLocking to *ContainsLocking

"Default" locking here simply means no locking. I find this convention
of saying "non-default key locking" a little verbose and confusing.

Epic: None

Release note: None

---

Release justification: fixes a bug that prevents some update statements from being executed under read committed isolation.

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Backports should only be created for serious
  issues or test-only changes.
• Backports should not break backwards-compatibility.
• Backports should change as little code as possible.
• Backports should not change on-disk formats or node communication protocols.
• Backports should not add new functionality (except as defined
  here).
• Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
• All backports must be reviewed by the owning areas TL and one additional
  TL. For more information as to how that review should be conducted, please consult the backport
  policy.

If your backport adds new functionality, please ensure that the following additional criteria are satisfied:

• There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
• The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
• New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
• Your backport must be accompanied by a post to the appropriate Slack
  channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

[cockroach-teamcity]

This change is 

[michae2]

Are all of the changes protected via a flag or option, new syntax, an environment variable or default-disabled cluster setting?

No:

The first commit is a small refactor of planTop, planComponents, and execbuilder that is not behind any protection. This is the riskiest part of the change. Unfortunately this part is necessary to make the fix possible.

The second commit is behind the sql.distsql.parallelize_checks.enabled cluster setting, but this setting is default-enabled in 23.2, so not much protection.

The third commit is a rename of some constants.

What are the risks to backporting this change? Can the risks of backporting be mitigated?

There is a small risk that the first commit (the refactor) breaks something that depends on the flags in planTop. These flags are used by distsql physical planning, metrics collection, and various checks.

What are the risks to not backporting?

Due to LeafTxn ... incompatible with locking request errors, it would be impossible to execute some UPDATE statements on some tables under read committed isolation with the sql.distsql.parallelize_checks.enabled cluster setting enabled (default in 23.2).

Does this change really need to be backported? Or can it reasonably wait until the next major release to be addressed?

If we don't backport, there are two workarounds to allow affected UPDATE statements to run:

• disabling cluster setting sql.distsql.parallelize_checks.enabled, but some customers rely on this parallelization for adequate query performance
• running the affected UPDATE statements under serializable isolation, but this might not be possible for some workloads

I believe it is worth the small risk of backporting the refactor in the first commit to make sure customers have a good experience with read committed isolation in 23.2.

Is this the smallest possible change that can be made?

The third commit (rename of some constants) could be skipped. I've included it to reduce future backport skew, but it doesn't have to be included. Curious to know what people think.

[mgartner]

I like the third commit and I think it's fine to backport. I left a few optional comments that you can consider in the future - no need to change anything in this PR.

Reviewed 15 of 15 files at r1, 2 of 2 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2, @nvanbenschoten, and @yuzefovich)

---

pkg/sql/distsql_running.go line 2103 at r2 (raw file):

			return false
		}
		cp := cascadePlan.(*planComponents)

🤔 this comment could probably use an update:

cockroach/pkg/sql/opt/exec/factory.go

Line 34 in 7ad0187

// Plan represents the plan for a query (currently maps to sql.planTop).

---

pkg/sql/opt/exec/factory.go line 41 at r1 (raw file):

// PlanFlags tracks various properties of the built plan.
type PlanFlags uint32

nit: Maybe a better name is PlanProperties or PlanProps? It won't match the sql.planFlags naming, though.

---

pkg/sql/opt/exec/factory.go line 80 at r1 (raw file):

func (pf PlanFlags) IsSet(flag PlanFlags) bool {
	return (pf & flag) != 0

super nit: I don't think we're terribly consistent with this, but I think it's safer to do return pf&flag == flag. That way, if flag has multiple bits set, IsSet will only return true if pf has all of those bits set rather than returning true if it has one bit set.

[michae2]

The failures look like known flakes #116702 and #112550. I'll try rebasing.

[michae2]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @nvanbenschoten, and @yuzefovich)

---

pkg/sql/opt/exec/factory.go line 80 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

super nit: I don't think we're terribly consistent with this, but I think it's safer to do return pf&flag == flag. That way, if flag has multiple bits set, IsSet will only return true if pf has all of those bits set rather than returning true if it has one bit set.

Good call! This inspired me to perform an audit of all such helper functions: #119035