From 52653e605aa0f7b79c0fc7fbdd453c9af22311a5 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Tue, 31 Mar 2020 23:09:50 -0400 Subject: [PATCH 1/4] sql: deal with retriable errors when using a new txn In #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 #43251 Release note: None --- pkg/sql/conn_executor_prepare.go | 45 +++++++++++++++++--------------- pkg/sql/conn_executor_test.go | 32 +++++++++++++++++++++++ pkg/sql/exec_util.go | 4 +++ 3 files changed, 60 insertions(+), 21 deletions(-) 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/exec_util.go b/pkg/sql/exec_util.go index 269ca358d1c6..a96b9d291643 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) From a2dde8d66bc28e5eef4a299e6deffc97e6a3fb62 Mon Sep 17 00:00:00 2001 From: Andrii Vorobiov Date: Wed, 1 Apr 2020 17:51:21 +0300 Subject: [PATCH 2/4] ui: (fix) Tooltip component styling and props This fix is rework of previous changes related to PR: https://github.com/cockroachdb/cockroach/pull/46557 Prior, to customize tooltip width for two particular cases, changes were made in shared component and affected all tooltips across project (tooltip width was set to 500px). Instead of this, current changes keep component styles without local changes and extend them with specific classes (via `overlayClassName` prop). It allows to apply changes in specific places (Statements and Jobs tables). Next fix: the order of destructing props in `components/tooltip/tooltip.tsx`. `{...props}` supplied as a last prop to `` component and it overrides all previous props which have to be preserved. To fix this, ...props was moved as first prop. And last fix: Tooltips for Diagnostics Status Badge was set to be visible always and with some random conditions tooltips appeared and were displayed instantly. To fix this, `visible` prop was removed to trigger tooltip visibility only on mouse hover. And to position Diagnostics Status Badge tooltip more elegantly - it is positioned to `bottomLeft` side, because this badge is displayed in the last columns and there is not enough place on the right side for tooltip. Release note (bug fix): Tooltips for statement diagnostics were shown always instead of only on hover Release justification: bug fixes and low-risk updates to new functionality --- pkg/ui/src/components/tooltip/tooltip.styl | 2 -- pkg/ui/src/components/tooltip/tooltip.tsx | 6 +++--- pkg/ui/src/views/jobs/jobDescriptionCell.tsx | 11 ++++++++--- .../statements/diagnostics/diagnosticStatusBadge.tsx | 3 +-- pkg/ui/src/views/statements/statements.styl | 5 +++++ pkg/ui/src/views/statements/statementsTable.tsx | 10 +++++++--- 6 files changed, 24 insertions(+), 13 deletions(-) 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) }
From de30021bf4b1b447fab56a03ffa97dfbc1635f30 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 1 Apr 2020 10:02:29 +0200 Subject: [PATCH 3/4] sqlmigrations: prevent schema change noise upon cluster creation The system.comments is now created without write permission to role public. No need to re-do that change on every new cluster. Release note: None Co-authored-by: Lucy Zhang --- pkg/sql/drop_test.go | 37 ++++++++----------- .../testdata/logic_test/information_schema | 2 +- pkg/sql/schema_changer_test.go | 21 +++++------ pkg/sqlmigrations/migrations.go | 5 ++- 4 files changed, 29 insertions(+), 36 deletions(-) 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/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. From 4ef2d9abe32eaef07077164a76b5ad343e7c4710 Mon Sep 17 00:00:00 2001 From: Bilal Akhtar Date: Wed, 1 Apr 2020 15:29:20 -0400 Subject: [PATCH 4/4] vendor: Bump pebble to 74d69792cb150369fb7a799be862524ad2b39d51 Pulls in these changes: - *: use errors.Errorf in replace of fmt.Errorf - *: use github.com/cockroachdb/errors - vendor: add cockroachdb/errors and its dependencies - *: add FileNum type - db: delete orphaned temporary files in Open - version_set: Handle case where RocksDB doesn't bump minUnflushedLogNum - db: modify Options.DebugCheck to be a function - internal/manifest: Relax SeqNum overlap invariant in L0 - internal/metamorphic: fix ingest_using_apply implementation - *: use Go 1.13 error wrapping - internal/metamorphic: randomize MaxConcurrentCompactions - internal/metamorphic: enable run comparison by default - internal/metamorphic: temporarily disable ingest_using_apply - db: document that Close may not be called twice - db: use require.* functions for test checks - *: use require.NoError everywhere - internal/errorfs: move to new package Release note: None --- Gopkg.lock | 4 ++-- vendor | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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/vendor b/vendor index f4d06c7a9e21..f4e764600792 160000 --- a/vendor +++ b/vendor @@ -1 +1 @@ -Subproject commit f4d06c7a9e217ad0567b670e95feeb979c417de6 +Subproject commit f4e764600792ee9071e8e406108f7401374b1765