diff --git a/Gopkg.lock b/Gopkg.lock index 3091939b4a9d..90d361f3ce41 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -464,7 +464,7 @@ [[projects]] branch = "master" - digest = "1:8582389220996045b3c5e31835635ab607214cbcc075eb3200194bdc699678ef" + digest = "1:72cd3a6421449aeb9b90ac2d284695178adb9372f89f82b088dca4c7b9602c31" name = "github.com/cockroachdb/pebble" packages = [ ".", @@ -490,7 +490,7 @@ "vfs", ] pruneopts = "UT" - revision = "a06c162aa79d8acdafff5609a1c30328457dae04" + revision = "74d69792cb150369fb7a799be862524ad2b39d51" [[projects]] branch = "master" diff --git a/pkg/sql/conn_executor_prepare.go b/pkg/sql/conn_executor_prepare.go index 6a1db7130bdd..ea8388401b69 100644 --- a/pkg/sql/conn_executor_prepare.go +++ b/pkg/sql/conn_executor_prepare.go @@ -164,34 +164,32 @@ func (ex *connExecutor) prepare( // Preparing needs a transaction because it needs to retrieve db/table // descriptors for type checking. If we already have an open transaction for // this planner, use it. Using the user's transaction here is critical for - // proper deadlock detection. At the time of writing it is the case that any + // proper deadlock detection. At the time of writing, it is the case that any // data read on behalf of this transaction is not cached for use in other // transactions. It's critical that this fact remain true but nothing really // enforces it. If we create a new transaction (newTxn is true), we'll need to // finish it before we return. - newTxn, txn := false, ex.state.mu.txn - if txn == nil || !txn.IsOpen() { - newTxn, txn = true, kv.NewTxn(ctx, ex.server.cfg.DB, ex.server.cfg.NodeID.Get()) + + var flags planFlags + prepare := func(ctx context.Context, txn *kv.Txn) (err error) { + ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes) + p := &ex.planner + ex.resetPlanner(ctx, p, txn, ex.server.cfg.Clock.PhysicalTime() /* stmtTS */) + p.stmt = &stmt + p.semaCtx.Annotations = tree.MakeAnnotations(stmt.NumAnnotations) + flags, err = ex.populatePrepared(ctx, txn, placeholderHints, p) + return err } - ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes) - p := &ex.planner - ex.resetPlanner(ctx, p, txn, ex.server.cfg.Clock.PhysicalTime() /* stmtTS */) - p.stmt = &stmt - p.semaCtx.Annotations = tree.MakeAnnotations(stmt.NumAnnotations) - flags, err := ex.populatePrepared(ctx, txn, placeholderHints, p) - if err != nil { - // NB: if this is not a new transaction then let the connExecutor state - // machine decide whether we should clean up intents; we may be restarting - // and want to leave them in place. - if newTxn { - txn.CleanupOnError(ctx, err) + if txn := ex.state.mu.txn; txn != nil && txn.IsOpen() { + // Use the existing transaction. + if err := prepare(ctx, txn); err != nil { + return nil, err } - return nil, err - } - if newTxn { - // Clean up the newly created transaction if we made one. - if err := txn.CommitOrCleanup(ctx); err != nil { + } else { + // Use a new transaction. This will handle retriable errors here rather + // than bubbling them up to the connExecutor state machine. + if err := ex.server.cfg.DB.Txn(ctx, prepare); err != nil { return nil, err } } @@ -209,6 +207,11 @@ func (ex *connExecutor) prepare( func (ex *connExecutor) populatePrepared( ctx context.Context, txn *kv.Txn, placeholderHints tree.PlaceholderTypes, p *planner, ) (planFlags, error) { + if before := ex.server.cfg.TestingKnobs.BeforePrepare; before != nil { + if err := before(ctx, ex.planner.stmt.String(), txn); err != nil { + return 0, err + } + } stmt := p.stmt if err := p.semaCtx.Placeholders.Init(stmt.NumPlaceholders, placeholderHints); err != nil { return 0, err diff --git a/pkg/sql/conn_executor_test.go b/pkg/sql/conn_executor_test.go index 2976320b496a..e9a229a68376 100644 --- a/pkg/sql/conn_executor_test.go +++ b/pkg/sql/conn_executor_test.go @@ -23,6 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/storagebase" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -631,6 +632,37 @@ func TestPrepareInExplicitTransactionDoesNotDeadlock(t *testing.T) { } } +// TestRetriableErrorDuringPrepare ensures that when preparing and using a new +// transaction, retriable errors are handled properly and do not propagate to +// the user's transaction. +func TestRetriableErrorDuringPrepare(t *testing.T) { + defer leaktest.AfterTest(t)() + const uniqueString = "'a very unique string'" + var failed int64 + const numToFail = 2 // only fail on the first two attempts + s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{ + Knobs: base.TestingKnobs{ + SQLExecutor: &sql.ExecutorTestingKnobs{ + BeforePrepare: func(ctx context.Context, stmt string, txn *kv.Txn) error { + if strings.Contains(stmt, uniqueString) && atomic.AddInt64(&failed, 1) <= numToFail { + return roachpb.NewTransactionRetryWithProtoRefreshError("boom", + txn.ID(), *txn.TestingCloneTxn()) + } + return nil + }, + }, + }, + }) + defer s.Stopper().Stop(context.Background()) + + testDB := sqlutils.MakeSQLRunner(sqlDB) + testDB.Exec(t, "CREATE TABLE foo (i INT PRIMARY KEY)") + + stmt, err := sqlDB.Prepare("SELECT " + uniqueString) + require.NoError(t, err) + defer func() { _ = stmt.Close() }() +} + // This test ensures that when in an explicit transaction and statement // preparation uses the user's transaction, errors during those planning queries // are handled correctly. diff --git a/pkg/sql/drop_test.go b/pkg/sql/drop_test.go index 4194df0f792c..d3776bad248b 100644 --- a/pkg/sql/drop_test.go +++ b/pkg/sql/drop_test.go @@ -231,11 +231,10 @@ INSERT INTO t.kv VALUES ('c', 'e'), ('a', 'c'), ('b', 'd'); } // Job still running, waiting for GC. - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. sqlRun := sqlutils.MakeSQLRunner(sqlDB) - if err := jobutils.VerifySystemJob(t, sqlRun, 4, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ + if err := jobutils.VerifySystemJob(t, sqlRun, 0, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, Description: "DROP DATABASE t CASCADE", DescriptorIDs: sqlbase.IDs{ @@ -382,10 +381,9 @@ INSERT INTO t.kv2 VALUES ('c', 'd'), ('a', 'b'), ('e', 'a'); tests.CheckKeyCount(t, kvDB, tableSpan, 6) tests.CheckKeyCount(t, kvDB, table2Span, 6) - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - const migrationJobOffset = 4 + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. + const migrationJobOffset = 0 sqlRun := sqlutils.MakeSQLRunner(sqlDB) if err := jobutils.VerifySystemJob(t, sqlRun, migrationJobOffset, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, @@ -559,10 +557,9 @@ func TestDropIndex(t *testing.T) { tests.CheckKeyCount(t, kvDB, indexSpan, numRows) tests.CheckKeyCount(t, kvDB, tableDesc.TableSpan(), 3*numRows) - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - const migrationJobOffset = 4 + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. + const migrationJobOffset = 0 sqlRun := sqlutils.MakeSQLRunner(sqlDB) if err := jobutils.VerifySystemJob(t, sqlRun, migrationJobOffset+1, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, @@ -776,7 +773,7 @@ func TestDropTable(t *testing.T) { // Job still running, waiting for GC. sqlRun := sqlutils.MakeSQLRunner(sqlDB) - if err := jobutils.VerifySystemJob(t, sqlRun, 5, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ + if err := jobutils.VerifySystemJob(t, sqlRun, 1, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, Description: `DROP TABLE t.public.kv`, DescriptorIDs: sqlbase.IDs{ @@ -848,10 +845,9 @@ func TestDropTableDeleteData(t *testing.T) { } } - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - const migrationJobOffset = 4 + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. + const migrationJobOffset = 0 // Data hasn't been GC-ed. sqlRun := sqlutils.MakeSQLRunner(sqlDB) @@ -1133,12 +1129,11 @@ func TestDropDatabaseAfterDropTable(t *testing.T) { } // Job still running, waiting for draining names. - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. sqlRun := sqlutils.MakeSQLRunner(sqlDB) if err := jobutils.VerifySystemJob( - t, sqlRun, 5, jobspb.TypeSchemaChange, jobs.StatusSucceeded, + t, sqlRun, 1, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, Description: "DROP TABLE t.public.kv", diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index e9f3d2cd619b..275c318df257 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -652,6 +652,10 @@ type ExecutorTestingKnobs struct { // statement has been executed. StatementFilter StatementFilter + // BeforePrepare can be used to trap execution of SQL statement preparation. + // If a nil error is returned, planning continues as usual. + BeforePrepare func(ctx context.Context, stmt string, txn *kv.Txn) error + // BeforeExecute is called by the Executor before plan execution. It is useful // for synchronizing statement execution. BeforeExecute func(ctx context.Context, stmt string) diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 37f945111978..707be8adb169 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -625,7 +625,7 @@ system public web_sessions BASE TABLE system public table_statistics BASE TABLE YES 1 system public locations BASE TABLE YES 1 system public role_members BASE TABLE YES 1 -system public comments BASE TABLE YES 5 +system public comments BASE TABLE YES 1 system public replication_constraint_stats BASE TABLE YES 1 system public replication_critical_localities BASE TABLE YES 1 system public replication_stats BASE TABLE YES 1 diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index 31a9a1ceda93..2091383a5f3a 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -1843,10 +1843,9 @@ CREATE TABLE t.test (k INT PRIMARY KEY, v INT8); // State of jobs table t.Skip("TODO(pbardea): The following fails due to causes seemingly unrelated to GC") runner := sqlutils.SQLRunner{DB: sqlDB} - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - const migrationJobOffset = 4 + // TODO (lucy): This test API should use an offset starting from the + // most recent job instead. + const migrationJobOffset = 0 for i, tc := range testCases { status := jobs.StatusSucceeded if tc.errString != "" { @@ -3989,10 +3988,9 @@ CREATE TABLE t.test (k INT PRIMARY KEY, v INT, pi DECIMAL REFERENCES t.pi (d) DE // Ensure that the job is marked as succeeded. sqlRun := sqlutils.MakeSQLRunner(sqlDB) - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - schemaChangeJobOffset := 4 + // TODO (lucy): This test API should use an offset starting from the + // most recent job instead. + schemaChangeJobOffset := 0 if err := jobutils.VerifySystemJob(t, sqlRun, schemaChangeJobOffset+2, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, Description: "TRUNCATE TABLE t.public.test", @@ -5542,10 +5540,9 @@ INSERT INTO t.test (k, v) VALUES (1, 99), (2, 100); sqlRun := sqlutils.MakeSQLRunner(sqlDB) runBeforeConstraintValidation = func() error { - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - return jobutils.VerifyRunningSystemJob(t, sqlRun, 4, jobspb.TypeSchemaChange, sql.RunningStatusValidation, jobs.Record{ + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. + return jobutils.VerifyRunningSystemJob(t, sqlRun, 0, jobspb.TypeSchemaChange, sql.RunningStatusValidation, jobs.Record{ Username: security.RootUser, Description: "ALTER TABLE t.public.test ADD COLUMN a INT8 AS (v - 1) STORED, ADD CHECK ((a < v) AND (a IS NOT NULL))", DescriptorIDs: sqlbase.IDs{ diff --git a/pkg/sqlmigrations/migrations.go b/pkg/sqlmigrations/migrations.go index 5acaaacc5550..2ae53abec03f 100644 --- a/pkg/sqlmigrations/migrations.go +++ b/pkg/sqlmigrations/migrations.go @@ -285,8 +285,9 @@ var backwardCompatibleMigrations = []migrationDescriptor{ }, { // Introduced in v20.1. - name: "remove public permissions on system.comments", - workFn: depublicizeSystemComments, + name: "remove public permissions on system.comments", + includedInBootstrap: clusterversion.VersionByKey(clusterversion.VersionSchemaChangeJob), + workFn: depublicizeSystemComments, }, { // Introduced in v20.1. diff --git a/pkg/ui/src/components/tooltip/tooltip.styl b/pkg/ui/src/components/tooltip/tooltip.styl index 9dd06c7bbd1e..6ef68445bc63 100644 --- a/pkg/ui/src/components/tooltip/tooltip.styl +++ b/pkg/ui/src/components/tooltip/tooltip.styl @@ -11,9 +11,7 @@ @require '~src/components/core/index.styl' .tooltip-overlay - max-width max-content .ant-tooltip-content - width 500px .ant-tooltip-inner @extend $text--body line-height $line-height--small diff --git a/pkg/ui/src/components/tooltip/tooltip.tsx b/pkg/ui/src/components/tooltip/tooltip.tsx index cbf820dd01f4..6f75af444a2e 100644 --- a/pkg/ui/src/components/tooltip/tooltip.tsx +++ b/pkg/ui/src/components/tooltip/tooltip.tsx @@ -21,13 +21,13 @@ export interface TooltipProps { } export function Tooltip(props: TooltipProps & AntTooltipProps) { - const { children, theme } = props; - const classes = cn("tooltip-overlay", `crl-tooltip--theme-${theme}`); + const { children, theme, overlayClassName } = props; + const classes = cn("tooltip-overlay", `crl-tooltip--theme-${theme}`, overlayClassName); return ( {children} diff --git a/pkg/ui/src/views/jobs/jobDescriptionCell.tsx b/pkg/ui/src/views/jobs/jobDescriptionCell.tsx index dc2126c1d096..07199cfa51d9 100644 --- a/pkg/ui/src/views/jobs/jobDescriptionCell.tsx +++ b/pkg/ui/src/views/jobs/jobDescriptionCell.tsx @@ -25,9 +25,14 @@ export class JobDescriptionCell extends React.PureComponent<{ job: Job }> { return (
- {description} - }> + {description} + } + overlayClassName="cl-table-link__statement-tooltip--fixed-width" + >
{job.statement || job.description}
diff --git a/pkg/ui/src/views/statements/diagnostics/diagnosticStatusBadge.tsx b/pkg/ui/src/views/statements/diagnostics/diagnosticStatusBadge.tsx index 08f96beadbe7..ba8f617daaf2 100644 --- a/pkg/ui/src/views/statements/diagnostics/diagnosticStatusBadge.tsx +++ b/pkg/ui/src/views/statements/diagnostics/diagnosticStatusBadge.tsx @@ -99,10 +99,9 @@ export function DiagnosticStatusBadge(props: OwnProps) { return (
- { getHighlightedText(props.statement, props.search) } - }> + + { getHighlightedText(props.statement, props.search) } + } + overlayClassName="cl-table-link__statement-tooltip--fixed-width" + >
{ getHighlightedText(shortStatement(summary, props.statement), props.search, true) }
diff --git a/vendor b/vendor index f4d06c7a9e21..f4e764600792 160000 --- a/vendor +++ b/vendor @@ -1 +1 @@ -Subproject commit f4d06c7a9e217ad0567b670e95feeb979c417de6 +Subproject commit f4e764600792ee9071e8e406108f7401374b1765