Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Feb 6, 2024

Conversation

michae2
Copy link
Collaborator

@michae2 michae2 commented Feb 3, 2024

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

@michae2 michae2 requested a review from a team as a code owner February 3, 2024 07:02
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@michae2 michae2 added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Feb 3, 2024
@michae2
Copy link
Collaborator Author

michae2 commented Feb 3, 2024

My plan is to backport the first two commits after this has baked on master for a few weeks.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Nice find!

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

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: cockroachdb#118720

Epic: None

Release note: None
In cockroachdb#111896 we disallowed parallel execution of FK and unique checks that
contain locking, to avoid usage of LeafTxns. But cockroachdb#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: cockroachdb#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.
"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
@michae2
Copy link
Collaborator Author

michae2 commented Feb 6, 2024

Thanks!

bors r=yuzefovich

@craig
Copy link
Contributor

craig bot commented Feb 6, 2024

Build succeeded:

@craig craig bot merged commit 9a3b3c9 into cockroachdb:master Feb 6, 2024
8 of 9 checks passed
Copy link

blathers-crl bot commented Feb 6, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 5021627 to blathers/backport-release-23.2-118722: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
3 participants