From 1424d308a9b033b3a49e7db9221b2b1feaaf493d Mon Sep 17 00:00:00 2001 From: Blake Kostner Date: Fri, 17 Mar 2023 16:03:13 -0600 Subject: [PATCH 1/6] feat: allow starting docker container via env variable Release note (general change): Allow setting docker command args via the `COCKROACH_ARGS` environment variable. --- build/deploy/cockroach.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/build/deploy/cockroach.sh b/build/deploy/cockroach.sh index 46433b2a239d..17ae9b4cceef 100755 --- a/build/deploy/cockroach.sh +++ b/build/deploy/cockroach.sh @@ -295,4 +295,10 @@ _main() { esac } -_main "$@" +set_env_var "COCKROACH_ARGS" + +if [[ -n "$COCKROACH_ARGS" ]]; then + _main "$COCKROACH_ARGS" +else + _main "$@" +fi From bdc8d1ae9fc181019181349588d56dd235de8006 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sun, 26 Mar 2023 14:18:10 +0200 Subject: [PATCH 2/6] sql: block DROP TENANT based on a session var In clusters where we will promote tenant management operations, we would like to ensure there is one extra step needed for administrators to drop a tenant (and thus irremedially lose data). Given that `sql_safe_updates` is not set automatically when users open their SQL session using their own client, we need another mechanism. This change introduces the new (hidden) session var, `disable_drop_tenant`. When set, tenant deletion fails with the following error message: ``` demo@127.0.0.1:26257/movr> drop tenant foo; ERROR: rejected (via sql_safe_updates or disable_drop_tenant): DROP TENANT causes irreversible data loss SQLSTATE: 01000 ``` (The session var `sql_safe_updates` is _also_ included as a blocker in the mechanism so that folk using `cockroach sql` get double protection). The default value of this session var is `false` in single-tenant clusters (set via a cluster setting `sql.drop_tenant.enabled`), for compatibility with CC Serverless. The default will be set to `true` via a config profile when suitable. Release note: None --- pkg/sql/exec_util.go | 23 +++++++++++++++ .../testdata/logic_test/information_schema | 1 + pkg/sql/logictest/testdata/logic_test/tenant | 18 ++++++++++++ .../local_only_session_data.proto | 4 ++- pkg/sql/tenant_deletion.go | 7 +++++ pkg/sql/vars.go | 28 +++++++++++++++++++ 6 files changed, 80 insertions(+), 1 deletion(-) diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 9eb59cba9be8..1274f9d24834 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -747,6 +747,25 @@ var errTransactionInProgress = errors.New("there is already a transaction in pro const sqlTxnName string = "sql txn" const metricsSampleInterval = 10 * time.Second +// enableDropTenant (or rather, its inverted boolean value) defines +// the default value for the session var "disable_drop_tenant". +// +// Note: +// - We use a cluster setting here instead of a default role option +// because we need this to be settable also for the 'admin' role. +// - The cluster setting is named "enable" because boolean cluster +// settings are all ".enabled" -- we do not have ".disabled" +// settings anywhere. +// - The session var is named "disable_" because we want the Go +// default value (false) to mean that tenant deletion is enabled. +// This is needed for backward-compatibility with Cockroach Cloud. +var enableDropTenant = settings.RegisterBoolSetting( + settings.SystemOnly, + "sql.drop_tenant.enabled", + "default value (inverted) for the disable_drop_tenant session setting", + true, +) + // Fully-qualified names for metrics. var ( MetaSQLExecLatency = metric.Metadata{ @@ -3146,6 +3165,10 @@ func (m *sessionDataMutator) SetSafeUpdates(val bool) { m.data.SafeUpdates = val } +func (m *sessionDataMutator) SetDisableDropTenant(val bool) { + m.data.DisableDropTenant = val +} + func (m *sessionDataMutator) SetCheckFunctionBodies(val bool) { m.data.CheckFunctionBodies = val } diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 34a3b2820331..06ffe0124e70 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -5206,6 +5206,7 @@ default_transaction_read_only off default_transaction_use_follower_reads off default_with_oids off descriptor_validation on +disable_drop_tenant off disable_hoist_projection_in_join_limitation off disable_partially_distributed_plans off disable_plan_gists off diff --git a/pkg/sql/logictest/testdata/logic_test/tenant b/pkg/sql/logictest/testdata/logic_test/tenant index 7169f9ccfc13..370c6beae33a 100644 --- a/pkg/sql/logictest/testdata/logic_test/tenant +++ b/pkg/sql/logictest/testdata/logic_test/tenant @@ -474,3 +474,21 @@ DROP TENANT tmpl statement ok RESET CLUSTER SETTING sql.create_tenant.default_template + +subtest block_drop_tenant + +statement ok +SET disable_drop_tenant = 'true' + +statement ok +CREATE TENANT nodelete + +statement error rejected.*irreversible data loss +DROP TENANT nodelete + +statement ok +RESET disable_drop_tenant + +statement ok +DROP TENANT nodelete + diff --git a/pkg/sql/sessiondatapb/local_only_session_data.proto b/pkg/sql/sessiondatapb/local_only_session_data.proto index 15e46dce7262..9a5fd41d163d 100644 --- a/pkg/sql/sessiondatapb/local_only_session_data.proto +++ b/pkg/sql/sessiondatapb/local_only_session_data.proto @@ -318,7 +318,6 @@ message LocalOnlySessionData { // optimizer should use improved statistics calculations for disjunctive // filters. bool optimizer_use_improved_disjunction_stats = 86; - // OptimizerUseLimitOrderingForStreamingGroupBy enables the exploration rule // which optimizes 'SELECT ... GROUP BY ... ORDER BY ... LIMIT n' queries. // The rule uses the required ordering in the limit expression to inform an @@ -367,6 +366,9 @@ message LocalOnlySessionData { int64 prepared_statements_cache_size = 97; // StreamerEnabled controls whether the Streamer API can be used. bool streamer_enabled = 98; + // DisableDropTenant causes errors when the client + // attempts to drop tenants or tenant records. + bool disable_drop_tenant = 99; /////////////////////////////////////////////////////////////////////////// // WARNING: consider whether a session parameter you're adding needs to // diff --git a/pkg/sql/tenant_deletion.go b/pkg/sql/tenant_deletion.go index e16dcfc326da..66e3a491ee7e 100644 --- a/pkg/sql/tenant_deletion.go +++ b/pkg/sql/tenant_deletion.go @@ -32,6 +32,13 @@ import ( func (p *planner) DropTenantByID( ctx context.Context, tenID uint64, synchronousImmediateDrop, ignoreServiceMode bool, ) error { + if p.SessionData().DisableDropTenant || p.SessionData().SafeUpdates { + err := errors.Newf("DROP TENANT causes irreversible data loss") + err = errors.WithMessage(err, "rejected (via sql_safe_updates or disable_drop_tenant)") + err = pgerror.WithCandidateCode(err, pgcode.Warning) + return err + } + if p.EvalContext().TxnReadOnly { return readOnlyError("DROP TENANT") } diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index 8c10a9a85ead..8da2c2f33c8a 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -1520,6 +1520,34 @@ var varGen = map[string]sessionVar{ // Setting is done by the SetTracing statement. }, + // CockroachDB extension. + `disable_drop_tenant`: { + Hidden: true, + Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) { + return formatBoolAsPostgresSetting(evalCtx.SessionData().DisableDropTenant), nil + }, + Set: func(_ context.Context, m sessionDataMutator, s string) error { + b, err := paramparse.ParseBoolVar("disable_drop_tenant", s) + if err != nil { + return err + } + m.SetDisableDropTenant(b) + return nil + }, + GlobalDefault: func(sv *settings.Values) string { + // Note: + // - We use a cluster setting here instead of a default role option + // because we need this to be settable also for the 'admin' role. + // - The cluster setting is named "enable" because boolean cluster + // settings are all ".enabled" -- we do not have ".disabled" + // settings anywhere. + // - The session var is named "disable_" because we want the Go + // default value (false) to mean that tenant deletion is enabled. + // This is needed for backward-compatibility with Cockroach Cloud. + return formatBoolAsPostgresSetting(!enableDropTenant.Get(sv)) + }, + }, + // CockroachDB extension. `allow_prepare_as_opt_plan`: { Hidden: true, From ab10d6c86d8a2442460386a36c5fc0cac97c3281 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Mon, 27 Mar 2023 18:59:05 -0700 Subject: [PATCH 3/6] sql: remove no longer used channel in createStatsNode This hasn't been used as of fe6377c4f1b6e822bb436f64808762e02a815c11. Also mark `create_stats.go` as owned by SQL Queries. Epic: None Release note: None --- .github/CODEOWNERS | 1 + pkg/sql/create_stats.go | 11 +++-------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 5a94785a85a8..c8df0df75344 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -48,6 +48,7 @@ /pkg/sql/stats/ @cockroachdb/sql-queries /pkg/sql/col* @cockroachdb/sql-queries +/pkg/sql/create_stats* @cockroachdb/sql-queries /pkg/sql/distsql*.go @cockroachdb/sql-queries /pkg/sql/exec* @cockroachdb/sql-queries #!/pkg/sql/exec_log*.go @cockroachdb/sql-queries-noreview diff --git a/pkg/sql/create_stats.go b/pkg/sql/create_stats.go index 55797f257b51..718f8fc40a88 100644 --- a/pkg/sql/create_stats.go +++ b/pkg/sql/create_stats.go @@ -104,22 +104,19 @@ type createStatsNode struct { // createStatsRun contains the run-time state of createStatsNode during local // execution. type createStatsRun struct { - resultsCh chan tree.Datums - errCh chan error + errCh chan error } func (n *createStatsNode) startExec(params runParams) error { telemetry.Inc(sqltelemetry.SchemaChangeCreateCounter("stats")) - n.run.resultsCh = make(chan tree.Datums) n.run.errCh = make(chan error) go func() { - err := n.startJob(params.ctx, n.run.resultsCh) + err := n.startJob(params.ctx) select { case <-params.ctx.Done(): case n.run.errCh <- err: } close(n.run.errCh) - close(n.run.resultsCh) }() return nil } @@ -130,8 +127,6 @@ func (n *createStatsNode) Next(params runParams) (bool, error) { return false, params.ctx.Err() case err := <-n.run.errCh: return false, err - case <-n.run.resultsCh: - return true, nil } } @@ -139,7 +134,7 @@ func (*createStatsNode) Close(context.Context) {} func (*createStatsNode) Values() tree.Datums { return nil } // startJob starts a CreateStats job to plan and execute statistics creation. -func (n *createStatsNode) startJob(ctx context.Context, resultsCh chan<- tree.Datums) error { +func (n *createStatsNode) startJob(ctx context.Context) error { record, err := n.makeJobRecord(ctx) if err != nil { return err From 119c54d0bab41c98dfb574afd7e0685c01e1a28d Mon Sep 17 00:00:00 2001 From: maryliag Date: Wed, 29 Mar 2023 14:22:52 -0400 Subject: [PATCH 4/6] ui: add checks for values Fixes #99655 Fixes #99538 Fixes #99539 Add checks to usages that could cause `Cannot read properties of undefined`. Release note: None --- .../cluster-ui/src/api/schemaInsightsApi.ts | 3 +++ .../cluster-ui/src/api/txnInsightsApi.ts | 2 +- .../databaseDetailsPage/databaseDetailsPage.tsx | 14 ++++++-------- .../cluster-ui/src/databasesPage/databasesPage.tsx | 14 ++++++-------- .../src/highlightedText/highlightedText.tsx | 2 +- .../multiSelectCheckbox/multiSelectCheckbox.tsx | 11 ++++++----- .../src/selectors/recentExecutions.selectors.ts | 2 +- .../src/sessions/sessionsPageConnected.tsx | 6 +++--- .../workspaces/cluster-ui/src/sqlActivity/util.tsx | 8 ++++---- .../statementDetails/planDetails/planDetails.tsx | 3 +++ .../src/statementsPage/statementsPage.selectors.ts | 12 ++++++------ .../src/statementsPage/statementsPage.tsx | 2 +- .../statementInsights.selectors.ts | 2 +- .../cluster-ui/src/store/jobs/jobs.selectors.ts | 2 +- .../src/store/sessions/sessions.selectors.ts | 2 +- .../transactionsPage/transactionsPage.selectors.ts | 2 +- .../cluster-ui/src/transactionsPage/utils.ts | 4 ++-- pkg/ui/workspaces/cluster-ui/src/util/format.ts | 2 +- .../workspaces/cluster-ui/src/util/formatNumber.ts | 2 +- .../redux/indexUsageStats/indexUsageStatsSagas.ts | 3 +++ .../src/selectors/recentExecutionsSelectors.ts | 2 +- .../db-console/src/util/highlightedText.tsx | 2 +- .../src/views/sessions/sessionDetails.tsx | 2 +- .../db-console/src/views/sessions/sessionsPage.tsx | 4 ++-- .../src/views/statements/statementsPage.tsx | 10 +++++----- .../src/views/transactions/transactionsPage.tsx | 2 +- 26 files changed, 63 insertions(+), 57 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts index 64794d20bd33..04b688358c36 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts @@ -95,6 +95,9 @@ function createIndexRecommendationsToSchemaInsight( txn_result.rows.forEach(row => { row.index_recommendations.forEach(rec => { + if (!rec.includes(" : ")) { + return; + } const recSplit = rec.split(" : "); const recType = recSplit[0]; const recQuery = recSplit[1]; diff --git a/pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts index 1e7b99a3c564..61767edd6e2c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts @@ -384,7 +384,7 @@ function formatTxnInsightsRow(row: TxnInsightsResponseRow): TxnInsightEvent { transactionExecutionID: row.txn_id, transactionFingerprintID: row.txn_fingerprint_id, implicitTxn: row.implicit_txn, - query: row.query.split(" ; ").join("\n"), + query: row.query?.split(" ; ").join("\n") || "", startTime, endTime, elapsedTimeMillis: endTime.diff(startTime, "milliseconds"), diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx index de00c45ade4b..a18f026a036c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx @@ -378,9 +378,9 @@ export class DatabaseDetailsPage extends React.Component< const { search, tables, filters, nodeRegions } = this.props; const regionsSelected = - filters.regions.length > 0 ? filters.regions.split(",") : []; + filters.regions?.length > 0 ? filters.regions.split(",") : []; const nodesSelected = - filters.nodes.length > 0 ? filters.nodes.split(",") : []; + filters.nodes?.length > 0 ? filters.nodes.split(",") : []; return tables .filter(table => (search ? filterBySearchQuery(table, search) : true)) @@ -392,13 +392,11 @@ export class DatabaseDetailsPage extends React.Component< let foundNode = nodesSelected.length == 0; table.details.nodes?.forEach(node => { - if ( - foundRegion || - regionsSelected.includes(nodeRegions[node.toString()]) - ) { + const n = node?.toString() || ""; + if (foundRegion || regionsSelected.includes(nodeRegions[n])) { foundRegion = true; } - if (foundNode || nodesSelected.includes("n" + node.toString())) { + if (foundNode || nodesSelected.includes("n" + n)) { foundNode = true; } if (foundNode && foundRegion) return true; @@ -738,7 +736,7 @@ export class DatabaseDetailsPage extends React.Component< hideAppNames={true} regions={regions} hideTimeLabel={true} - nodes={nodes.map(n => "n" + n.toString())} + nodes={nodes.map(n => "n" + n?.toString())} activeFilters={activeFilters} filters={defaultFilters} onSubmitFilters={this.onSubmitFilters} diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx index b268e3ae9594..9d965bf6f463 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx @@ -424,9 +424,9 @@ export class DatabasesPage extends React.Component< // The regions and nodes selected from the filter dropdown. const regionsSelected = - filters.regions.length > 0 ? filters.regions.split(",") : []; + filters.regions?.length > 0 ? filters.regions.split(",") : []; const nodesSelected = - filters.nodes.length > 0 ? filters.nodes.split(",") : []; + filters.nodes?.length > 0 ? filters.nodes.split(",") : []; return databases .filter(db => (search ? filterBySearchQuery(db, search) : true)) @@ -438,13 +438,11 @@ export class DatabasesPage extends React.Component< let foundNode = nodesSelected.length == 0; db.nodes?.forEach(node => { - if ( - foundRegion || - regionsSelected.includes(nodeRegions[node.toString()]) - ) { + const n = node?.toString() || ""; + if (foundRegion || regionsSelected.includes(nodeRegions[n])) { foundRegion = true; } - if (foundNode || nodesSelected.includes("n" + node.toString())) { + if (foundNode || nodesSelected.includes("n" + n)) { foundNode = true; } if (foundNode && foundRegion) return true; @@ -617,7 +615,7 @@ export class DatabasesPage extends React.Component< hideAppNames={true} regions={regions} hideTimeLabel={true} - nodes={nodes.map(n => "n" + n.toString())} + nodes={nodes.map(n => "n" + n?.toString())} activeFilters={activeFilters} filters={defaultFilters} onSubmitFilters={this.onSubmitFilters} diff --git a/pkg/ui/workspaces/cluster-ui/src/highlightedText/highlightedText.tsx b/pkg/ui/workspaces/cluster-ui/src/highlightedText/highlightedText.tsx index 7858a0ce3abc..6025a275dfa4 100644 --- a/pkg/ui/workspaces/cluster-ui/src/highlightedText/highlightedText.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/highlightedText/highlightedText.tsx @@ -97,7 +97,7 @@ export function getHighlightedText( }) .join("|"); const parts = isOriginalText - ? text.split(new RegExp(`(${search})`, "gi")) + ? text?.split(new RegExp(`(${search})`, "gi")) : rebaseText(text, highlight).split(new RegExp(`(${search})`, "gi")); const highlightClass = hasDarkBkg ? "_text-bold-light" : "_text-bold"; return parts.map((part, i) => { diff --git a/pkg/ui/workspaces/cluster-ui/src/multiSelectCheckbox/multiSelectCheckbox.tsx b/pkg/ui/workspaces/cluster-ui/src/multiSelectCheckbox/multiSelectCheckbox.tsx index 417f9440ba4c..fc8ebce86ecc 100644 --- a/pkg/ui/workspaces/cluster-ui/src/multiSelectCheckbox/multiSelectCheckbox.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/multiSelectCheckbox/multiSelectCheckbox.tsx @@ -102,11 +102,12 @@ export const MultiSelectCheckbox = (props: MultiSelectCheckboxProps) => { field: string, parent: any, ) => { - const selected = selectedOptions - .map(function (option: SelectOption) { - return option.label; - }) - .toString(); + const selected = + selectedOptions + ?.map(function (option: SelectOption) { + return option.label; + }) + .toString() || ""; parent.setState({ filters: { ...parent.state.filters, diff --git a/pkg/ui/workspaces/cluster-ui/src/selectors/recentExecutions.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/selectors/recentExecutions.selectors.ts index e790115fe9d4..75cb2fa25432 100644 --- a/pkg/ui/workspaces/cluster-ui/src/selectors/recentExecutions.selectors.ts +++ b/pkg/ui/workspaces/cluster-ui/src/selectors/recentExecutions.selectors.ts @@ -96,7 +96,7 @@ export const selectContentionDetailsForStatement = createSelector( export const selectAppName = createSelector( (state: AppState) => state.adminUI?.sessions, response => { - if (!response.data) return null; + if (!response?.data) return null; return response.data.internal_app_name_prefix; }, ); diff --git a/pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPageConnected.tsx b/pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPageConnected.tsx index b6e8732ba1c8..633d6145b42b 100644 --- a/pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPageConnected.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPageConnected.tsx @@ -36,7 +36,7 @@ export const selectSessionsData = createSelector( export const selectSessions = createSelector( (state: AppState) => state.adminUI?.sessions, (state: SessionsState) => { - if (!state.data) { + if (!state?.data) { return null; } return state.data.sessions.map(session => { @@ -48,7 +48,7 @@ export const selectSessions = createSelector( export const selectAppName = createSelector( (state: AppState) => state.adminUI?.sessions, (state: SessionsState) => { - if (!state.data) { + if (!state?.data) { return null; } return state.data.internal_app_name_prefix; @@ -64,7 +64,7 @@ export const selectColumns = createSelector( localStorageSelector, localStorage => localStorage["showColumns/SessionsPage"] - ? localStorage["showColumns/SessionsPage"].split(",") + ? localStorage["showColumns/SessionsPage"]?.split(",") : null, ); diff --git a/pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx b/pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx index 5f0bb539cc4e..d770226c812a 100644 --- a/pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx @@ -22,7 +22,7 @@ export function filteredStatementsData( ): AggregateStatistics[] { const timeValue = getTimeValueInSeconds(filters); const sqlTypes = - filters.sqlType.length > 0 + filters.sqlType?.length > 0 ? filters.sqlType.split(",").map(function (sqlType: string) { // Adding "Type" to match the value on the Statement // Possible values: TypeDDL, TypeDML, TypeDCL and TypeTCL @@ -30,12 +30,12 @@ export function filteredStatementsData( }) : []; const databases = - filters.database.length > 0 ? filters.database.split(",") : []; + filters.database?.length > 0 ? filters.database.split(",") : []; if (databases.includes(unset)) { databases.push(""); } - const regions = filters.regions.length > 0 ? filters.regions.split(",") : []; - const nodes = filters.nodes.length > 0 ? filters.nodes.split(",") : []; + const regions = filters.regions?.length > 0 ? filters.regions.split(",") : []; + const nodes = filters.nodes?.length > 0 ? filters.nodes.split(",") : []; // Return statements filtered by the values selected on the filter and // the search text. A statement must match all selected filters to be diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx index 699eafd1c117..6a5c16606b8e 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx @@ -241,6 +241,9 @@ function formatIdxRecommendations( for (let i = 0; i < idxRecs.length; i++) { const rec = idxRecs[i]; let idxType: InsightType; + if (!rec?.includes(" : ")) { + continue; + } const t = rec.split(" : ")[0]; switch (t) { case "creation": diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts index f0be5e98c63c..1b6b90f1f8e2 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts +++ b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts @@ -57,7 +57,7 @@ export const selectStatementsLastUpdated = createSelector( // selectApps returns the array of all apps with statement statistics present // in the data. export const selectApps = createSelector(sqlStatsSelector, sqlStatsState => { - if (!sqlStatsState.data || !sqlStatsState.valid) { + if (!sqlStatsState?.data || !sqlStatsState?.valid) { return []; } @@ -88,7 +88,7 @@ export const selectApps = createSelector(sqlStatsSelector, sqlStatsState => { // selectDatabases returns the array of all databases in the cluster. export const selectDatabases = createSelector(databasesListSelector, state => { - if (!state.data) { + if (!state?.data) { return []; } @@ -102,7 +102,7 @@ export const selectDatabases = createSelector(databasesListSelector, state => { export const selectTotalFingerprints = createSelector( sqlStatsSelector, state => { - if (!state.data) { + if (!state?.data) { return 0; } const aggregated = aggregateStatementStats(state.data.statements); @@ -113,7 +113,7 @@ export const selectTotalFingerprints = createSelector( // selectLastReset returns a string displaying the last time the statement // statistics were reset. export const selectLastReset = createSelector(sqlStatsSelector, state => { - if (!state.data) { + if (!state?.data) { return ""; } @@ -144,7 +144,7 @@ export const selectStatements = createSelector( diagnosticsReportsPerStatement, ): AggregateStatistics[] => { // State is valid if we successfully fetched data, and the data has not yet been invalidated. - if (!state.data || !state.valid) { + if (!state?.data || !state?.valid) { return null; } let statements = flattenStatementStats(state.data.statements); @@ -229,7 +229,7 @@ export const selectColumns = createSelector( // return array of columns if user have customized it or `null` otherwise localStorage => localStorage["showColumns/StatementsPage"] - ? localStorage["showColumns/StatementsPage"].split(",") + ? localStorage["showColumns/StatementsPage"]?.split(",") : null, ); diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx index f4904b19d3b7..7c23dda95fe5 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx @@ -561,7 +561,7 @@ export class StatementsPage extends React.Component< // hiding columns that won't be displayed for tenants. const columns = makeStatementsColumns( statements, - filters.app.split(","), + filters.app?.split(","), this.props.stmtsTotalRuntimeSecs, "statement", isTenant, diff --git a/pkg/ui/workspaces/cluster-ui/src/store/insights/statementInsights/statementInsights.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/store/insights/statementInsights/statementInsights.selectors.ts index f30b39bf3c68..8a66abaa7d21 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/insights/statementInsights/statementInsights.selectors.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/insights/statementInsights/statementInsights.selectors.ts @@ -46,7 +46,7 @@ export const selectColumns = createSelector( localStorageSelector, localStorage => localStorage["showColumns/StatementInsightsPage"] - ? localStorage["showColumns/StatementInsightsPage"].split(",") + ? localStorage["showColumns/StatementInsightsPage"]?.split(",") : null, ); diff --git a/pkg/ui/workspaces/cluster-ui/src/store/jobs/jobs.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/store/jobs/jobs.selectors.ts index f5aa7de346b8..2d5fc6288f5d 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/jobs/jobs.selectors.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/jobs/jobs.selectors.ts @@ -42,6 +42,6 @@ export const selectColumns = createSelector( // return array of columns if user have customized it or `null` otherwise localStorage => localStorage["showColumns/JobsPage"] - ? localStorage["showColumns/JobsPage"].split(",") + ? localStorage["showColumns/JobsPage"]?.split(",") : null, ); diff --git a/pkg/ui/workspaces/cluster-ui/src/store/sessions/sessions.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/store/sessions/sessions.selectors.ts index b9a4c0e2f46c..65ff4ce97e23 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/sessions/sessions.selectors.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/sessions/sessions.selectors.ts @@ -20,7 +20,7 @@ export const selectSession = createSelector( (state: AppState) => state.adminUI?.sessions, (_state: AppState, props: RouteComponentProps) => props, (state: SessionsState, props: RouteComponentProps) => { - if (!state.data) { + if (!state?.data) { return null; } const sessionID = getMatchParamByName(props.match, sessionAttr); diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.selectors.ts index 3a9fd6b740e4..b5ede47c03ba 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.selectors.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.selectors.ts @@ -43,7 +43,7 @@ export const selectTxnColumns = createSelector( // return array of columns if user have customized it or `null` otherwise localStorage => localStorage["showColumns/TransactionPage"] - ? localStorage["showColumns/TransactionPage"].split(",") + ? localStorage["showColumns/TransactionPage"]?.split(",") : null, ); diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts index 49459d7e099f..3f4684650866 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts @@ -171,8 +171,8 @@ export const filterTransactions = ( activeFilters: 0, }; const timeValue = getTimeValueInSeconds(filters); - const regions = filters.regions.length > 0 ? filters.regions.split(",") : []; - const nodes = filters.nodes.length > 0 ? filters.nodes.split(",") : []; + const regions = filters.regions?.length > 0 ? filters.regions.split(",") : []; + const nodes = filters.nodes?.length > 0 ? filters.nodes.split(",") : []; const activeFilters = calculateActiveFilters(filters); diff --git a/pkg/ui/workspaces/cluster-ui/src/util/format.ts b/pkg/ui/workspaces/cluster-ui/src/util/format.ts index a9c67ca6161f..f06c0458367b 100644 --- a/pkg/ui/workspaces/cluster-ui/src/util/format.ts +++ b/pkg/ui/workspaces/cluster-ui/src/util/format.ts @@ -282,7 +282,7 @@ function add(a: string, b: string): string { // to an int64 (in string form). export function HexStringToInt64String(s: string): string { let dec = "0"; - s.split("").forEach(function (chr: string) { + s?.split("").forEach(function (chr: string) { const n = parseInt(chr, 16); for (let t = 8; t; t >>= 1) { dec = add(dec, dec); diff --git a/pkg/ui/workspaces/cluster-ui/src/util/formatNumber.ts b/pkg/ui/workspaces/cluster-ui/src/util/formatNumber.ts index 5352f0009e8d..b106895a63d2 100644 --- a/pkg/ui/workspaces/cluster-ui/src/util/formatNumber.ts +++ b/pkg/ui/workspaces/cluster-ui/src/util/formatNumber.ts @@ -11,7 +11,7 @@ import { isNumber } from "lodash"; function numberToString(n: number) { - return n.toString(); + return n?.toString() || ""; } export function formatNumberForDisplay( diff --git a/pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts b/pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts index c030a03a8141..be75504e64fc 100644 --- a/pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts +++ b/pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts @@ -36,6 +36,9 @@ export const selectIndexStatsKeys = createSelector( ); export const KeyToTableRequest = (key: string): TableIndexStatsRequest => { + if (!key?.includes("/")) { + return new TableIndexStatsRequest({ database: "", table: "" }); + } const s = key.split("/"); const database = s[0]; const table = s[1]; diff --git a/pkg/ui/workspaces/db-console/src/selectors/recentExecutionsSelectors.ts b/pkg/ui/workspaces/db-console/src/selectors/recentExecutionsSelectors.ts index 3012a28c7cc3..1962080c3eef 100644 --- a/pkg/ui/workspaces/db-console/src/selectors/recentExecutionsSelectors.ts +++ b/pkg/ui/workspaces/db-console/src/selectors/recentExecutionsSelectors.ts @@ -61,7 +61,7 @@ export const selectRecentStatement = createSelector( export const selectAppName = createSelector( (state: AdminUIState) => state.cachedData.sessions, (state?: CachedDataReducerState) => { - if (!state.data) { + if (!state?.data) { return null; } return state.data.internal_app_name_prefix; diff --git a/pkg/ui/workspaces/db-console/src/util/highlightedText.tsx b/pkg/ui/workspaces/db-console/src/util/highlightedText.tsx index f9f8cd916885..9c547295253c 100644 --- a/pkg/ui/workspaces/db-console/src/util/highlightedText.tsx +++ b/pkg/ui/workspaces/db-console/src/util/highlightedText.tsx @@ -37,7 +37,7 @@ export default function getHighlightedText( }) .join("|"); const parts = isOriginalText - ? text.split(new RegExp(`(${search})`, "gi")) + ? text?.split(new RegExp(`(${search})`, "gi")) : rebaseText(text, highlight).split(new RegExp(`(${search})`, "gi")); const highlightClass = hasDarkBkg ? "_text-bold-light" : "_text-bold"; return parts.map((part, i) => { diff --git a/pkg/ui/workspaces/db-console/src/views/sessions/sessionDetails.tsx b/pkg/ui/workspaces/db-console/src/views/sessions/sessionDetails.tsx index 5c10b78d1780..8dfe58ab436b 100644 --- a/pkg/ui/workspaces/db-console/src/views/sessions/sessionDetails.tsx +++ b/pkg/ui/workspaces/db-console/src/views/sessions/sessionDetails.tsx @@ -39,7 +39,7 @@ export const selectSession = createSelector( state: CachedDataReducerState, props: RouteComponentProps, ) => { - if (!state.data) { + if (!state?.data) { return null; } const sessionID = getMatchParamByName(props.match, sessionAttr); diff --git a/pkg/ui/workspaces/db-console/src/views/sessions/sessionsPage.tsx b/pkg/ui/workspaces/db-console/src/views/sessions/sessionsPage.tsx index 7f19ce38e2eb..19944c1cbc53 100644 --- a/pkg/ui/workspaces/db-console/src/views/sessions/sessionsPage.tsx +++ b/pkg/ui/workspaces/db-console/src/views/sessions/sessionsPage.tsx @@ -48,7 +48,7 @@ export const selectSessions = createSelector( state: CachedDataReducerState, _: RouteComponentProps, ) => { - if (!state.data) { + if (!state?.data) { return null; } return state.data.sessions.map(session => { @@ -64,7 +64,7 @@ export const selectAppName = createSelector( state: CachedDataReducerState, _: RouteComponentProps, ) => { - if (!state.data) { + if (!state?.data) { return null; } return state.data.internal_app_name_prefix; diff --git a/pkg/ui/workspaces/db-console/src/views/statements/statementsPage.tsx b/pkg/ui/workspaces/db-console/src/views/statements/statementsPage.tsx index 44c0347d480b..34244d26c15b 100644 --- a/pkg/ui/workspaces/db-console/src/views/statements/statementsPage.tsx +++ b/pkg/ui/workspaces/db-console/src/views/statements/statementsPage.tsx @@ -113,7 +113,7 @@ export const selectStatements = createSelector( props: RouteComponentProps, diagnosticsReportsPerStatement, ): AggregateStatistics[] => { - if (!state.data || !state.valid) { + if (!state?.data || !state?.valid) { return null; } let statements = flattenStatementStats(state.data.statements); @@ -193,7 +193,7 @@ export const selectStatements = createSelector( export const selectApps = createSelector( (state: AdminUIState) => state.cachedData.statements, (state: CachedDataReducerState) => { - if (!state.data) { + if (!state?.data) { return []; } @@ -228,7 +228,7 @@ export const selectApps = createSelector( export const selectDatabases = createSelector( (state: AdminUIState) => state.cachedData.databases, (state: CachedDataReducerState) => { - if (!state.data) { + if (!state?.data) { return []; } @@ -243,7 +243,7 @@ export const selectDatabases = createSelector( export const selectTotalFingerprints = createSelector( (state: AdminUIState) => state.cachedData.statements, (state: CachedDataReducerState) => { - if (!state.data) { + if (!state?.data) { return 0; } const aggregated = aggregateStatementStats(state.data.statements); @@ -256,7 +256,7 @@ export const selectTotalFingerprints = createSelector( export const selectLastReset = createSelector( (state: AdminUIState) => state.cachedData.statements, (state: CachedDataReducerState) => { - if (!state.data) { + if (!state?.data) { return "unknown"; } return PrintTime(util.TimestampToMoment(state.data.last_reset)); diff --git a/pkg/ui/workspaces/db-console/src/views/transactions/transactionsPage.tsx b/pkg/ui/workspaces/db-console/src/views/transactions/transactionsPage.tsx index 219428954c48..a577acf7f6af 100644 --- a/pkg/ui/workspaces/db-console/src/views/transactions/transactionsPage.tsx +++ b/pkg/ui/workspaces/db-console/src/views/transactions/transactionsPage.tsx @@ -67,7 +67,7 @@ export const selectData = createSelector( export const selectLastReset = createSelector( (state: AdminUIState) => state.cachedData.transactions, (state: CachedDataReducerState) => { - if (!state.data) { + if (!state?.data) { return "unknown"; } From e4396957e5afa590144b0382e159956c9d326a49 Mon Sep 17 00:00:00 2001 From: maryliag Date: Mon, 27 Mar 2023 08:10:28 -0400 Subject: [PATCH 5/6] ui: drop index with space Previously, if the index had a space on its name, it would fail to drop. This commit adds quotes so it can be executed. Fixes #97988 Release note (bug fix): Index recommendation to DROP an index that have a space on its name can now be properly executed. --- pkg/ui/workspaces/cluster-ui/src/api/safesql.ts | 2 +- pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts | 7 ++++++- .../cluster-ui/src/databaseTablePage/databaseTablePage.tsx | 5 ++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/api/safesql.ts b/pkg/ui/workspaces/cluster-ui/src/api/safesql.ts index 3e560dbf10aa..6166c91786d7 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/safesql.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/safesql.ts @@ -83,7 +83,7 @@ export class SQL implements SQLStringer { // case sensitive when used in a query. If the input string contains a zero // byte, the result will be truncated immediately before it. // Cribbed from https://github.com/lib/pq and Typescript-ified. -function QuoteIdentifier(name: string): string { +export function QuoteIdentifier(name: string): string { // Use a search regex to replace all occurrences instead of just the first occurrence. const search = /"/g; return `"` + name.replace(search, `""`) + `"`; diff --git a/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts index 64794d20bd33..b44eadf9d7a5 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts @@ -24,6 +24,7 @@ import { recommendDropUnusedIndex, } from "../insights"; import { HexStringToInt64String } from "../util"; +import { QuoteIdentifier } from "./safesql"; // Export for db-console import from clusterUiApi. export type { InsightRecommendation } from "../insights"; @@ -72,7 +73,11 @@ function clusterIndexUsageStatsToSchemaInsight( results[key] = { type: "DropIndex", database: row.database_name, - query: `DROP INDEX ${row.schema_name}.${row.table_name}@${row.index_name};`, + query: `DROP INDEX ${QuoteIdentifier( + row.schema_name, + )}.${QuoteIdentifier(row.table_name)}@${QuoteIdentifier( + row.index_name, + )};`, indexDetails: { table: row.table_name, indexID: row.index_id, diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx index 596b07bebe99..4b0bef8dad98 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx @@ -60,6 +60,7 @@ import { RecommendationType } from "../indexDetailsPage"; import LoadingError from "../sqlActivity/errorComponent"; import { Loading } from "../loading"; import { UIConfigState } from "../store"; +import { QuoteIdentifier } from "../api/safesql"; const cx = classNames.bind(styles); const booleanSettingCx = classnames.bind(booleanSettingStyles); @@ -375,7 +376,9 @@ export class DatabaseTablePage extends React.Component< const query = indexStat.indexRecommendations.map(recommendation => { switch (recommendation.type) { case "DROP_UNUSED": - return `DROP INDEX ${this.props.name}@${indexStat.indexName};`; + return `DROP INDEX ${QuoteIdentifier( + this.props.name, + )}@${QuoteIdentifier(indexStat.indexName)};`; } }); if (query.length === 0) { From 545d49a03efd3765b4ac1b5dbb49ff7b79bf7a56 Mon Sep 17 00:00:00 2001 From: Nick Travers Date: Wed, 29 Mar 2023 11:35:19 -0700 Subject: [PATCH 6/6] roachtest: use local SSDs for disk-stall failover tests The disk-stalled roachtests were updated in #99747 to use PDs in favor of local SSDs. This change broke the `failover/*/disk-stall` tests, which look for `/dev/sdb` on GCE (the device name used for GCE Persistent Disks), but the tests still create clusters with local SSDs (the roachtest default). Fix #99902. Fix #99926. Fix #99930. Touches #97968. Release note: None. --- pkg/cmd/roachtest/tests/disk_stall.go | 2 ++ pkg/cmd/roachtest/tests/failover.go | 15 ++++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/roachtest/tests/disk_stall.go b/pkg/cmd/roachtest/tests/disk_stall.go index 0c0918c986e0..3318f017be91 100644 --- a/pkg/cmd/roachtest/tests/disk_stall.go +++ b/pkg/cmd/roachtest/tests/disk_stall.go @@ -47,6 +47,8 @@ func registerDiskStalledDetection(r registry.Registry) { } makeSpec := func() spec.ClusterSpec { s := r.MakeClusterSpec(4, spec.ReuseNone()) + // Use PDs in an attempt to work around flakes encountered when using SSDs. + // See #97968. s.PreferLocalSSD = false return s } diff --git a/pkg/cmd/roachtest/tests/failover.go b/pkg/cmd/roachtest/tests/failover.go index 5181a6e56552..5927035bccfd 100644 --- a/pkg/cmd/roachtest/tests/failover.go +++ b/pkg/cmd/roachtest/tests/failover.go @@ -45,11 +45,20 @@ func registerFailover(r registry.Registry) { failureModeDiskStall, } { failureMode := failureMode // pin loop variable + makeSpec := func(nNodes, nCPU int) spec.ClusterSpec { + s := r.MakeClusterSpec(nNodes, spec.CPU(nCPU)) + if failureMode == failureModeDiskStall { + // Use PDs in an attempt to work around flakes encountered when using + // SSDs. See #97968. + s.PreferLocalSSD = false + } + return s + } r.Add(registry.TestSpec{ Name: fmt.Sprintf("failover/non-system/%s", failureMode), Owner: registry.OwnerKV, Timeout: 30 * time.Minute, - Cluster: r.MakeClusterSpec(7, spec.CPU(4)), + Cluster: makeSpec(7 /* nodes */, 4 /* cpus */), Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { runFailoverNonSystem(ctx, t, c, failureMode) }, @@ -58,7 +67,7 @@ func registerFailover(r registry.Registry) { Name: fmt.Sprintf("failover/liveness/%s", failureMode), Owner: registry.OwnerKV, Timeout: 30 * time.Minute, - Cluster: r.MakeClusterSpec(5, spec.CPU(4)), + Cluster: makeSpec(5 /* nodes */, 4 /* cpus */), Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { runFailoverLiveness(ctx, t, c, failureMode) }, @@ -67,7 +76,7 @@ func registerFailover(r registry.Registry) { Name: fmt.Sprintf("failover/system-non-liveness/%s", failureMode), Owner: registry.OwnerKV, Timeout: 30 * time.Minute, - Cluster: r.MakeClusterSpec(7, spec.CPU(4)), + Cluster: makeSpec(7 /* nodes */, 4 /* cpus */), Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { runFailoverSystemNonLiveness(ctx, t, c, failureMode) },