sql: resolve schema in high-priority transactions #46170
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 |
ajwerner
approved these changes
Mar 17, 2020
There was a problem hiding this comment.
Thanks for picking this up!
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
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.
andreimatei
force-pushed
the
sql.high-priority-leasing
branch
from
b9a63f2 to
8654b02
Compare
andreimatei
commented
Mar 17, 2020
There was a problem hiding this comment.
bors r+
Reviewable status:
Build succeeded |
This was referenced Mar 20, 2020
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Mar 23, 2020
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.
craig bot
pushed a commit
that referenced
this pull request
Mar 23, 2020
46384: sql: resolve databases in high-priority transactions r=andreimatei a=ajwerner This commit is a follow up to #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 #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. Co-authored-by: Andrew Werner <[email protected]>
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Apr 2, 2020
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.
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Apr 20, 2020
Prior to this change, the StatsRefresher was being notified of a new table during the execution of the createTable node, before the creating transaction had committed. Prior to cockroachdb#46170, the StatsRefresher was likely to block on the intent of the creating transaction. After that change, the StatsRefresher might not discover the new table because the creating transaction is much more likely to get pushed further into the future, past the read of the stats refresher. Release note (bug fix): Fix rare bug where stats were not automatically generated for a new table.
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Apr 24, 2020
Prior to this change, the StatsRefresher was being notified of a new table during the execution of the createTable node, before the creating transaction had committed. Prior to cockroachdb#46170, the StatsRefresher was likely to block on the intent of the creating transaction. After that change, the StatsRefresher might not discover the new table because the creating transaction is much more likely to get pushed further into the future, past the read of the stats refresher. Release note (bug fix): Fix rare bug where stats were not automatically generated for a new table.
craig bot
pushed a commit
that referenced
this pull request
Apr 24, 2020
47718: sql: move notifying StatsRefresh of new table to post-commit hook r=andreimatei a=ajwerner Prior to this change, the StatsRefresher was being notified of a new table during the execution of the createTable node, before the creating transaction had committed. Prior to #46170, the StatsRefresher was likely to block on the intent of the creating transaction. After that change, the StatsRefresher might not discover the new table because the creating transaction is much more likely to get pushed further into the future, past the read of the stats refresher. Release note (bug fix): Fix rare bug where stats were not automatically generated for a new table. Co-authored-by: Andrew Werner <[email protected]>
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
May 4, 2020
Prior to this change, the StatsRefresher was being notified of a new table during the execution of the createTable node, before the creating transaction had committed. Prior to cockroachdb#46170, the StatsRefresher was likely to block on the intent of the creating transaction. After that change, the StatsRefresher might not discover the new table because the creating transaction is much more likely to get pushed further into the future, past the read of the stats refresher. Release note (bug fix): Fix rare bug where stats were not automatically generated for a new table.
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
May 4, 2020
Prior to this change, the StatsRefresher was being notified of a new table during the execution of the createTable node, before the creating transaction had committed. Prior to cockroachdb#46170, the StatsRefresher was likely to block on the intent of the creating transaction. After that change, the StatsRefresher might not discover the new table because the creating transaction is much more likely to get pushed further into the future, past the read of the stats refresher. Release note (bug fix): Fix rare bug where stats were not automatically generated for a new table.
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.
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 #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.