From 7caf7978d888875c3ebc1c47d1fd182e1f695f17 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Fri, 20 Mar 2020 16:43:16 -0400 Subject: [PATCH] sql: resolve databases in high-priority transactions 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. --- pkg/sql/database.go | 17 +++++++++++++++++ .../testdata/logic_test/schema_change_in_txn | 13 ++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/pkg/sql/database.go b/pkg/sql/database.go index 2a5a13d74606..2298acd3f69f 100644 --- a/pkg/sql/database.go +++ b/pkg/sql/database.go @@ -17,6 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/config" "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -178,6 +179,14 @@ func (dc *databaseCache) getDatabaseDesc( } if desc == nil { if err := txnRunner(ctx, func(ctx context.Context, txn *kv.Txn) error { + // Run the descriptor read as high-priority, thereby pushing any intents out + // of its way. We don't want schema changes to prevent database lookup; + // we'd rather force them to refresh. Also this prevents deadlocks in cases + // where the name resolution is triggered by the transaction doing the + // schema change itself. + if err := txn.SetUserPriority(roachpb.MaxUserPriority); err != nil { + return err + } a := UncachedPhysicalAccessor{} desc, err = a.GetDatabaseDesc(ctx, txn, name, tree.DatabaseLookupFlags{Required: required}) @@ -222,6 +231,14 @@ func (dc *databaseCache) getDatabaseID( } if dbID == sqlbase.InvalidID { if err := txnRunner(ctx, func(ctx context.Context, txn *kv.Txn) error { + // Run the namespace read as high-priority, thereby pushing any intents out + // of its way. We don't want schema changes to prevent database acquisitions; + // we'd rather force them to refresh. Also this prevents deadlocks in cases + // where the name resolution is triggered by the transaction doing the + // schema change itself. + if err := txn.SetUserPriority(roachpb.MaxUserPriority); err != nil { + return err + } var err error dbID, err = getDatabaseID(ctx, txn, name, required) return err diff --git a/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn b/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn index dbdf7b92f0db..c3fe4bf9e748 100644 --- a/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn +++ b/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn @@ -1777,7 +1777,7 @@ COMMIT # namespace entries could block the schema resolution after the rollback (schema # resolution uses different transactions to do its reads). We've fixed it by having those # other transactions run at high priority, thus pushing the intents out of their way. -subtest no_schemachange_deadlock_after_savepoint_rollback +subtest no_table_schemachange_deadlock_after_savepoint_rollback statement ok begin; savepoint s; create table t(x int); rollback to savepoint s; @@ -1787,3 +1787,14 @@ select * from t; statement ok commit; + +subtest no_database_schemachange_deadlock_after_savepoint_rollback + +statement ok +begin; savepoint s; create database d46224; rollback to savepoint s; + +query error relation "d46224.t" does not exist +select * from d46224.t; + +statement ok +commit;