release-19.2: backport fixes for schema change deadlocks #46904
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
|
@andreimatei is there an additional commit that needs to be backported to deal with restarts and high priority transactions? |
This patch makes reading the system.namespace table and reading table descriptors in high-priority transactions. The transactions will push locks owned by schema changes out of their way. The idea is that regular SQL transactions reading schema don't want to wait for the transactions performing DDL statements. Instead, those DDLs will be pushed and forced to refresh. Besides the benefit to regular transactions, this patch also prevents deadlocks for the transactions performing DDL. Before this patch, the final select in the following sequence would deadlock: begin; savepoint s; create table t(x int); rollback to savepoint s; select * from t; The select is reading the namespace table, using a different txn from its own. That read would block on the intent laid down by the prior create. With this patch, the transaction effectively pushes itself, but gets to otherwise run. Fixes cockroachdb#24885 Release justification: Fix for an old bug that became more preminent when we introduced SAVEPOINTs recently. Release note: A rare bug causing transactions that have performed schema changes to deadlock after they restart has been fixed.
This commit is a follow up to cockroachdb#46170. That PR dealt with the table resolution self-deadlock and additionally ensures that lease acquisition is not blocked. That commit did not deal with the fact that schema changes which mutate databases can deadlock themselves on retry because a separate transaction is used to resolve database descriptors during planning. This patch makes reading the system.namespace table and reading database descriptors in high-priority transactions. The transactions will push locks owned by schema changes out of their way. The idea is that regular SQL transactions reading schema don't want to wait for the transactions performing DDL statements. Instead, those DDLs will be pushed and forced to refresh. Besides the benefit to regular transactions, this patch also prevents deadlocks for the transactions performing DDL. Before this patch, the final select in the following sequence would deadlock: begin; savepoint s; create database d; rollback to savepoint s; select * from d.t; The select is reading the namespace table, using a different txn from its own. That read would block on the intent laid down by the prior create. With this patch, the transaction effectively pushes itself, but gets to otherwise run. Fixes cockroachdb#46224 Release justification: Fix for an old bug that became more preminent when we introduced SAVEPOINTs recently. Release note (bug fix): A rare bug causing transactions that have performed schema changes to deadlock after they restart has been fixed.
Prepartion of certain queries requires performing reads against the database. If the user has already laid down intents, these reads may become part of a dependency cycle. Prior to this commit, these reads would be on a different transaction and thus this cycle would not be detected by our deadlock detection mechanism. This change opts to use the user's transaction for planning if there is one and thus will properly interact with deadlock detection. Fixes cockroachdb#46447. Release justification: fixes for high-priority or high-severity bugs in existing functionality Release note: None
In cockroachdb#46588 a bug was introduced when a retriable error was encountered while using a new transaction for preparing. Prior to that commit, all error were treated as not retriable. This was sort of a bummer. Retriable errors can occur due to read within uncertainty. Before this PR, those retriable errors would make their way to the client. Now we'll handle those retry errors internally underneath `connExecutor.prepare` Fixes cockroachdb#43251 Release note: None
ajwerner
force-pushed
the
backport19.2-46170-46384-46588-46829
branch
from
3525108 to
75b2070
Compare
|
nb: the 19.2.6 branch would like to be cut today #46946, so if there was a way to merge this soon that would be swell. |
Don't think so. LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 1/1 commits from #46170.
Backport 1/1 commits from #46384.
Backport 1/1 commits from #46588.
Backport 1/1 commits from #46829.
/cc @cockroachdb/release
Backports commits to deal with deadlocks due to being unable to detect deadlocks during schema element lookup.