From a81c820d2d147e0563ab4d9afd3ceba2d2828da1 Mon Sep 17 00:00:00 2001 From: Marylia Gutierrez Date: Thu, 10 Mar 2022 13:40:12 -0500 Subject: [PATCH 1/4] ui: new plan table on statement details Previously, the Explain Plan tab on Statement Details was showing only one plan. This commit introduces a table of plan with their respective executions stats. When a plan is clicked on the table, it shows the Plan and its statistics. Fixes #72129 Release justification: Category 4 Release note (ui change): Explain Plan tab on Statement Details shows statistics for all the plans executed by the selected statement on the selected period. --- .../src/sql/sqlhighlight.module.scss | 2 +- .../src/statementDetails/planDetails/index.ts | 11 ++ .../planDetails/planDetails.tsx | 76 ++++++++++ .../planDetails/plansTable.tsx | 140 ++++++++++++++++++ .../statementDetails.fixture.ts | 98 +++++++++++- .../src/statementDetails/statementDetails.tsx | 21 ++- .../src/statsTableUtil/statsTableUtil.tsx | 1 - 7 files changed, 334 insertions(+), 15 deletions(-) create mode 100644 pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/index.ts create mode 100644 pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx create mode 100644 pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/plansTable.tsx diff --git a/pkg/ui/workspaces/cluster-ui/src/sql/sqlhighlight.module.scss b/pkg/ui/workspaces/cluster-ui/src/sql/sqlhighlight.module.scss index 820715c912d6..cffe2188c45b 100644 --- a/pkg/ui/workspaces/cluster-ui/src/sql/sqlhighlight.module.scss +++ b/pkg/ui/workspaces/cluster-ui/src/sql/sqlhighlight.module.scss @@ -27,7 +27,7 @@ font-family: SFMono-Semibold; font-size: 14px; line-height: 1.57; - white-space: pre-line; + white-space: pre; word-wrap: break-word; span { diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/index.ts b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/index.ts new file mode 100644 index 000000000000..36a26ec6363c --- /dev/null +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/index.ts @@ -0,0 +1,11 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +export * from "./planDetails"; diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx new file mode 100644 index 000000000000..bc88de2f978d --- /dev/null +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx @@ -0,0 +1,76 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +import React, { useState } from "react"; +import { Helmet } from "react-helmet"; +import { ArrowLeft } from "@cockroachlabs/icons"; +import { + PlansSortedTable, + makeExplainPlanColumns, + PlanHashStats, +} from "./plansTable"; +import { Button } from "../../button"; +import { SqlBox } from "../../sql"; + +interface PlanDetailsProps { + plans: PlanHashStats[]; +} + +export function PlanDetails({ plans }: PlanDetailsProps): React.ReactElement { + const [plan, setPlan] = useState(null); + const handleDetails = (plan: PlanHashStats): void => { + setPlan(plan); + }; + const backToPlanTable = (): void => { + setPlan(null); + }; + + if (plan) { + return renderExplainPlan(plan, backToPlanTable); + } else { + return renderPlanTable(plans, handleDetails); + } +} + +function renderPlanTable( + plans: PlanHashStats[], + handleDetails: (plan: PlanHashStats) => void, +): React.ReactElement { + const columns = makeExplainPlanColumns(handleDetails); + return ( + + ); +} + +function renderExplainPlan( + plan: PlanHashStats, + backToPlanTable: () => void, +): React.ReactElement { + return ( +
+ + + +
+ ); +} diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/plansTable.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/plansTable.tsx new file mode 100644 index 000000000000..f4a4eda2805d --- /dev/null +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/plansTable.tsx @@ -0,0 +1,140 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +import React from "react"; +import { ColumnDescriptor, SortedTable } from "src/sortedtable"; +import { Tooltip } from "@cockroachlabs/ui-components"; +import { cockroach } from "@cockroachlabs/crdb-protobuf-client"; +import { + Duration, + formatNumberForDisplay, + longToInt, + TimestampToMoment, +} from "../../util"; + +export type PlanHashStats = cockroach.server.serverpb.StatementDetailsResponse.ICollectedStatementGroupedByPlanHash; +export class PlansSortedTable extends SortedTable {} + +const planDetailsColumnLabels = { + planID: "Plan ID", + lastExecTime: "Last Execution Time", + avgExecTime: "Average Execution Time", + execCount: "Execution Count", + avgRowsRead: "Average Rows Read", +}; +export type PlanDetailsTableColumnKeys = keyof typeof planDetailsColumnLabels; + +type PlanDetailsTableTitleType = { + [key in PlanDetailsTableColumnKeys]: () => JSX.Element; +}; + +export const planDetailsTableTitles: PlanDetailsTableTitleType = { + planID: () => { + return ( + + {planDetailsColumnLabels.planID} + + ); + }, + lastExecTime: () => { + return ( + + {planDetailsColumnLabels.lastExecTime} + + ); + }, + avgExecTime: () => { + return ( + + {planDetailsColumnLabels.avgExecTime} + + ); + }, + execCount: () => { + return ( + + {planDetailsColumnLabels.execCount} + + ); + }, + avgRowsRead: () => { + return ( + + {planDetailsColumnLabels.avgRowsRead} + + ); + }, +}; + +export function makeExplainPlanColumns( + handleDetails: (plan: PlanHashStats) => void, +): ColumnDescriptor[] { + const duration = (v: number) => Duration(v * 1e9); + return [ + { + name: "planID", + title: planDetailsTableTitles.planID(), + cell: (item: PlanHashStats) => ( + handleDetails(item)}>{longToInt(item.plan_hash)} + ), + sort: (item: PlanHashStats) => longToInt(item.plan_hash), + alwaysShow: true, + }, + { + name: "lastExecTime", + title: planDetailsTableTitles.lastExecTime(), + cell: (item: PlanHashStats) => + TimestampToMoment(item.stats.last_exec_timestamp).format( + "MMM DD, YYYY HH:MM", + ), + sort: (item: PlanHashStats) => + TimestampToMoment(item.stats.last_exec_timestamp).unix(), + }, + { + name: "avgExecTime", + title: planDetailsTableTitles.avgExecTime(), + cell: (item: PlanHashStats) => + formatNumberForDisplay(item.stats.run_lat.mean, duration), + sort: (item: PlanHashStats) => item.stats.run_lat.mean, + }, + { + name: "execCount", + title: planDetailsTableTitles.execCount(), + cell: (item: PlanHashStats) => longToInt(item.stats.count), + sort: (item: PlanHashStats) => longToInt(item.stats.count), + }, + { + name: "avg_rows_read", + title: planDetailsTableTitles.avgRowsRead(), + cell: (item: PlanHashStats) => longToInt(item.stats.rows_read.mean), + sort: (item: PlanHashStats) => longToInt(item.stats.rows_read.mean), + }, + ]; +} diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts index 8254f7a9eced..4e70452dbdcf 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts @@ -536,8 +536,8 @@ export const getStatementDetailsPropsFixture = (): StatementDetailsProps => ({ statements_per_plan_hash: [ { stats: { - count: new Long(5), - first_attempt_count: new Long(5), + count: new Long(3), + first_attempt_count: new Long(3), max_retries: new Long(0), legacy_last_err: "", legacy_last_err_redacted: "", @@ -628,6 +628,100 @@ export const getStatementDetailsPropsFixture = (): StatementDetailsProps => ({ explain_plan: "• virtual table\n table: @primary", plan_hash: new Long(14192395335876201826), }, + { + stats: { + count: new Long(2), + first_attempt_count: new Long(2), + max_retries: new Long(0), + legacy_last_err: "", + legacy_last_err_redacted: "", + num_rows: { + mean: 6, + squared_diffs: 0, + }, + parse_lat: { + mean: 0.0000876, + squared_diffs: 2.35792e-8, + }, + plan_lat: { + mean: 0.008131, + squared_diffs: 0.00127640837, + }, + run_lat: { + mean: 0.0002796, + squared_diffs: 2.401919999999999e-8, + }, + service_lat: { + mean: 0.008522, + squared_diffs: 0.001298238058, + }, + overhead_lat: { + mean: 0.000023799999999999972, + squared_diffs: 5.492799999999973e-9, + }, + sensitive_info: { + last_err: "", + most_recent_plan_description: { + name: "virtual table", + attrs: [ + { + key: "Table", + value: "node_build_info@primary", + }, + ], + children: [], + }, + most_recent_plan_timestamp: { + seconds: new Long(1614851546), + nanos: 956814000, + }, + }, + bytes_read: { + mean: 0, + squared_diffs: 0, + }, + rows_read: { + mean: 0, + squared_diffs: 0, + }, + rows_written: { + mean: 0, + squared_diffs: 0, + }, + exec_stats: { + count: new Long(5), + network_bytes: { + mean: 0, + squared_diffs: 0, + }, + max_mem_usage: { + mean: 10240, + squared_diffs: 0, + }, + contention_time: { + mean: 0, + squared_diffs: 0, + }, + network_messages: { + mean: 0, + squared_diffs: 0, + }, + max_disk_usage: { + mean: 0, + squared_diffs: 0, + }, + }, + sql_type: "TypeDML", + last_exec_timestamp: { + seconds: Long.fromInt(1599670292), + nanos: 111613000, + }, + nodes: [new Long(1)], + plan_gists: ["Ah0GAg=="], + }, + explain_plan: "• virtual table\n table: @primary\nFULL SCAN", + plan_hash: new Long(14192395335876212345), + }, ], internal_app_name_prefix: "$ internal", toJSON: () => ({}), diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx index 83967cec098c..25e77855dec3 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx @@ -41,7 +41,7 @@ import { Button } from "src/button"; import { SqlBox } from "src/sql"; import { SortSetting } from "src/sortedtable"; import { Tooltip } from "@cockroachlabs/ui-components"; -import { PlanView } from "./planView"; +import { PlanDetails } from "./planDetails"; import { SummaryCard } from "src/summaryCard"; import { latencyBreakdown, @@ -444,6 +444,7 @@ export class StatementDetails extends React.Component< hasViewActivityRedactedRole, } = this.props; const { currentTab } = this.state; + const { statements_per_plan_hash } = this.props.statementDetails; const { stats, app_names, @@ -500,9 +501,7 @@ export class StatementDetails extends React.Component< const regions = unique( (stats.nodes || []).map(node => nodeRegions[node.toString()]), ).sort(); - const explainPlan = - stats.sensitive_info && stats.sensitive_info.most_recent_plan_description; - const explainGlobalProps = { distribution: distSQL, vectorized: vec }; + const duration = (v: number) => Duration(v * 1e9); const hasDiagnosticReports = diagnosticsReports.length > 0; const lastExec = @@ -776,13 +775,13 @@ export class StatementDetails extends React.Component< )} - - - + + + + + +

+ Date: Thu, 3 Mar 2022 14:35:37 -0500 Subject: [PATCH 2/4] spanconfig: add Record constructor and validation This change adds a constructor method that returns a new `spanconfig.Record` and conditionally performs some validation if the target is a SystemTarget. Informs: #73727 Release note: None Release justification: low-risk updates to new functionality --- .../seed_tenant_span_configs_external_test.go | 8 +- .../datadriven_test.go | 14 +- .../datadriven_test.go | 20 +-- pkg/ccl/testccl/sqlccl/tenant_gc_test.go | 9 +- pkg/kv/kvserver/client_spanconfigs_test.go | 9 +- .../migrations/seed_tenant_span_configs.go | 10 +- pkg/roachpb/span_config.go | 45 +++++++ pkg/roachpb/span_config.proto | 4 + pkg/spanconfig/BUILD.bazel | 2 + pkg/spanconfig/record.go | 52 ++++++++ pkg/spanconfig/record_test.go | 122 ++++++++++++++++++ pkg/spanconfig/spanconfig.go | 44 +++---- .../spanconfigkvaccessor/kvaccessor.go | 22 ++-- .../spanconfigkvaccessor/validation_test.go | 79 +++++------- .../spanconfigkvsubscriber/kvsubscriber.go | 2 +- .../spanconfigdecoder.go | 13 +- .../spanconfigdecoder_test.go | 14 +- .../spanconfigreconciler/reconciler.go | 27 +++- .../spanconfigsqltranslator/sqltranslator.go | 118 +++++++++-------- .../spanconfigstore/spanconfigstore.go | 47 ++++--- .../spanconfigstore/spanconfigstore_test.go | 30 +++-- pkg/spanconfig/spanconfigstore/store.go | 23 ++-- pkg/spanconfig/spanconfigstore/store_test.go | 35 ++--- .../spanconfigstore/systemspanconfigstore.go | 35 ++--- .../systemspanconfigstore_test.go | 17 ++- .../spanconfigtestutils/recorder.go | 24 ++-- pkg/spanconfig/spanconfigtestutils/utils.go | 14 +- pkg/spanconfig/target.go | 13 +- pkg/sql/tenant.go | 17 ++- 29 files changed, 568 insertions(+), 301 deletions(-) create mode 100644 pkg/spanconfig/record.go create mode 100644 pkg/spanconfig/record_test.go diff --git a/pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go b/pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go index 1138c8d8f08b..daa712827d67 100644 --- a/pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go +++ b/pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go @@ -75,7 +75,7 @@ func TestPreSeedSpanConfigsWrittenWhenActive(t *testing.T) { }) require.NoError(t, err) require.Len(t, records, 1) - require.Equal(t, records[0].Target.GetSpan(), tenantSeedSpan) + require.Equal(t, records[0].GetTarget().GetSpan(), tenantSeedSpan) } } @@ -144,7 +144,7 @@ func TestSeedTenantSpanConfigs(t *testing.T) { }) require.NoError(t, err) require.Len(t, records, 1) - require.Equal(t, records[0].Target.GetSpan(), tenantSeedSpan) + require.Equal(t, records[0].GetTarget().GetSpan(), tenantSeedSpan) } } @@ -200,7 +200,7 @@ func TestSeedTenantSpanConfigsWithExistingEntry(t *testing.T) { }) require.NoError(t, err) require.Len(t, records, 1) - require.Equal(t, records[0].Target.GetSpan(), tenantSeedSpan) + require.Equal(t, records[0].GetTarget().GetSpan(), tenantSeedSpan) } // Ensure the cluster version bump goes through successfully. @@ -215,6 +215,6 @@ func TestSeedTenantSpanConfigsWithExistingEntry(t *testing.T) { }) require.NoError(t, err) require.Len(t, records, 1) - require.Equal(t, records[0].Target.GetSpan(), tenantSeedSpan) + require.Equal(t, records[0].GetTarget().GetSpan(), tenantSeedSpan) } } diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go index d0671ba0ddc2..b27e3906ab80 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go @@ -208,18 +208,18 @@ func TestDataDriven(t *testing.T) { ) require.NoError(t, err) sort.Slice(records, func(i, j int) bool { - return records[i].Target.Less(records[j].Target) + return records[i].GetTarget().Less(records[j].GetTarget()) }) lines := make([]string, len(records)) for i, record := range records { switch { - case record.Target.IsSpanTarget(): - lines[i] = fmt.Sprintf("%-42s %s", record.Target.GetSpan(), - spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.Config)) - case record.Target.IsSystemTarget(): - lines[i] = fmt.Sprintf("%-42s %s", record.Target.GetSystemTarget(), - spanconfigtestutils.PrintSystemSpanConfigDiffedAgainstDefault(record.Config)) + case record.GetTarget().IsSpanTarget(): + lines[i] = fmt.Sprintf("%-42s %s", record.GetTarget().GetSpan(), + spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.GetConfig())) + case record.GetTarget().IsSystemTarget(): + lines[i] = fmt.Sprintf("%-42s %s", record.GetTarget().GetSystemTarget(), + spanconfigtestutils.PrintSystemSpanConfigDiffedAgainstDefault(record.GetConfig())) default: panic("unsupported target type") } diff --git a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go index d8c65cbeda58..9a5d91a2254c 100644 --- a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go +++ b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go @@ -161,18 +161,18 @@ func TestDataDriven(t *testing.T) { records, _, err := sqlTranslator.Translate(ctx, descIDs, generateSystemSpanConfigs) require.NoError(t, err) sort.Slice(records, func(i, j int) bool { - return records[i].Target.Less(records[j].Target) + return records[i].GetTarget().Less(records[j].GetTarget()) }) var output strings.Builder for _, record := range records { switch { - case record.Target.IsSpanTarget(): - output.WriteString(fmt.Sprintf("%-42s %s\n", record.Target.GetSpan(), - spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.Config))) - case record.Target.IsSystemTarget(): - output.WriteString(fmt.Sprintf("%-42s %s\n", record.Target.GetSystemTarget(), - spanconfigtestutils.PrintSystemSpanConfigDiffedAgainstDefault(record.Config))) + case record.GetTarget().IsSpanTarget(): + output.WriteString(fmt.Sprintf("%-42s %s\n", record.GetTarget().GetSpan(), + spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.GetConfig()))) + case record.GetTarget().IsSystemTarget(): + output.WriteString(fmt.Sprintf("%-42s %s\n", record.GetTarget().GetSystemTarget(), + spanconfigtestutils.PrintSystemSpanConfigDiffedAgainstDefault(record.GetConfig()))) default: panic("unsupported target type") } @@ -185,12 +185,12 @@ func TestDataDriven(t *testing.T) { require.NoError(t, err) sort.Slice(records, func(i, j int) bool { - return records[i].Target.Less(records[j].Target) + return records[i].GetTarget().Less(records[j].GetTarget()) }) var output strings.Builder for _, record := range records { - output.WriteString(fmt.Sprintf("%-42s %s\n", record.Target.GetSpan(), - spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.Config))) + output.WriteString(fmt.Sprintf("%-42s %s\n", record.GetTarget().GetSpan(), + spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.GetConfig()))) } return output.String() diff --git a/pkg/ccl/testccl/sqlccl/tenant_gc_test.go b/pkg/ccl/testccl/sqlccl/tenant_gc_test.go index 18c474d6e5f6..492d0741ab52 100644 --- a/pkg/ccl/testccl/sqlccl/tenant_gc_test.go +++ b/pkg/ccl/testccl/sqlccl/tenant_gc_test.go @@ -70,12 +70,9 @@ func TestGCTenantRemovesSpanConfigs(t *testing.T) { // keyspace. systemTarget, err := spanconfig.MakeTenantKeyspaceTarget(tenantID, tenantID) require.NoError(t, err) - err = tenantKVAccessor.UpdateSpanConfigRecords(ctx, nil /* toDelete */, []spanconfig.Record{ - { - Target: spanconfig.MakeTargetFromSystemTarget(systemTarget), - Config: roachpb.SpanConfig{}, // Doesn't matter - }, - }) + rec, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSystemTarget(systemTarget), roachpb.SpanConfig{}) + require.NoError(t, err) + err = tenantKVAccessor.UpdateSpanConfigRecords(ctx, nil /* toDelete */, []spanconfig.Record{rec}) require.NoError(t, err) // Ensure there are 2 configs for the tenant -- one that spans its entire diff --git a/pkg/kv/kvserver/client_spanconfigs_test.go b/pkg/kv/kvserver/client_spanconfigs_test.go index 3f46b364dfbf..bc8db609e381 100644 --- a/pkg/kv/kvserver/client_spanconfigs_test.go +++ b/pkg/kv/kvserver/client_spanconfigs_test.go @@ -70,15 +70,18 @@ func TestSpanConfigUpdateAppliedToReplica(t *testing.T) { span := repl.Desc().RSpan().AsRawSpanWithNoLocals() conf := roachpb.SpanConfig{NumReplicas: 5, NumVoters: 3} + add, err := spanconfig.Addition(spanconfig.MakeTargetFromSpan(span), conf) + require.NoError(t, err) deleted, added := spanConfigStore.Apply( ctx, false, /* dryrun */ - spanconfig.Addition(spanconfig.MakeTargetFromSpan(span), conf), + add, ) require.Empty(t, deleted) require.Len(t, added, 1) - require.True(t, added[0].Target.GetSpan().Equal(span)) - require.True(t, added[0].Config.Equal(conf)) + require.True(t, added[0].GetTarget().GetSpan().Equal(span)) + addedCfg := added[0].GetConfig() + require.True(t, addedCfg.Equal(conf)) require.NotNil(t, mockSubscriber.callback) mockSubscriber.callback(ctx, span) // invoke the callback diff --git a/pkg/migration/migrations/seed_tenant_span_configs.go b/pkg/migration/migrations/seed_tenant_span_configs.go index 3010ac30bacf..53c91a7bb91e 100644 --- a/pkg/migration/migrations/seed_tenant_span_configs.go +++ b/pkg/migration/migrations/seed_tenant_span_configs.go @@ -67,12 +67,12 @@ func seedTenantSpanConfigsMigration( Key: tenantPrefix, EndKey: tenantPrefix.Next(), } - toUpsert := []spanconfig.Record{ - { - Target: spanconfig.MakeTargetFromSpan(tenantSeedSpan), - Config: tenantSpanConfig, - }, + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(tenantSeedSpan), + tenantSpanConfig) + if err != nil { + return err } + toUpsert := []spanconfig.Record{record} scRecords, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{tenantTarget}) if err != nil { return err diff --git a/pkg/roachpb/span_config.go b/pkg/roachpb/span_config.go index 375a8c5fa910..40c0be7ee2ef 100644 --- a/pkg/roachpb/span_config.go +++ b/pkg/roachpb/span_config.go @@ -14,6 +14,8 @@ import ( "fmt" "strings" "time" + + "github.com/cockroachdb/errors" ) // StoreMatchesConstraint returns whether a store's attributes or node's @@ -50,6 +52,49 @@ func (s *SpanConfig) TTL() time.Duration { return time.Duration(s.GCPolicy.TTLSeconds) * time.Second } +// ValidateSystemTargetSpanConfig ensures that only protection policies +// (GCPolicy.ProtectionPolicies) field is set on the underlying +// roachpb.SpanConfig. +func (s *SpanConfig) ValidateSystemTargetSpanConfig() error { + if s.RangeMinBytes != 0 { + return errors.AssertionFailedf("RangeMinBytes set on system span config") + } + if s.RangeMaxBytes != 0 { + return errors.AssertionFailedf("RangeMaxBytes set on system span config") + } + if s.GCPolicy.TTLSeconds != 0 { + return errors.AssertionFailedf("TTLSeconds set on system span config") + } + if s.GCPolicy.IgnoreStrictEnforcement { + return errors.AssertionFailedf("IgnoreStrictEnforcement set on system span config") + } + if s.GlobalReads { + return errors.AssertionFailedf("GlobalReads set on system span config") + } + if s.NumReplicas != 0 { + return errors.AssertionFailedf("NumReplicas set on system span config") + } + if s.NumVoters != 0 { + return errors.AssertionFailedf("NumVoters set on system span config") + } + if len(s.Constraints) != 0 { + return errors.AssertionFailedf("Constraints set on system span config") + } + if len(s.VoterConstraints) != 0 { + return errors.AssertionFailedf("VoterConstraints set on system span config") + } + if len(s.LeasePreferences) != 0 { + return errors.AssertionFailedf("LeasePreferences set on system span config") + } + if s.RangefeedEnabled { + return errors.AssertionFailedf("RangefeedEnabled set on system span config") + } + if s.ExcludeDataFromBackup { + return errors.AssertionFailedf("ExcludeDataFromBackup set on system span config") + } + return nil +} + // GetNumVoters returns the number of voting replicas as defined in the // span config. // TODO(arul): We can get rid of this now that we're correctly populating diff --git a/pkg/roachpb/span_config.proto b/pkg/roachpb/span_config.proto index ab01ca3db20d..d0aad4d66057 100644 --- a/pkg/roachpb/span_config.proto +++ b/pkg/roachpb/span_config.proto @@ -185,6 +185,10 @@ message SpanConfig { bool exclude_data_from_backup = 11; // Next ID: 12 + // + // When adding a field, also add a check a to `ValidateSystemTargetSpanConfig` + // if it is not expected to be set on a SpanConfig corresponding to a + // SystemTarget. } // SystemSpanConfigTarget specifies the target of system span configurations. diff --git a/pkg/spanconfig/BUILD.bazel b/pkg/spanconfig/BUILD.bazel index 47308eba01a2..19b4a19b8598 100644 --- a/pkg/spanconfig/BUILD.bazel +++ b/pkg/spanconfig/BUILD.bazel @@ -4,6 +4,7 @@ go_library( name = "spanconfig", srcs = [ "protectedts_state_reader.go", + "record.go", "spanconfig.go", "systemtarget.go", "target.go", @@ -30,6 +31,7 @@ go_test( name = "spanconfig_test", srcs = [ "protectedts_state_reader_test.go", + "record_test.go", "target_test.go", ], embed = [":spanconfig"], diff --git a/pkg/spanconfig/record.go b/pkg/spanconfig/record.go new file mode 100644 index 000000000000..a011d1c10b1e --- /dev/null +++ b/pkg/spanconfig/record.go @@ -0,0 +1,52 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package spanconfig + +import ( + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/errors" +) + +// Record ties a target to its corresponding config. +type Record struct { + // target specifies the target (keyspan(s)) the config applies over. + target Target + + // config is the set of attributes that apply over the corresponding target. + config roachpb.SpanConfig +} + +// MakeRecord returns a Record with the specified Target and SpanConfig. If the +// Record targets a SystemTarget, we also validate the SpanConfig. +func MakeRecord(target Target, cfg roachpb.SpanConfig) (Record, error) { + if target.IsSystemTarget() { + if err := cfg.ValidateSystemTargetSpanConfig(); err != nil { + return Record{}, + errors.NewAssertionErrorWithWrappedErrf(err, "failed to validate SystemTarget SpanConfig") + } + } + return Record{target: target, config: cfg}, nil +} + +// IsEmpty returns true if the receiver is an empty Record. +func (r *Record) IsEmpty() bool { + return r.target.isEmpty() && r.config.IsEmpty() +} + +// GetTarget returns the Record target. +func (r *Record) GetTarget() Target { + return r.target +} + +// GetConfig returns the Record config. +func (r *Record) GetConfig() roachpb.SpanConfig { + return r.config +} diff --git a/pkg/spanconfig/record_test.go b/pkg/spanconfig/record_test.go new file mode 100644 index 000000000000..6a58afd55d90 --- /dev/null +++ b/pkg/spanconfig/record_test.go @@ -0,0 +1,122 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package spanconfig + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/testutils" +) + +// TestRecordSystemTargetValidation checks that a Record with SystemTarget is +// validated on construction. +func TestRecordSystemTargetValidation(t *testing.T) { + for _, tc := range []struct { + name string + fn func(scfg *roachpb.SpanConfig) + expectedErr string + }{ + { + "range-min-bytes", + func(scfg *roachpb.SpanConfig) { + scfg.RangeMinBytes = 1 + }, + "RangeMinBytes set on system span config", + }, + { + "range-max-bytes", + func(scfg *roachpb.SpanConfig) { + scfg.RangeMaxBytes = 1 + }, + "RangeMaxBytes set on system span config", + }, + { + "gcttl", + func(scfg *roachpb.SpanConfig) { + scfg.GCPolicy.TTLSeconds = 1 + }, + "TTLSeconds set on system span config", + }, + { + "ignore-strict-gc", + func(scfg *roachpb.SpanConfig) { + scfg.GCPolicy.IgnoreStrictEnforcement = true + }, + "IgnoreStrictEnforcement set on system span config", + }, + { + "global-reads", + func(scfg *roachpb.SpanConfig) { + scfg.GlobalReads = true + }, + "GlobalReads set on system span config", + }, + { + "num-replicas", + func(scfg *roachpb.SpanConfig) { + scfg.NumReplicas = 1 + }, + "NumReplicas set on system span config", + }, + { + "num-voters", + func(scfg *roachpb.SpanConfig) { + scfg.NumVoters = 1 + }, + "NumVoters set on system span config", + }, + { + "constraints", + func(scfg *roachpb.SpanConfig) { + scfg.Constraints = append(scfg.Constraints, roachpb.ConstraintsConjunction{}) + }, + "Constraints set on system span config", + }, + { + "voter-constraints", + func(scfg *roachpb.SpanConfig) { + scfg.VoterConstraints = append(scfg.VoterConstraints, roachpb.ConstraintsConjunction{}) + }, + "VoterConstraints set on system span config", + }, + { + "lease-preferences", + func(scfg *roachpb.SpanConfig) { + scfg.LeasePreferences = append(scfg.LeasePreferences, roachpb.LeasePreference{}) + }, + "LeasePreferences set on system span config", + }, + { + "rangefeed-enabled", + func(scfg *roachpb.SpanConfig) { + scfg.RangefeedEnabled = true + }, + "RangefeedEnabled set on system span config", + }, + { + "exclude-data-from-backup", + func(scfg *roachpb.SpanConfig) { + scfg.ExcludeDataFromBackup = true + }, + "ExcludeDataFromBackup set on system span config", + }, + } { + t.Run(tc.name, func(t *testing.T) { + emptyScfg := roachpb.SpanConfig{} + systemTarget := TestingMakeTenantKeyspaceTargetOrFatal(t, roachpb.MakeTenantID(2), + roachpb.MakeTenantID(2)) + target := MakeTargetFromSystemTarget(systemTarget) + _, err := MakeRecord(target, emptyScfg) + testutils.IsError(err, tc.expectedErr) + }) + } +} diff --git a/pkg/spanconfig/spanconfig.go b/pkg/spanconfig/spanconfig.go index 757e7c009d1b..40d90a806572 100644 --- a/pkg/spanconfig/spanconfig.go +++ b/pkg/spanconfig/spanconfig.go @@ -281,20 +281,6 @@ type Splitter interface { Splits(ctx context.Context, table catalog.TableDescriptor) (int, error) } -// Record ties a target to its corresponding config. -type Record struct { - // Target specifies the target (keyspan(s)) the config applies over. - Target Target - - // Config is the set of attributes that apply over the corresponding target. - Config roachpb.SpanConfig -} - -// IsEmpty returns true if the receiver is an empty Record. -func (r *Record) IsEmpty() bool { - return r.Target.isEmpty() && r.Config.IsEmpty() -} - // SQLUpdate captures either a descriptor or a protected timestamp update. // It is the unit emitted by the SQLWatcher. type SQLUpdate struct { @@ -386,26 +372,28 @@ type Update Record // Deletion constructs an update that represents a deletion over the given // target. -func Deletion(target Target) Update { - return Update{ - Target: target, - Config: roachpb.SpanConfig{}, // delete +func Deletion(target Target) (Update, error) { + record, err := MakeRecord(target, roachpb.SpanConfig{}) // delete + if err != nil { + return Update{}, err } + return Update(record), nil } // Addition constructs an update that represents adding the given config over // the given target. -func Addition(target Target, conf roachpb.SpanConfig) Update { - return Update{ - Target: target, - Config: conf, +func Addition(target Target, conf roachpb.SpanConfig) (Update, error) { + record, err := MakeRecord(target, conf) + if err != nil { + return Update{}, err } + return Update(record), nil } // Deletion returns true if the update corresponds to a span config being // deleted. func (u Update) Deletion() bool { - return u.Config.IsEmpty() + return u.config.IsEmpty() } // Addition returns true if the update corresponds to a span config being added. @@ -413,6 +401,16 @@ func (u Update) Addition() bool { return !u.Deletion() } +// GetTarget returns the underlying spanconfig.Record target. +func (u Update) GetTarget() Target { + return u.target +} + +// GetConfig returns the underlying spanconfig.Record config. +func (u Update) GetConfig() roachpb.SpanConfig { + return u.config +} + // ProtectedTSReader is the read-only portion for querying protected // timestamp information. It doubles up as an adaptor interface for // protectedts.Cache. diff --git a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go index a1f725bc8bb2..7ea6bbe8c645 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go +++ b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go @@ -105,10 +105,11 @@ func (k *KVAccessor) GetSpanConfigRecords( return nil, err } - records = append(records, spanconfig.Record{ - Target: spanconfig.DecodeTarget(span), - Config: conf, - }) + record, err := spanconfig.MakeRecord(spanconfig.DecodeTarget(span), conf) + if err != nil { + return nil, err + } + records = append(records, record) } if err != nil { return nil, err @@ -322,14 +323,15 @@ func (k *KVAccessor) constructUpsertStmtAndArgs( upsertValues := make([]string, len(toUpsert)) upsertQueryArgs := make([]interface{}, len(toUpsert)*3) for i, record := range toUpsert { - marshaled, err := protoutil.Marshal(&record.Config) + cfg := record.GetConfig() + marshaled, err := protoutil.Marshal(&cfg) if err != nil { return "", nil, err } startKeyIdx, endKeyIdx, configIdx := i*3, (i*3)+1, (i*3)+2 - upsertQueryArgs[startKeyIdx] = record.Target.Encode().Key - upsertQueryArgs[endKeyIdx] = record.Target.Encode().EndKey + upsertQueryArgs[startKeyIdx] = record.GetTarget().Encode().Key + upsertQueryArgs[endKeyIdx] = record.GetTarget().Encode().EndKey upsertQueryArgs[configIdx] = marshaled upsertValues[i] = fmt.Sprintf("($%d::BYTES, $%d::BYTES, $%d::BYTES)", startKeyIdx+1, endKeyIdx+1, configIdx+1) // prepared statement placeholders (1-indexed) @@ -384,8 +386,8 @@ func (k *KVAccessor) constructValidationStmtAndArgs( } startKeyIdx, endKeyIdx := i*2, (i*2)+1 - validationQueryArgs[startKeyIdx] = entry.Target.Encode().Key - validationQueryArgs[endKeyIdx] = entry.Target.Encode().EndKey + validationQueryArgs[startKeyIdx] = entry.GetTarget().Encode().Key + validationQueryArgs[endKeyIdx] = entry.GetTarget().Encode().EndKey fmt.Fprintf(&validationInnerStmtBuilder, ` SELECT count(*) = 1 FROM ( @@ -415,7 +417,7 @@ func validateUpdateArgs(toDelete []spanconfig.Target, toUpsert []spanconfig.Reco targetsToUpdate := func(recs []spanconfig.Record) []spanconfig.Target { targets := make([]spanconfig.Target, len(recs)) for i, ent := range recs { - targets[i] = ent.Target + targets[i] = ent.GetTarget() } return targets }(toUpsert) diff --git a/pkg/spanconfig/spanconfigkvaccessor/validation_test.go b/pkg/spanconfig/spanconfigkvaccessor/validation_test.go index 040f899aa404..31ed22658fee 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/validation_test.go +++ b/pkg/spanconfig/spanconfigkvaccessor/validation_test.go @@ -35,6 +35,12 @@ func TestValidateUpdateArgs(t *testing.T) { return spanconfig.MakeTargetFromSystemTarget(target) } + makeRecord := func(target spanconfig.Target, cfg roachpb.SpanConfig) spanconfig.Record { + record, err := spanconfig.MakeRecord(target, cfg) + require.NoError(t, err) + return record + } + for _, tc := range []struct { toDelete []spanconfig.Target toUpsert []spanconfig.Record @@ -54,21 +60,17 @@ func TestValidateUpdateArgs(t *testing.T) { }, { toUpsert: []spanconfig.Record{ - { - Target: spanconfig.MakeTargetFromSpan( - roachpb.Span{Key: roachpb.Key("a")}, // empty end key in update list - ), - }, + makeRecord(spanconfig.MakeTargetFromSpan( + roachpb.Span{Key: roachpb.Key("a")}, // empty end key in update list + ), roachpb.SpanConfig{}), }, expErr: "invalid span: a", }, { toUpsert: []spanconfig.Record{ - { - Target: spanconfig.MakeTargetFromSpan( - roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("a")}, // invalid span; end < start - ), - }, + makeRecord(spanconfig.MakeTargetFromSpan( + roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("a")}, // invalid span; end < start + ), roachpb.SpanConfig{}), }, expErr: "invalid span: {b-a}", }, @@ -90,16 +92,12 @@ func TestValidateUpdateArgs(t *testing.T) { }, { toUpsert: []spanconfig.Record{ // overlapping spans in the same list - { - Target: spanconfig.MakeTargetFromSpan( - roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}, - ), - }, - { - Target: spanconfig.MakeTargetFromSpan( - roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("c")}, - ), - }, + makeRecord(spanconfig.MakeTargetFromSpan( + roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}, + ), roachpb.SpanConfig{}), + makeRecord(spanconfig.MakeTargetFromSpan( + roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("c")}, + ), roachpb.SpanConfig{}), }, expErr: "overlapping spans {a-c} and {b-c} in same list", }, @@ -110,16 +108,12 @@ func TestValidateUpdateArgs(t *testing.T) { spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), }, toUpsert: []spanconfig.Record{ - { - Target: spanconfig.MakeTargetFromSpan( - roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("b")}, - ), - }, - { - Target: spanconfig.MakeTargetFromSpan( - roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("c")}, - ), - }, + makeRecord(spanconfig.MakeTargetFromSpan( + roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("b")}, + ), roachpb.SpanConfig{}), + makeRecord(spanconfig.MakeTargetFromSpan( + roachpb.Span{Key: roachpb.Key("b"), EndKey: roachpb.Key("c")}, + ), roachpb.SpanConfig{}), }, expErr: "", }, @@ -132,11 +126,8 @@ func TestValidateUpdateArgs(t *testing.T) { { // Duplicate in toUpsert. toUpsert: []spanconfig.Record{ - { - Target: makeTenantTarget(10), - }, - { - Target: makeTenantTarget(10)}, + makeRecord(makeTenantTarget(10), roachpb.SpanConfig{}), + makeRecord(makeTenantTarget(10), roachpb.SpanConfig{}), }, expErr: "duplicate system targets .* in the same list", }, @@ -173,12 +164,10 @@ func TestValidateUpdateArgs(t *testing.T) { spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("g"), EndKey: roachpb.Key("h")}), }, toUpsert: []spanconfig.Record{ - { - Target: makeTenantTarget(20), - }, - { - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), - }, + makeRecord(makeTenantTarget(10), roachpb.SpanConfig{}), + makeRecord(makeTenantTarget(20), roachpb.SpanConfig{}), + makeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), + roachpb.SpanConfig{}), }, expErr: "", }, @@ -194,11 +183,9 @@ func TestValidateUpdateArgs(t *testing.T) { { // Read only targets are not valid upsert args. toUpsert: []spanconfig.Record{ - { - Target: spanconfig.MakeTargetFromSystemTarget( - spanconfig.MakeAllTenantKeyspaceTargetsSet(roachpb.SystemTenantID), - ), - }, + makeRecord(spanconfig.MakeTargetFromSystemTarget( + spanconfig.MakeAllTenantKeyspaceTargetsSet(roachpb.SystemTenantID), + ), roachpb.SpanConfig{}), }, expErr: "cannot use read only system target .* as an update argument", }, diff --git a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go index 256b5651cd73..183a54e5dd0a 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go +++ b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go @@ -279,7 +279,7 @@ func (s *KVSubscriber) handlePartialUpdate( for i := range handlers { handler := &handlers[i] // mutated by invoke for _, ev := range events { - target := ev.(*bufferEvent).Update.Target + target := ev.(*bufferEvent).Update.GetTarget() handler.invoke(ctx, target.KeyspaceTargeted()) } } diff --git a/pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder.go b/pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder.go index d12a39badddb..20f2548d8335 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder.go +++ b/pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder.go @@ -87,10 +87,7 @@ func (sd *spanConfigDecoder) decode(kv roachpb.KeyValue) (spanconfig.Record, err } } - return spanconfig.Record{ - Target: spanconfig.DecodeTarget(rawSp), - Config: conf, - }, nil + return spanconfig.MakeRecord(spanconfig.DecodeTarget(rawSp), conf) } func (sd *spanConfigDecoder) translateEvent( @@ -121,12 +118,16 @@ func (sd *spanConfigDecoder) translateEvent( } if log.ExpensiveLogEnabled(ctx, 1) { - log.Infof(ctx, "received span configuration update for %s (deleted=%t)", record.Target, deleted) + log.Infof(ctx, "received span configuration update for %s (deleted=%t)", + record.GetTarget(), deleted) } var update spanconfig.Update if deleted { - update = spanconfig.Deletion(record.Target) + update, err = spanconfig.Deletion(record.GetTarget()) + if err != nil { + log.Fatalf(ctx, "failed to construct Deletion: %+v", err) + } } else { update = spanconfig.Update(record) } diff --git a/pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder_test.go b/pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder_test.go index 0c0d8d3211bd..ca0fbc2b42de 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder_test.go +++ b/pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder_test.go @@ -71,10 +71,10 @@ func TestDecodeSpanTargets(t *testing.T) { }, ) require.NoError(t, err) - require.Truef(t, span.Equal(got.Target.GetSpan()), - "expected span=%s, got span=%s", span, got.Target.GetSpan()) - require.Truef(t, conf.Equal(got.Config), - "expected config=%s, got config=%s", conf, got.Config) + require.Truef(t, span.Equal(got.GetTarget().GetSpan()), + "expected span=%s, got span=%s", span, got.GetTarget().GetSpan()) + require.Truef(t, conf.Equal(got.GetConfig()), + "expected config=%s, got config=%s", conf, got.GetConfig()) } // TestSpanConfigDecoder verifies that we can decode system target rows stored @@ -149,9 +149,9 @@ func TestDecodeSystemTargets(t *testing.T) { ) require.NoError(t, err) - require.Equal(t, conf, got.Config) - require.True(t, got.Target.IsSystemTarget()) - require.Equal(t, systemTarget, got.Target.GetSystemTarget()) + require.Equal(t, conf, got.GetConfig()) + require.True(t, got.GetTarget().IsSystemTarget()) + require.Equal(t, systemTarget, got.GetTarget().GetSystemTarget()) } } diff --git a/pkg/spanconfig/spanconfigreconciler/reconciler.go b/pkg/spanconfig/spanconfigreconciler/reconciler.go index a08a4f90bfd2..479246dae7b8 100644 --- a/pkg/spanconfig/spanconfigreconciler/reconciler.go +++ b/pkg/spanconfig/spanconfigreconciler/reconciler.go @@ -245,7 +245,11 @@ func (f *fullReconciler) reconcile( var storeWithExtraneousSpanConfigs *spanconfigstore.Store { for _, u := range updates { - storeWithExistingSpanConfigs.Apply(ctx, false /* dryrun */, spanconfig.Deletion(u.Target)) + del, err := spanconfig.Deletion(u.GetTarget()) + if err != nil { + return nil, hlc.Timestamp{}, err + } + storeWithExistingSpanConfigs.Apply(ctx, false /* dryrun */, del) } storeWithExtraneousSpanConfigs = storeWithExistingSpanConfigs } @@ -259,7 +263,11 @@ func (f *fullReconciler) reconcile( // contents. As before, we could've fetched this state from KV directly, but // doing it this way is cheaper. for _, d := range deletedSpans { - storeWithLatestSpanConfigs.Apply(ctx, false /* dryrun */, spanconfig.Deletion(d)) + del, err := spanconfig.Deletion(d) + if err != nil { + return nil, hlc.Timestamp{}, err + } + storeWithLatestSpanConfigs.Apply(ctx, false /* dryrun */, del) } return storeWithLatestSpanConfigs, reconciledUpUntil, nil @@ -337,7 +345,7 @@ func (f *fullReconciler) deleteExtraneousSpanConfigs( ) ([]spanconfig.Target, error) { var extraneousTargets []spanconfig.Target if err := storeWithExtraneousSpanConfigs.Iterate(func(record spanconfig.Record) error { - extraneousTargets = append(extraneousTargets, record.Target) + extraneousTargets = append(extraneousTargets, record.GetTarget()) return nil }, ); err != nil { @@ -428,11 +436,18 @@ func (r *incrementalReconciler) reconcile( Key: r.codec.TablePrefix(uint32(missingID)), EndKey: r.codec.TablePrefix(uint32(missingID)).PrefixEnd(), } - updates = append(updates, spanconfig.Deletion(spanconfig.MakeTargetFromSpan(tableSpan))) + del, err := spanconfig.Deletion(spanconfig.MakeTargetFromSpan(tableSpan)) + if err != nil { + return err + } + updates = append(updates, del) } for _, missingSystemTarget := range missingProtectedTimestampTargets { - updates = append(updates, spanconfig.Deletion( - spanconfig.MakeTargetFromSystemTarget(missingSystemTarget))) + del, err := spanconfig.Deletion(spanconfig.MakeTargetFromSystemTarget(missingSystemTarget)) + if err != nil { + return err + } + updates = append(updates, del) } toDelete, toUpsert := r.storeWithKVContents.Apply(ctx, false /* dryrun */, updates...) diff --git a/pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go b/pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go index cdd1ea064f61..3b37f531490c 100644 --- a/pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go +++ b/pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go @@ -177,9 +177,12 @@ func (s *SQLTranslator) generateSystemSpanConfigRecords( return nil, err } } - clusterSystemRecord := spanconfig.Record{ - Target: spanconfig.MakeTargetFromSystemTarget(systemTarget), - Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ProtectionPolicies: clusterProtections}}} + clusterSystemRecord, err := spanconfig.MakeRecord( + spanconfig.MakeTargetFromSystemTarget(systemTarget), + roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ProtectionPolicies: clusterProtections}}) + if err != nil { + return nil, err + } records = append(records, clusterSystemRecord) } @@ -191,10 +194,12 @@ func (s *SQLTranslator) generateSystemSpanConfigRecords( if err != nil { return nil, err } - tenantSystemRecord := spanconfig.Record{ - Target: spanconfig.MakeTargetFromSystemTarget(systemTarget), - Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ - ProtectionPolicies: tenantProtection.GetTenantProtections()}}, + tenantSystemRecord, err := spanconfig.MakeRecord( + spanconfig.MakeTargetFromSystemTarget(systemTarget), + roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ + ProtectionPolicies: tenantProtection.GetTenantProtections()}}) + if err != nil { + return nil, err } records = append(records, tenantSystemRecord) } @@ -292,10 +297,11 @@ func (s *SQLTranslator) generateSpanConfigurationsForNamedZone( spanConfig := zoneConfig.AsSpanConfig() var records []spanconfig.Record for _, span := range spans { - records = append(records, spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(span), - Config: spanConfig, - }) + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(span), spanConfig) + if err != nil { + return nil, err + } + records = append(records, record) } return records, nil } @@ -353,13 +359,15 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( // there's no data within [/Tenant// - /Tenant//Table/3), // but looking at range boundaries, it's slightly less confusing // this way. - records = append(records, spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ + record, err := spanconfig.MakeRecord( + spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: s.codec.TenantPrefix(), EndKey: tableEndKey, - }), - Config: tableSpanConfig, - }) + }), tableSpanConfig) + if err != nil { + return nil, err + } + records = append(records, record) } else { // The same as above, except we have named ranges preceding // `system.descriptor`. Not doing anything special here would mean @@ -370,13 +378,14 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( // somewhat useful for understandability reasons and reducing the // (tiny) re-splitting costs when switching between the two // subsystems. - records = append(records, spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ - Key: keys.SystemConfigSpan.Key, - EndKey: tableEndKey, - }), - Config: tableSpanConfig, - }) + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{ + Key: keys.SystemConfigSpan.Key, + EndKey: tableEndKey, + }), tableSpanConfig) + if err != nil { + return nil, err + } + records = append(records, record) } return records, nil @@ -433,12 +442,12 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( // If there is a "hole" in the spans covered by the subzones array we fill // it using the parent zone configuration. if !prevEndKey.Equal(span.Key) { - records = append(records, - spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{Key: prevEndKey, EndKey: span.Key}), - Config: tableSpanConfig, - }, - ) + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan( + roachpb.Span{Key: prevEndKey, EndKey: span.Key}), tableSpanConfig) + if err != nil { + return nil, err + } + records = append(records, record) } // Add an entry for the subzone. @@ -451,12 +460,12 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( subzoneSpanConfig.RangefeedEnabled = true subzoneSpanConfig.GCPolicy.IgnoreStrictEnforcement = true } - records = append(records, - spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{Key: span.Key, EndKey: span.EndKey}), - Config: subzoneSpanConfig, - }, - ) + record, err := spanconfig.MakeRecord( + spanconfig.MakeTargetFromSpan(roachpb.Span{Key: span.Key, EndKey: span.EndKey}), subzoneSpanConfig) + if err != nil { + return nil, err + } + records = append(records, record) prevEndKey = span.EndKey } @@ -464,12 +473,12 @@ func (s *SQLTranslator) generateSpanConfigurationsForTable( // If the last subzone span doesn't cover the entire table's keyspace then // we cover the remaining key range with the table's zone configuration. if !prevEndKey.Equal(tableEndKey) { - records = append(records, - spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{Key: prevEndKey, EndKey: tableEndKey}), - Config: tableSpanConfig, - }, - ) + record, err := spanconfig.MakeRecord( + spanconfig.MakeTargetFromSpan(roachpb.Span{Key: prevEndKey, EndKey: tableEndKey}), tableSpanConfig) + if err != nil { + return nil, err + } + records = append(records, record) } return records, nil } @@ -636,13 +645,14 @@ func (s *SQLTranslator) maybeGeneratePseudoTableRecords( for _, pseudoTableID := range keys.PseudoTableIDs { tableStartKey := s.codec.TablePrefix(pseudoTableID) tableEndKey := tableStartKey.PrefixEnd() - records = append(records, spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ - Key: tableStartKey, - EndKey: tableEndKey, - }), - Config: tableSpanConfig, - }) + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{ + Key: tableStartKey, + EndKey: tableEndKey, + }), tableSpanConfig) + if err != nil { + return nil, err + } + records = append(records, record) } return records, nil @@ -668,13 +678,15 @@ func (s *SQLTranslator) maybeGenerateScratchRangeRecord( return spanconfig.Record{}, err } - return spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ + record, err := spanconfig.MakeRecord( + spanconfig.MakeTargetFromSpan(roachpb.Span{ Key: keys.ScratchRangeMin, EndKey: keys.ScratchRangeMax, - }), - Config: zone.AsSpanConfig(), - }, nil + }), zone.AsSpanConfig()) + if err != nil { + return spanconfig.Record{}, err + } + return record, nil } return spanconfig.Record{}, nil diff --git a/pkg/spanconfig/spanconfigstore/spanconfigstore.go b/pkg/spanconfig/spanconfigstore/spanconfigstore.go index 2e3af504697f..13b93688aef0 100644 --- a/pkg/spanconfig/spanconfigstore/spanconfigstore.go +++ b/pkg/spanconfig/spanconfigstore/spanconfigstore.go @@ -43,10 +43,11 @@ func newSpanConfigStore() *spanConfigStore { func (s *spanConfigStore) copy(ctx context.Context) *spanConfigStore { clone := newSpanConfigStore() _ = s.forEachOverlapping(keys.EverythingSpan, func(entry spanConfigEntry) error { - _, _, err := clone.apply(false /* dryrun */, spanconfig.Update{ - Target: spanconfig.MakeTargetFromSpan(entry.span), - Config: entry.config, - }) + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(entry.span), entry.config) + if err != nil { + log.Fatalf(ctx, "%v", err) + } + _, _, err = clone.apply(false /* dryrun */, spanconfig.Update(record)) if err != nil { log.Fatalf(ctx, "%v", err) } @@ -141,7 +142,7 @@ func (s *spanConfigStore) apply( sorted := make([]spanconfig.Update, len(updates)) copy(sorted, updates) sort.Slice(sorted, func(i, j int) bool { - return sorted[i].Target.Less(sorted[j].Target) + return sorted[i].GetTarget().Less(sorted[j].GetTarget()) }) updates = sorted // re-use the same variable @@ -273,13 +274,17 @@ func (s *spanConfigStore) accumulateOpsFor( for _, update := range updates { var carriedOver spanConfigEntry carriedOver, carryOver = carryOver, spanConfigEntry{} - if update.Target.GetSpan().Overlaps(carriedOver.span) { - gapBetweenUpdates := roachpb.Span{Key: carriedOver.span.Key, EndKey: update.Target.GetSpan().Key} + if update.GetTarget().GetSpan().Overlaps(carriedOver.span) { + gapBetweenUpdates := roachpb.Span{ + Key: carriedOver.span.Key, + EndKey: update.GetTarget().GetSpan().Key} if gapBetweenUpdates.Valid() { toAdd = append(toAdd, s.makeEntry(gapBetweenUpdates, carriedOver.config)) } - carryOverSpanAfterUpdate := roachpb.Span{Key: update.Target.GetSpan().EndKey, EndKey: carriedOver.span.EndKey} + carryOverSpanAfterUpdate := roachpb.Span{ + Key: update.GetTarget().GetSpan().EndKey, + EndKey: carriedOver.span.EndKey} if carryOverSpanAfterUpdate.Valid() { carryOver = spanConfigEntry{ span: carryOverSpanAfterUpdate, @@ -291,22 +296,22 @@ func (s *spanConfigStore) accumulateOpsFor( } skipAddingSelf := false - for _, overlapping := range s.tree.Get(update.Target.GetSpan().AsRange()) { + for _, overlapping := range s.tree.Get(update.GetTarget().GetSpan().AsRange()) { existing := overlapping.(*spanConfigStoreEntry) if existing.span.Overlaps(carriedOver.span) { continue // we've already processed this entry above. } var ( - union = existing.span.Combine(update.Target.GetSpan()) - inter = existing.span.Intersect(update.Target.GetSpan()) + union = existing.span.Combine(update.GetTarget().GetSpan()) + inter = existing.span.Intersect(update.GetTarget().GetSpan()) pre = roachpb.Span{Key: union.Key, EndKey: inter.Key} post = roachpb.Span{Key: inter.EndKey, EndKey: union.EndKey} ) if update.Addition() { - if existing.span.Equal(update.Target.GetSpan()) && existing.config.Equal(update.Config) { + if existing.span.Equal(update.GetTarget().GetSpan()) && existing.config.Equal(update.GetConfig()) { skipAddingSelf = true break // no-op; peep-hole optimization } @@ -316,7 +321,7 @@ func (s *spanConfigStore) accumulateOpsFor( // non-intersecting parts of the span. toDelete = append(toDelete, *existing) // existing entry contains the update span's start key - if existing.span.ContainsKey(update.Target.GetSpan().Key) { + if existing.span.ContainsKey(update.GetTarget().GetSpan().Key) { // ex: [-----------------) // // up: [-------) @@ -332,7 +337,7 @@ func (s *spanConfigStore) accumulateOpsFor( } } - if existing.span.ContainsKey(update.Target.GetSpan().EndKey) { // existing entry contains the update span's end key + if existing.span.ContainsKey(update.GetTarget().GetSpan().EndKey) { // existing entry contains the update span's end key // ex: [-----------------) // // up: -------------) @@ -349,7 +354,7 @@ func (s *spanConfigStore) accumulateOpsFor( if update.Addition() && !skipAddingSelf { // Add the update itself. - toAdd = append(toAdd, s.makeEntry(update.Target.GetSpan(), update.Config)) + toAdd = append(toAdd, s.makeEntry(update.GetTarget().GetSpan(), update.GetConfig())) // TODO(irfansharif): If we're adding an entry, we could inspect the // entries before and after and check whether either of them have @@ -385,11 +390,11 @@ func (s *spanConfigStore) makeEntry(sp roachpb.Span, conf roachpb.SpanConfig) sp // spans, those spans be valid, and non-overlapping. func validateApplyArgs(updates ...spanconfig.Update) error { for i := range updates { - if !updates[i].Target.IsSpanTarget() { + if !updates[i].GetTarget().IsSpanTarget() { return errors.New("expected update to target a span") } - sp := updates[i].Target.GetSpan() + sp := updates[i].GetTarget().GetSpan() if !sp.Valid() || len(sp.EndKey) == 0 { return errors.New("invalid span") } @@ -398,7 +403,7 @@ func validateApplyArgs(updates ...spanconfig.Update) error { sorted := make([]spanconfig.Update, len(updates)) copy(sorted, updates) sort.Slice(sorted, func(i, j int) bool { - return sorted[i].Target.GetSpan().Key.Compare(sorted[j].Target.GetSpan().Key) < 0 + return sorted[i].GetTarget().GetSpan().Key.Compare(sorted[j].GetTarget().GetSpan().Key) < 0 }) updates = sorted // re-use the same variable @@ -406,11 +411,11 @@ func validateApplyArgs(updates ...spanconfig.Update) error { if i == 0 { continue } - if updates[i].Target.GetSpan().Overlaps(updates[i-1].Target.GetSpan()) { + if updates[i].GetTarget().GetSpan().Overlaps(updates[i-1].GetTarget().GetSpan()) { return errors.Newf( "found overlapping updates %s and %s", - updates[i-1].Target.GetSpan(), - updates[i].Target.GetSpan(), + updates[i-1].GetTarget().GetSpan(), + updates[i].GetTarget().GetSpan(), ) } } diff --git a/pkg/spanconfig/spanconfigstore/spanconfigstore_test.go b/pkg/spanconfig/spanconfigstore/spanconfigstore_test.go index 537c442fcbed..8d2d53f8142d 100644 --- a/pkg/spanconfig/spanconfigstore/spanconfigstore_test.go +++ b/pkg/spanconfig/spanconfigstore/spanconfigstore_test.go @@ -65,9 +65,13 @@ func TestRandomized(t *testing.T) { sp, conf, op := genRandomSpan(), getRandomConf(), getRandomOp() switch op { case "set": - return spanconfig.Addition(spanconfig.MakeTargetFromSpan(sp), conf) + addition, err := spanconfig.Addition(spanconfig.MakeTargetFromSpan(sp), conf) + require.NoError(t, err) + return addition case "del": - return spanconfig.Deletion(spanconfig.MakeTargetFromSpan(sp)) + del, err := spanconfig.Deletion(spanconfig.MakeTargetFromSpan(sp)) + require.NoError(t, err) + return del default: } t.Fatalf("unexpected op: %s", op) @@ -82,11 +86,11 @@ func TestRandomized(t *testing.T) { updates[i] = getRandomUpdate() } sort.Slice(updates, func(i, j int) bool { - return updates[i].Target.Less(updates[j].Target) + return updates[i].GetTarget().Less(updates[j].GetTarget()) }) invalid := false for i := 1; i < numUpdates; i++ { - if updates[i].Target.GetSpan().Overlaps(updates[i-1].Target.GetSpan()) { + if updates[i].GetTarget().GetSpan().Overlaps(updates[i-1].GetTarget().GetSpan()) { invalid = true } } @@ -113,9 +117,9 @@ func TestRandomized(t *testing.T) { _, _, err := store.apply(false /* dryrun */, updates...) require.NoError(t, err) for _, update := range updates { - if testSpan.Overlaps(update.Target.GetSpan()) { + if testSpan.Overlaps(update.GetTarget().GetSpan()) { if update.Addition() { - expConfig, expFound = update.Config, true + expConfig, expFound = update.GetConfig(), true } else { expConfig, expFound = roachpb.SpanConfig{}, false } @@ -126,11 +130,10 @@ func TestRandomized(t *testing.T) { if !expFound { _ = store.forEachOverlapping(testSpan, func(entry spanConfigEntry) error { + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(entry.span), entry.config) + require.NoError(t, err) t.Fatalf("found unexpected entry: %s", - spanconfigtestutils.PrintSpanConfigRecord(t, spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(entry.span), - Config: entry.config, - })) + spanconfigtestutils.PrintSpanConfigRecord(t, record)) return nil }, ) @@ -139,11 +142,10 @@ func TestRandomized(t *testing.T) { _ = store.forEachOverlapping(testSpan, func(entry spanConfigEntry) error { if !foundEntry.isEmpty() { + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(entry.span), entry.config) + require.NoError(t, err) t.Fatalf("expected single overlapping entry, found second: %s", - spanconfigtestutils.PrintSpanConfigRecord(t, spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(entry.span), - Config: entry.config, - })) + spanconfigtestutils.PrintSpanConfigRecord(t, record)) } foundEntry = entry diff --git a/pkg/spanconfig/spanconfigstore/store.go b/pkg/spanconfig/spanconfigstore/store.go index eab10ce690e7..d3e94c48e238 100644 --- a/pkg/spanconfig/spanconfigstore/store.go +++ b/pkg/spanconfig/spanconfigstore/store.go @@ -154,9 +154,9 @@ func (s *Store) applyInternal( systemSpanConfigStoreUpdates := make([]spanconfig.Update, 0, len(updates)) for _, update := range updates { switch { - case update.Target.IsSpanTarget(): + case update.GetTarget().IsSpanTarget(): spanStoreUpdates = append(spanStoreUpdates, update) - case update.Target.IsSystemTarget(): + case update.GetTarget().IsSystemTarget(): systemSpanConfigStoreUpdates = append(systemSpanConfigStoreUpdates, update) default: return nil, nil, errors.AssertionFailedf("unknown target type") @@ -172,10 +172,12 @@ func (s *Store) applyInternal( } for _, entry := range addedEntries { - added = append(added, spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(entry.span), - Config: entry.config, - }) + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(entry.span), + entry.config) + if err != nil { + return nil, nil, err + } + added = append(added, record) } deletedSystemTargets, addedSystemSpanConfigRecords, err := s.mu.systemSpanConfigStore.apply( @@ -203,9 +205,10 @@ func (s *Store) Iterate(f func(spanconfig.Record) error) error { return s.mu.spanConfigStore.forEachOverlapping( keys.EverythingSpan, func(s spanConfigEntry) error { - return f(spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(s.span), - Config: s.config, - }) + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(s.span), s.config) + if err != nil { + return err + } + return f(record) }) } diff --git a/pkg/spanconfig/spanconfigstore/store_test.go b/pkg/spanconfig/spanconfigstore/store_test.go index 62c751be37bb..8c3afe332ee0 100644 --- a/pkg/spanconfig/spanconfigstore/store_test.go +++ b/pkg/spanconfig/spanconfigstore/store_test.go @@ -89,7 +89,7 @@ func TestDataDriven(t *testing.T) { sort.Sort(spanconfig.Targets(deleted)) sort.Slice(added, func(i, j int) bool { - return added[i].Target.Less(added[j].Target) + return added[i].GetTarget().Less(added[j].GetTarget()) }) var b strings.Builder @@ -128,12 +128,11 @@ func TestDataDriven(t *testing.T) { var results []string _ = store.ForEachOverlappingSpanConfig(ctx, span, func(sp roachpb.Span, conf roachpb.SpanConfig) error { - results = append(results, - spanconfigtestutils.PrintSpanConfigRecord(t, spanconfig.Record{ - Target: spanconfig.MakeTargetFromSpan(sp), - Config: conf, - }), - ) + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(sp), conf) + if err != nil { + return err + } + results = append(results, spanconfigtestutils.PrintSpanConfigRecord(t, record)) return nil }, ) @@ -155,30 +154,35 @@ func TestStoreClone(t *testing.T) { ctx := context.Background() + makeSpanConfigAddition := func(target spanconfig.Target, conf roachpb.SpanConfig) spanconfig.Update { + addition, err := spanconfig.Addition(target, conf) + require.NoError(t, err) + return addition + } updates := []spanconfig.Update{ - spanconfig.Addition( + makeSpanConfigAddition( spanconfig.MakeTargetFromSpan(spanconfigtestutils.ParseSpan(t, "[a, b)")), spanconfigtestutils.ParseConfig(t, "A"), ), - spanconfig.Addition( + makeSpanConfigAddition( spanconfig.MakeTargetFromSpan(spanconfigtestutils.ParseSpan(t, "[c, d)")), spanconfigtestutils.ParseConfig(t, "C"), ), - spanconfig.Addition( + makeSpanConfigAddition( spanconfig.MakeTargetFromSpan(spanconfigtestutils.ParseSpan(t, "[e, f)")), spanconfigtestutils.ParseConfig(t, "E"), ), - spanconfig.Addition( + makeSpanConfigAddition( spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeEntireKeyspaceTarget()), spanconfigtestutils.ParseConfig(t, "G"), ), - spanconfig.Addition( + makeSpanConfigAddition( spanconfig.MakeTargetFromSystemTarget(spanconfig.TestingMakeTenantKeyspaceTargetOrFatal( t, roachpb.SystemTenantID, roachpb.MakeTenantID(10), )), spanconfigtestutils.ParseConfig(t, "H"), ), - spanconfig.Addition( + makeSpanConfigAddition( spanconfig.MakeTargetFromSystemTarget(spanconfig.TestingMakeTenantKeyspaceTargetOrFatal( t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10), )), @@ -205,8 +209,9 @@ func TestStoreClone(t *testing.T) { require.Equal(t, len(originalRecords), len(clonedRecords)) for i := 0; i < len(originalRecords); i++ { require.True( - t, originalRecords[i].Target.Equal(clonedRecords[i].Target), + t, originalRecords[i].GetTarget().Equal(clonedRecords[i].GetTarget()), ) - require.True(t, originalRecords[i].Config.Equal(clonedRecords[i].Config)) + originalConfig := originalRecords[i].GetConfig() + require.True(t, originalConfig.Equal(clonedRecords[i].GetConfig())) } } diff --git a/pkg/spanconfig/spanconfigstore/systemspanconfigstore.go b/pkg/spanconfig/spanconfigstore/systemspanconfigstore.go index 70b7f403ab2f..42bb67df0102 100644 --- a/pkg/spanconfig/spanconfigstore/systemspanconfigstore.go +++ b/pkg/spanconfig/spanconfigstore/systemspanconfigstore.go @@ -47,26 +47,26 @@ func (s *systemSpanConfigStore) apply( for _, update := range updates { if update.Addition() { - conf, found := s.store[update.Target.GetSystemTarget()] + conf, found := s.store[update.GetTarget().GetSystemTarget()] if found { - if conf.Equal(update.Config) { + if conf.Equal(update.GetConfig()) { // no-op. continue } - deleted = append(deleted, update.Target.GetSystemTarget()) + deleted = append(deleted, update.GetTarget().GetSystemTarget()) } - s.store[update.Target.GetSystemTarget()] = update.Config + s.store[update.GetTarget().GetSystemTarget()] = update.GetConfig() added = append(added, spanconfig.Record(update)) } if update.Deletion() { - _, found := s.store[update.Target.GetSystemTarget()] + _, found := s.store[update.GetTarget().GetSystemTarget()] if !found { continue // no-op } - delete(s.store, update.Target.GetSystemTarget()) - deleted = append(deleted, update.Target.GetSystemTarget()) + delete(s.store, update.GetTarget().GetSystemTarget()) + deleted = append(deleted, update.GetTarget().GetSystemTarget()) } } @@ -139,10 +139,11 @@ func (s *systemSpanConfigStore) iterate(f func(record spanconfig.Record) error) sort.Sort(spanconfig.Targets(targets)) for _, target := range targets { - if err := f(spanconfig.Record{ - Target: target, - Config: s.store[target.GetSystemTarget()], - }); err != nil { + record, err := spanconfig.MakeRecord(target, s.store[target.GetSystemTarget()]) + if err != nil { + return err + } + if err := f(record); err != nil { return err } } @@ -154,11 +155,11 @@ func (s *systemSpanConfigStore) iterate(f func(record spanconfig.Record) error) // and duplicate targets do not exist. func (s *systemSpanConfigStore) validateApplyArgs(updates []spanconfig.Update) error { for _, update := range updates { - if !update.Target.IsSystemTarget() { + if !update.GetTarget().IsSystemTarget() { return errors.AssertionFailedf("expected update to system target update") } - if update.Target.GetSystemTarget().IsReadOnly() { + if update.GetTarget().GetSystemTarget().IsReadOnly() { return errors.AssertionFailedf("invalid system target update") } } @@ -166,7 +167,7 @@ func (s *systemSpanConfigStore) validateApplyArgs(updates []spanconfig.Update) e sorted := make([]spanconfig.Update, len(updates)) copy(sorted, updates) sort.Slice(sorted, func(i, j int) bool { - return sorted[i].Target.Less(sorted[j].Target) + return sorted[i].GetTarget().Less(sorted[j].GetTarget()) }) updates = sorted // re-use the same variable @@ -175,11 +176,11 @@ func (s *systemSpanConfigStore) validateApplyArgs(updates []spanconfig.Update) e continue } - if updates[i].Target.Equal(updates[i-1].Target) { + if updates[i].GetTarget().Equal(updates[i-1].GetTarget()) { return errors.Newf( "found duplicate updates %s and %s", - updates[i-1].Target, - updates[i].Target, + updates[i-1].GetTarget(), + updates[i].GetTarget(), ) } } diff --git a/pkg/spanconfig/spanconfigstore/systemspanconfigstore_test.go b/pkg/spanconfig/spanconfigstore/systemspanconfigstore_test.go index 76e1adc6d11f..fa0b34cce375 100644 --- a/pkg/spanconfig/spanconfigstore/systemspanconfigstore_test.go +++ b/pkg/spanconfig/spanconfigstore/systemspanconfigstore_test.go @@ -54,41 +54,46 @@ func TestSystemSpanConfigStoreCombine(t *testing.T) { // Ten 20 -> Ten 20: 300 // // Span config: 1 + makeSpanConfigAddition := func(target spanconfig.Target, conf roachpb.SpanConfig) spanconfig.Update { + addition, err := spanconfig.Addition(target, conf) + require.NoError(t, err) + return addition + } updates := []spanconfig.Update{ - spanconfig.Addition( + makeSpanConfigAddition( spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeEntireKeyspaceTarget()), makeSystemSpanConfig(100), ), - spanconfig.Addition(spanconfig.MakeTargetFromSystemTarget( + makeSpanConfigAddition(spanconfig.MakeTargetFromSystemTarget( spanconfig.TestingMakeTenantKeyspaceTargetOrFatal( t, roachpb.SystemTenantID, roachpb.SystemTenantID, ), ), makeSystemSpanConfig(120), ), - spanconfig.Addition(spanconfig.MakeTargetFromSystemTarget( + makeSpanConfigAddition(spanconfig.MakeTargetFromSystemTarget( spanconfig.TestingMakeTenantKeyspaceTargetOrFatal( t, roachpb.SystemTenantID, roachpb.MakeTenantID(10), ), ), makeSystemSpanConfig(150), ), - spanconfig.Addition(spanconfig.MakeTargetFromSystemTarget( + makeSpanConfigAddition(spanconfig.MakeTargetFromSystemTarget( spanconfig.TestingMakeTenantKeyspaceTargetOrFatal( t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10), ), ), makeSystemSpanConfig(200), ), - spanconfig.Addition(spanconfig.MakeTargetFromSystemTarget( + makeSpanConfigAddition(spanconfig.MakeTargetFromSystemTarget( spanconfig.TestingMakeTenantKeyspaceTargetOrFatal( t, roachpb.MakeTenantID(20), roachpb.MakeTenantID(20), ), ), makeSystemSpanConfig(300), ), - spanconfig.Addition(spanconfig.MakeTargetFromSystemTarget( + makeSpanConfigAddition(spanconfig.MakeTargetFromSystemTarget( spanconfig.TestingMakeTenantKeyspaceTargetOrFatal( t, roachpb.SystemTenantID, roachpb.MakeTenantID(30), ), diff --git a/pkg/spanconfig/spanconfigtestutils/recorder.go b/pkg/spanconfig/spanconfigtestutils/recorder.go index dab0543198fc..6dafa75951af 100644 --- a/pkg/spanconfig/spanconfigtestutils/recorder.go +++ b/pkg/spanconfig/spanconfigtestutils/recorder.go @@ -66,8 +66,12 @@ func (r *KVAccessorRecorder) UpdateSpanConfigRecords( defer r.mu.Unlock() for _, d := range toDelete { + del, err := spanconfig.Deletion(d) + if err != nil { + return err + } r.mu.mutations = append(r.mu.mutations, mutation{ - update: spanconfig.Deletion(d), + update: del, batchIdx: r.mu.batchCount, }) } @@ -98,8 +102,8 @@ func (r *KVAccessorRecorder) Recording(clear bool) string { if mi.batchIdx != mj.batchIdx { // sort by batch/ts order return mi.batchIdx < mj.batchIdx } - if !mi.update.Target.Equal(mj.update.Target) { // sort by target order - return mi.update.Target.Less(mj.update.Target) + if !mi.update.GetTarget().Equal(mj.update.GetTarget()) { // sort by target order + return mi.update.GetTarget().Less(mj.update.GetTarget()) } return mi.update.Deletion() // sort deletes before upserts @@ -111,15 +115,15 @@ func (r *KVAccessorRecorder) Recording(clear bool) string { var output strings.Builder for _, m := range r.mu.mutations { if m.update.Deletion() { - output.WriteString(fmt.Sprintf("delete %s\n", m.update.Target)) + output.WriteString(fmt.Sprintf("delete %s\n", m.update.GetTarget())) } else { switch { - case m.update.Target.IsSpanTarget(): - output.WriteString(fmt.Sprintf("upsert %-35s %s\n", m.update.Target, - PrintSpanConfigDiffedAgainstDefaults(m.update.Config))) - case m.update.Target.IsSystemTarget(): - output.WriteString(fmt.Sprintf("upsert %-35s %s\n", m.update.Target, - PrintSystemSpanConfigDiffedAgainstDefault(m.update.Config))) + case m.update.GetTarget().IsSpanTarget(): + output.WriteString(fmt.Sprintf("upsert %-35s %s\n", m.update.GetTarget(), + PrintSpanConfigDiffedAgainstDefaults(m.update.GetConfig()))) + case m.update.GetTarget().IsSystemTarget(): + output.WriteString(fmt.Sprintf("upsert %-35s %s\n", m.update.GetTarget(), + PrintSystemSpanConfigDiffedAgainstDefault(m.update.GetConfig()))) default: panic("unsupported target type") } diff --git a/pkg/spanconfig/spanconfigtestutils/utils.go b/pkg/spanconfig/spanconfigtestutils/utils.go index 00a4b9fc775d..b610a7413a7f 100644 --- a/pkg/spanconfig/spanconfigtestutils/utils.go +++ b/pkg/spanconfig/spanconfigtestutils/utils.go @@ -134,10 +134,10 @@ func ParseSpanConfigRecord(t *testing.T, conf string) spanconfig.Record { if len(parts) != 2 { t.Fatalf("expected single %q separator", ":") } - return spanconfig.Record{ - Target: ParseTarget(t, parts[0]), - Config: ParseConfig(t, parts[1]), - } + record, err := spanconfig.MakeRecord(ParseTarget(t, parts[0]), + ParseConfig(t, parts[1])) + require.NoError(t, err) + return record } // ParseKVAccessorGetArguments is a helper function that parses datadriven @@ -236,7 +236,9 @@ func ParseStoreApplyArguments(t *testing.T, input string) (updates []spanconfig. switch { case strings.HasPrefix(line, deletePrefix): line = strings.TrimPrefix(line, line[:len(deletePrefix)]) - updates = append(updates, spanconfig.Deletion(ParseTarget(t, line))) + del, err := spanconfig.Deletion(ParseTarget(t, line)) + require.NoError(t, err) + updates = append(updates, del) case strings.HasPrefix(line, setPrefix): line = strings.TrimPrefix(line, line[:len(setPrefix)]) entry := ParseSpanConfigRecord(t, line) @@ -310,7 +312,7 @@ func PrintSpanConfig(config roachpb.SpanConfig) string { // above, or the constituent span and config to have been constructed using the // Parse{Span,Config} helpers above. func PrintSpanConfigRecord(t *testing.T, record spanconfig.Record) string { - return fmt.Sprintf("%s:%s", PrintTarget(t, record.Target), PrintSpanConfig(record.Config)) + return fmt.Sprintf("%s:%s", PrintTarget(t, record.GetTarget()), PrintSpanConfig(record.GetConfig())) } // PrintSystemSpanConfigDiffedAgainstDefault is a helper function that diffs the diff --git a/pkg/spanconfig/target.go b/pkg/spanconfig/target.go index 16336536725c..361012cbe2d6 100644 --- a/pkg/spanconfig/target.go +++ b/pkg/spanconfig/target.go @@ -234,8 +234,8 @@ func RecordsToEntries(records []Record) []roachpb.SpanConfigEntry { entries := make([]roachpb.SpanConfigEntry, 0, len(records)) for _, rec := range records { entries = append(entries, roachpb.SpanConfigEntry{ - Target: rec.Target.ToProto(), - Config: rec.Config, + Target: rec.GetTarget().ToProto(), + Config: rec.GetConfig(), }) } return entries @@ -250,10 +250,11 @@ func EntriesToRecords(entries []roachpb.SpanConfigEntry) ([]Record, error) { if err != nil { return nil, err } - records = append(records, Record{ - Target: target, - Config: entry.Config, - }) + record, err := MakeRecord(target, entry.Config) + if err != nil { + return nil, err + } + records = append(records, record) } return records, nil } diff --git a/pkg/sql/tenant.go b/pkg/sql/tenant.go index 10ea4db47ef5..4f7081fb5b90 100644 --- a/pkg/sql/tenant.go +++ b/pkg/sql/tenant.go @@ -161,15 +161,14 @@ func CreateTenantRecord( tenantSpanConfig.GCPolicy.IgnoreStrictEnforcement = true tenantPrefix := keys.MakeTenantPrefix(roachpb.MakeTenantID(tenID)) - toUpsert := []spanconfig.Record{ - { - Target: spanconfig.MakeTargetFromSpan(roachpb.Span{ - Key: tenantPrefix, - EndKey: tenantPrefix.Next(), - }), - Config: tenantSpanConfig, - }, + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(roachpb.Span{ + Key: tenantPrefix, + EndKey: tenantPrefix.Next(), + }), tenantSpanConfig) + if err != nil { + return err } + toUpsert := []spanconfig.Record{record} scKVAccessor := execCfg.SpanConfigKVAccessor.WithTxn(ctx, txn) return scKVAccessor.UpdateSpanConfigRecords( ctx, nil /* toDelete */, toUpsert, @@ -486,7 +485,7 @@ func GCTenantSync(ctx context.Context, execCfg *ExecutorConfig, info *descpb.Ten toDelete := make([]spanconfig.Target, len(records)) for i, record := range records { - toDelete[i] = record.Target + toDelete[i] = record.GetTarget() } return scKVAccessor.UpdateSpanConfigRecords(ctx, toDelete, nil /* toUpsert */) }) From e55e6b6eb54b909322bd47035781075554ea5fa3 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 10 Mar 2022 10:57:09 -0500 Subject: [PATCH 3/4] optbuilder: do not create invalid casts when building COALESCE and IF The optbuilder no longer creates invalid casts when building COALESCE and IF expressions that have children with different types. Expressions that previously caused internal errors now result in user-facing errors. Both UNION and CASE expressions had similar bugs that were recently fixed in #75219 and #76193. This commit also updates the `tree.ReType` function to return `ok=false` if there is no valid cast to re-type the expression to the given type. This forces callers to explicitly deal with situations where re-typing is not possible and it ensures that the function never creates invalid casts. This will make it easier to track down future related bugs because internal errors should originate from the call site of `tree.ReType` rather than from logic further along in the optimization process (in the case of #76807 the internal error originated from the logical props builder when it attempted to lookup the volatility of the invalid cast). This commit also adds special logic to make casts from any tuple type to `types.AnyTuple` valid immutable, implicit casts. Evaluation of these casts are no-ops. Users cannot construct these casts, but they are built by optbuilder in some cases. Fixes #76807 Release justification: This is a low-risk change that fixes a minor bug. Release note (bug fix): A bug has been fixed that caused internal errors when COALESCE and IF expressions had inner expressions with different types that could not be cast to a common type. --- pkg/sql/opt/exec/execbuilder/scalar.go | 6 +- pkg/sql/opt/optbuilder/scalar.go | 98 ++++++++++++++++---------- pkg/sql/opt/optbuilder/testdata/scalar | 42 ++++++++++- pkg/sql/opt/optbuilder/window.go | 2 +- pkg/sql/sem/tree/cast.go | 15 +++- pkg/sql/sem/tree/constant_eval.go | 9 ++- pkg/sql/sem/tree/normalize.go | 40 +++++++---- 7 files changed, 156 insertions(+), 56 deletions(-) diff --git a/pkg/sql/opt/exec/execbuilder/scalar.go b/pkg/sql/opt/exec/execbuilder/scalar.go index 4c57ae459d24..84a6ca1d1ad6 100644 --- a/pkg/sql/opt/exec/execbuilder/scalar.go +++ b/pkg/sql/opt/exec/execbuilder/scalar.go @@ -114,7 +114,11 @@ func (b *Builder) buildTypedExpr( } func (b *Builder) buildNull(ctx *buildScalarCtx, scalar opt.ScalarExpr) (tree.TypedExpr, error) { - return tree.ReType(tree.DNull, scalar.DataType()), nil + retypedNull, ok := tree.ReType(tree.DNull, scalar.DataType()) + if !ok { + return nil, errors.AssertionFailedf("failed to retype NULL to %s", scalar.DataType()) + } + return retypedNull, nil } func (b *Builder) buildVariable( diff --git a/pkg/sql/opt/optbuilder/scalar.go b/pkg/sql/opt/optbuilder/scalar.go index 7cd57eb137d5..a47c7082d5f9 100644 --- a/pkg/sql/opt/optbuilder/scalar.go +++ b/pkg/sql/opt/optbuilder/scalar.go @@ -119,8 +119,8 @@ func (b *Builder) buildScalar( return b.finishBuildScalarRef(t.col, inScope, outScope, outCol, colRefs) case *tree.AndExpr: - left := b.buildScalar(tree.ReType(t.TypedLeft(), types.Bool), inScope, nil, nil, colRefs) - right := b.buildScalar(tree.ReType(t.TypedRight(), types.Bool), inScope, nil, nil, colRefs) + left := b.buildScalar(reType(t.TypedLeft(), types.Bool), inScope, nil, nil, colRefs) + right := b.buildScalar(reType(t.TypedRight(), types.Bool), inScope, nil, nil, colRefs) out = b.factory.ConstructAnd(left, right) case *tree.Array: @@ -216,8 +216,8 @@ func (b *Builder) buildScalar( // select the right overload. The solution is to wrap any mismatched // arguments with a CastExpr that preserves the static type. - left := tree.ReType(t.TypedLeft(), t.ResolvedBinOp().LeftType) - right := tree.ReType(t.TypedRight(), t.ResolvedBinOp().RightType) + left := reType(t.TypedLeft(), t.ResolvedBinOp().LeftType) + right := reType(t.TypedRight(), t.ResolvedBinOp().RightType) out = b.constructBinary( treebin.MakeBinaryOperator(t.Operator.Symbol), b.buildScalar(left, inScope, nil, nil, colRefs), @@ -235,43 +235,32 @@ func (b *Builder) buildScalar( input = memo.TrueSingleton } - // validateCastToValType panics if tree.ReType with the given source - // type would create an invalid cast to valType. - validateCastToValType := func(src *types.T) { - if valType.Family() == types.AnyFamily || src.Identical(valType) { - // If valType's family is AnyFamily or src is identical to - // valType, then tree.Retype will not create a cast expression. - return - } - if tree.ValidCast(src, valType, tree.CastContextExplicit) { - // TODO(#75103): For legacy reasons, we check for a valid cast - // in the most permissive context, CastContextExplicit. To be - // consistent with Postgres, we should check for a valid cast in - // the most restrictive context, CastContextImplicit. - return - } - panic(pgerror.Newf( - pgcode.DatatypeMismatch, - "CASE types %s and %s cannot be matched", src, valType, - )) - } - whens := make(memo.ScalarListExpr, 0, len(t.Whens)+1) for i := range t.Whens { condExpr := t.Whens[i].Cond.(tree.TypedExpr) cond := b.buildScalar(condExpr, inScope, nil, nil, colRefs) - valExpr := t.Whens[i].Val.(tree.TypedExpr) - validateCastToValType(valExpr.ResolvedType()) - valExpr = tree.ReType(valExpr, valType) + valExpr, ok := tree.ReType(t.Whens[i].Val.(tree.TypedExpr), valType) + if !ok { + panic(pgerror.Newf( + pgcode.DatatypeMismatch, + "CASE WHEN types %s and %s cannot be matched", + t.Whens[i].Val.(tree.TypedExpr).ResolvedType(), valType, + )) + } val := b.buildScalar(valExpr, inScope, nil, nil, colRefs) whens = append(whens, b.factory.ConstructWhen(cond, val)) } // Add the ELSE expression to the end of whens as a raw scalar expression. var orElse opt.ScalarExpr if t.Else != nil { - elseExpr := t.Else.(tree.TypedExpr) - validateCastToValType(elseExpr.ResolvedType()) - elseExpr = tree.ReType(elseExpr, valType) + elseExpr, ok := tree.ReType(t.Else.(tree.TypedExpr), valType) + if !ok { + panic(pgerror.Newf( + pgcode.DatatypeMismatch, + "CASE ELSE type %s cannot be matched to WHEN type %s", + t.Else.(tree.TypedExpr).ResolvedType(), valType, + )) + } orElse = b.buildScalar(elseExpr, inScope, nil, nil, colRefs) } else { orElse = b.factory.ConstructNull(valType) @@ -290,7 +279,14 @@ func (b *Builder) buildScalar( // The type of the CoalesceExpr might be different than the inputs (e.g. // when they are NULL). Force all inputs to be the same type, so that we // build coalesce operator with the correct type. - expr := tree.ReType(t.TypedExprAt(i), typ) + expr, ok := tree.ReType(t.TypedExprAt(i), typ) + if !ok { + panic(pgerror.Newf( + pgcode.DatatypeMismatch, + "COALESCE types %s and %s cannot be matched", + t.TypedExprAt(i).ResolvedType(), typ, + )) + } args[i] = b.buildScalar(expr, inScope, nil, nil, colRefs) } out = b.factory.ConstructCoalesce(args) @@ -328,10 +324,19 @@ func (b *Builder) buildScalar( case *tree.IfExpr: valType := t.ResolvedType() input := b.buildScalar(t.Cond.(tree.TypedExpr), inScope, nil, nil, colRefs) - ifTrueExpr := tree.ReType(t.True.(tree.TypedExpr), valType) + // Re-typing the True expression should always succeed because they + // are given the same type during type-checking. + ifTrueExpr := reType(t.True.(tree.TypedExpr), valType) ifTrue := b.buildScalar(ifTrueExpr, inScope, nil, nil, colRefs) whens := memo.ScalarListExpr{b.factory.ConstructWhen(memo.TrueSingleton, ifTrue)} - orElseExpr := tree.ReType(t.Else.(tree.TypedExpr), valType) + orElseExpr, ok := tree.ReType(t.Else.(tree.TypedExpr), valType) + if !ok { + panic(pgerror.Newf( + pgcode.DatatypeMismatch, + "IF types %s and %s cannot be matched", + t.Else.(tree.TypedExpr).ResolvedType(), valType, + )) + } orElse := b.buildScalar(orElseExpr, inScope, nil, nil, colRefs) out = b.factory.ConstructCase(input, whens, orElse) @@ -343,7 +348,7 @@ func (b *Builder) buildScalar( out = b.factory.ConstructVariable(inScope.cols[t.Idx].id) case *tree.NotExpr: - input := b.buildScalar(tree.ReType(t.TypedInnerExpr(), types.Bool), inScope, nil, nil, colRefs) + input := b.buildScalar(reType(t.TypedInnerExpr(), types.Bool), inScope, nil, nil, colRefs) out = b.factory.ConstructNot(input) case *tree.IsNullExpr: @@ -368,7 +373,7 @@ func (b *Builder) buildScalar( // of the NULLIF expression so that type inference will be correct in the // CASE expression constructed below. For example, the type of // NULLIF(NULL, 0) should be int. - expr1 := tree.ReType(t.Expr1.(tree.TypedExpr), valType) + expr1 := reType(t.Expr1.(tree.TypedExpr), valType) input := b.buildScalar(expr1, inScope, nil, nil, colRefs) cond := b.buildScalar(t.Expr2.(tree.TypedExpr), inScope, nil, nil, colRefs) whens := memo.ScalarListExpr{ @@ -377,8 +382,8 @@ func (b *Builder) buildScalar( out = b.factory.ConstructCase(input, whens, input) case *tree.OrExpr: - left := b.buildScalar(tree.ReType(t.TypedLeft(), types.Bool), inScope, nil, nil, colRefs) - right := b.buildScalar(tree.ReType(t.TypedRight(), types.Bool), inScope, nil, nil, colRefs) + left := b.buildScalar(reType(t.TypedLeft(), types.Bool), inScope, nil, nil, colRefs) + right := b.buildScalar(reType(t.TypedRight(), types.Bool), inScope, nil, nil, colRefs) out = b.factory.ConstructOr(left, right) case *tree.ParenExpr: @@ -875,3 +880,20 @@ func (sb *ScalarBuilder) Build(expr tree.Expr) (err error) { sb.factory.Memo().SetScalarRoot(scalar) return nil } + +// reType is similar to tree.ReType, except that it panics with an internal +// error if the expression cannot be re-typed. This should only be used when +// re-typing is expected to always be successful. For example, it is used to +// re-type the left and right children of an OrExpr to booleans, which should +// always succeed during the optbuild phase because type-checking has already +// validated the types of the children. +func reType(expr tree.TypedExpr, typ *types.T) tree.TypedExpr { + retypedExpr, ok := tree.ReType(expr, typ) + if !ok { + panic(errors.AssertionFailedf( + "expected successful retype from %s to %s", + expr.ResolvedType(), typ, + )) + } + return retypedExpr +} diff --git a/pkg/sql/opt/optbuilder/testdata/scalar b/pkg/sql/opt/optbuilder/testdata/scalar index 231a462d813d..886b67011409 100644 --- a/pkg/sql/opt/optbuilder/testdata/scalar +++ b/pkg/sql/opt/optbuilder/testdata/scalar @@ -1494,9 +1494,47 @@ is [type=bool] build SELECT CASE WHEN false THEN ARRAY[('', 0)] ELSE ARRAY[]::RECORD[] END ---- -error (42804): CASE types tuple[] and tuple{string, int}[] cannot be matched +error (42804): CASE ELSE type tuple[] cannot be matched to WHEN type tuple{string, int}[] build SELECT CASE WHEN false THEN ARRAY[('', 0)] WHEN true THEN ARRAY[]::RECORD[] ELSE ARRAY[('', 0)] END ---- -error (42804): CASE types tuple[] and tuple{string, int}[] cannot be matched +error (42804): CASE WHEN types tuple[] and tuple{string, int}[] cannot be matched + +# Regression test for #76807. Do not create invalid casts when building COALESCE +# and IF expressions. +build +SELECT COALESCE(t.v, ARRAY[]:::RECORD[]) +FROM (VALUES (ARRAY[(1, 'foo')])) AS t(v) +---- +error (42804): COALESCE types tuple[] and tuple{int, string}[] cannot be matched + +build +SELECT COALESCE(ARRAY[]:::RECORD[], t.v) +FROM (VALUES (ARRAY[(1, 'foo')])) AS t(v) +---- +project + ├── columns: coalesce:2 + ├── values + │ ├── columns: column1:1 + │ └── (ARRAY[(1, 'foo')],) + └── projections + └── COALESCE(ARRAY[], column1:1::RECORD[]) [as=coalesce:2] + +build +SELECT IF(true, t.v, ARRAY[]:::RECORD[]) +FROM (VALUES (ARRAY[(1, 'foo')])) AS t(v) +---- +error (42804): IF types tuple[] and tuple{int, string}[] cannot be matched + +build +SELECT IF(true, ARRAY[]:::RECORD[], t.v) +FROM (VALUES (ARRAY[(1, 'foo')])) AS t(v) +---- +project + ├── columns: if:2 + ├── values + │ ├── columns: column1:1 + │ └── (ARRAY[(1, 'foo')],) + └── projections + └── CASE WHEN true THEN ARRAY[] ELSE column1:1::RECORD[] END [as=if:2] diff --git a/pkg/sql/opt/optbuilder/window.go b/pkg/sql/opt/optbuilder/window.go index 7bde10553c05..d5047e858d14 100644 --- a/pkg/sql/opt/optbuilder/window.go +++ b/pkg/sql/opt/optbuilder/window.go @@ -332,7 +332,7 @@ func (b *Builder) getTypedWindowArgs(w *windowInfo) []tree.TypedExpr { argExprs = append(argExprs, tree.NewDInt(1)) } if len(argExprs) < 3 { - null := tree.ReType(tree.DNull, argExprs[0].ResolvedType()) + null := reType(tree.DNull, argExprs[0].ResolvedType()) argExprs = append(argExprs, null) } } diff --git a/pkg/sql/sem/tree/cast.go b/pkg/sql/sem/tree/cast.go index b8854f12afa2..d66ff8b47ef7 100644 --- a/pkg/sql/sem/tree/cast.go +++ b/pkg/sql/sem/tree/cast.go @@ -1283,7 +1283,11 @@ func ValidCast(src, tgt *types.T, ctx CastContext) bool { // If src and tgt are tuple types, check for a valid cast between each // corresponding tuple element. - if srcFamily == types.TupleFamily && tgtFamily == types.TupleFamily { + // + // Casts from a tuple type to AnyTuple are a no-op so they are always valid. + // If tgt is AnyTuple, we continue to lookupCast below which contains a + // special case for these casts. + if srcFamily == types.TupleFamily && tgtFamily == types.TupleFamily && tgt != types.AnyTuple { srcTypes := src.TupleContents() tgtTypes := tgt.TupleContents() // The tuple types must have the same number of elements. @@ -1375,6 +1379,15 @@ func lookupCast(src, tgt *types.T, intervalStyleEnabled, dateStyleEnabled bool) }, true } + // Casts from any tuple type to AnyTuple are no-ops, so they are implicit + // and immutable. + if srcFamily == types.TupleFamily && tgt == types.AnyTuple { + return cast{ + maxContext: CastContextImplicit, + volatility: VolatilityImmutable, + }, true + } + // Casts from string types to array and tuple types are stable and allowed // in explicit contexts. if srcFamily == types.StringFamily && diff --git a/pkg/sql/sem/tree/constant_eval.go b/pkg/sql/sem/tree/constant_eval.go index ae4d92ced7ab..e0bb4b7b089e 100644 --- a/pkg/sql/sem/tree/constant_eval.go +++ b/pkg/sql/sem/tree/constant_eval.go @@ -10,6 +10,8 @@ package tree +import "github.com/cockroachdb/errors" + // ConstantEvalVisitor replaces constant TypedExprs with the result of Eval. type ConstantEvalVisitor struct { ctx *EvalContext @@ -58,7 +60,12 @@ func (v *ConstantEvalVisitor) VisitPost(expr Expr) Expr { if value == DNull { // We don't want to return an expression that has a different type; cast // the NULL if necessary. - return ReType(DNull, typedExpr.ResolvedType()) + retypedNull, ok := ReType(DNull, typedExpr.ResolvedType()) + if !ok { + v.err = errors.AssertionFailedf("failed to retype NULL to %s", typedExpr.ResolvedType()) + return expr + } + return retypedNull } return value } diff --git a/pkg/sql/sem/tree/normalize.go b/pkg/sql/sem/tree/normalize.go index d0765aa39822..e76b9951af01 100644 --- a/pkg/sql/sem/tree/normalize.go +++ b/pkg/sql/sem/tree/normalize.go @@ -135,25 +135,25 @@ func (expr *BinaryExpr) normalize(v *NormalizeVisitor) TypedExpr { switch expr.Operator.Symbol { case treebin.Plus: if v.isNumericZero(right) { - final = ReType(left, expectedType) + final, _ = ReType(left, expectedType) break } if v.isNumericZero(left) { - final = ReType(right, expectedType) + final, _ = ReType(right, expectedType) break } case treebin.Minus: if types.IsAdditiveType(left.ResolvedType()) && v.isNumericZero(right) { - final = ReType(left, expectedType) + final, _ = ReType(left, expectedType) break } case treebin.Mult: if v.isNumericOne(right) { - final = ReType(left, expectedType) + final, _ = ReType(left, expectedType) break } if v.isNumericOne(left) { - final = ReType(right, expectedType) + final, _ = ReType(right, expectedType) break } // We can't simplify multiplication by zero to zero, @@ -161,11 +161,13 @@ func (expr *BinaryExpr) normalize(v *NormalizeVisitor) TypedExpr { // the result must be NULL. case treebin.Div, treebin.FloorDiv: if v.isNumericOne(right) { - final = ReType(left, expectedType) + final, _ = ReType(left, expectedType) break } } + // final is nil when the binary expression did not match the cases above, + // or when ReType was unsuccessful. if final == nil { return expr } @@ -710,7 +712,12 @@ func (v *NormalizeVisitor) VisitPost(expr Expr) Expr { if value == DNull { // We don't want to return an expression that has a different type; cast // the NULL if necessary. - return ReType(DNull, expr.(TypedExpr).ResolvedType()) + retypedNull, ok := ReType(DNull, expr.(TypedExpr).ResolvedType()) + if !ok { + v.err = errors.AssertionFailedf("failed to retype NULL to %s", expr.(TypedExpr).ResolvedType()) + return expr + } + return retypedNull } return value } @@ -942,14 +949,23 @@ func init() { DecimalOne.SetInt64(1) } -// ReType ensures that the given expression evaluates -// to the requested type, inserting a cast if necessary. -func ReType(expr TypedExpr, wantedType *types.T) TypedExpr { +// ReType ensures that the given expression evaluates to the requested type, +// wrapping the expression in a cast if necessary. Returns ok=false if a cast +// cannot wrap the expression because no valid cast from the expression's type +// to the wanted type exists. +func ReType(expr TypedExpr, wantedType *types.T) (_ TypedExpr, ok bool) { resolvedType := expr.ResolvedType() if wantedType.Family() == types.AnyFamily || resolvedType.Identical(wantedType) { - return expr + return expr, true + } + // TODO(#75103): For legacy reasons, we check for a valid cast in the most + // permissive context, CastContextExplicit. To be consistent with Postgres, + // we should check for a valid cast in the most restrictive context, + // CastContextImplicit. + if !ValidCast(resolvedType, wantedType, CastContextExplicit) { + return nil, false } res := &CastExpr{Expr: expr, Type: wantedType} res.typ = wantedType - return res + return res, true } From 82e0b121c715c59cebbb8d53e29edf7952d6913f Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Fri, 11 Mar 2022 11:33:02 -0700 Subject: [PATCH 4/4] ci: ensure all nightlies post github issues when tests fail Release justification: ensure failing nightlies post github issues Release note: None --- build/teamcity/cockroach/nightlies/compose.sh | 21 +++++++-- .../cockroach/nightlies/lint_urls_impl.sh | 19 ++++++-- .../nightlies/optimizer_tests_impl.sh | 45 +++++++++++++++---- .../nightlies/random_syntax_tests_impl.sh | 17 ++++++- .../nightlies/sqlite_logic_test_impl.sh | 17 ++++++- 5 files changed, 100 insertions(+), 19 deletions(-) diff --git a/build/teamcity/cockroach/nightlies/compose.sh b/build/teamcity/cockroach/nightlies/compose.sh index da364dde1468..ca2e3639b225 100755 --- a/build/teamcity/cockroach/nightlies/compose.sh +++ b/build/teamcity/cockroach/nightlies/compose.sh @@ -4,22 +4,35 @@ set -xeuo pipefail dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))" source "$dir/teamcity-support.sh" +source "$dir/teamcity-bazel-support.sh" tc_start_block "Run compose tests" -bazel build //pkg/cmd/bazci --config=ci -BAZCI=$(bazel info bazel-bin --config=ci)/pkg/cmd/bazci/bazci_/bazci +bazel build //pkg/cmd/bazci //pkg/cmd/github-post //pkg/cmd/testfilter --config=ci +BAZEL_BIN=$(bazel info bazel-bin --config=ci) +BAZCI=$BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci bazel build //pkg/cmd/cockroach //pkg/compose/compare/compare:compare_test --config=ci --config=crosslinux --config=test --config=with_ui CROSSBIN=$(bazel info bazel-bin --config=ci --config=crosslinux --config=test --config=with_ui) COCKROACH=$CROSSBIN/pkg/cmd/cockroach/cockroach_/cockroach COMPAREBIN=$(bazel run //pkg/compose/compare/compare:compare_test --config=ci --config=crosslinux --config=test --config=with_ui --run_under=realpath | grep '^/' | tail -n1) +ARTIFACTS_DIR=$PWD/artifacts -$BAZCI run --config=ci --config=test --artifacts_dir=$PWD/artifacts \ +GO_TEST_JSON_OUTPUT_FILE=$PWD/artifacts/test.json.txt +exit_status=0 +$BAZCI run --config=ci --config=test --artifacts_dir=$ARTIFACTS_DIR \ //pkg/compose:compose_test -- \ --test_env=GO_TEST_WRAP_TESTV=1 \ + --test_env=GO_TEST_JSON_OUTPUT_FILE=$GO_TEST_JSON_OUTPUT_FILE \ --test_arg -cockroach --test_arg $COCKROACH \ --test_arg -compare --test_arg $COMPAREBIN \ - --test_timeout=1800 + --test_timeout=1800 || exit_status=$? +process_test_json \ + $BAZEL_BIN/pkg/cmd/testfilter/testfilter_/testfilter \ + $BAZEL_BIN/pkg/cmd/github-post/github-post_/github-post \ + $ARTIFACTS_DIR \ + $GO_TEST_JSON_OUTPUT_FILE \ + $exit_status tc_end_block "Run compose tests" +exit $exit_status diff --git a/build/teamcity/cockroach/nightlies/lint_urls_impl.sh b/build/teamcity/cockroach/nightlies/lint_urls_impl.sh index f16e64c305c0..86ad6627bf74 100755 --- a/build/teamcity/cockroach/nightlies/lint_urls_impl.sh +++ b/build/teamcity/cockroach/nightlies/lint_urls_impl.sh @@ -2,10 +2,23 @@ set -xeuo pipefail -bazel build //pkg/cmd/bazci --config=ci +dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))" +source "$dir/teamcity-bazel-support.sh" # For process_test_json + +bazel build //pkg/cmd/bazci //pkg/cmd/github-post //pkg/cmd/testfilter --config=ci +BAZEL_BIN=$(bazel info bazel-bin --config=ci) +GO_TEST_JSON_OUTPUT_FILE=/artifacts/test.json.txt +exit_status=0 XML_OUTPUT_FILE=/artifacts/test.xml GO_TEST_WRAP_TESTV=1 GO_TEST_WRAP=1 bazel \ run --config=ci --config=test --define gotags=bazel,gss,nightly \ - //build/bazelutil:lint + //build/bazelutil:lint || exit_status=$? # The schema of the output test.xml will be slightly wrong -- ask `bazci` to fix # it up. -$(bazel info bazel-bin --config=ci)/pkg/cmd/bazci/bazci_/bazci munge-test-xml /artifacts/test.xml +$BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci munge-test-xml /artifacts/test.xml +process_test_json \ + $BAZEL_BIN/pkg/cmd/testfilter/testfilter_/testfilter \ + $BAZEL_BIN/pkg/cmd/github-post/github-post_/github-post \ + /artifacts \ + $GO_TEST_JSON_OUTPUT_FILE \ + $exit_status +exit $exit_status diff --git a/build/teamcity/cockroach/nightlies/optimizer_tests_impl.sh b/build/teamcity/cockroach/nightlies/optimizer_tests_impl.sh index 576878195efd..c34c6de4835a 100755 --- a/build/teamcity/cockroach/nightlies/optimizer_tests_impl.sh +++ b/build/teamcity/cockroach/nightlies/optimizer_tests_impl.sh @@ -3,26 +3,55 @@ set -xeuo pipefail dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))" +source "$dir/teamcity-bazel-support.sh" source "$dir/teamcity/util.sh" -bazel build //pkg/cmd/bazci --config=ci +bazel build //pkg/cmd/bazci //pkg/cmd/github-post //pkg/cmd/testfilter --config=ci BAZEL_BIN=$(bazel info bazel-bin --config=ci) tc_start_block "Run opt tests with fast_int_set_large" -$BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci --config=ci \ +ARTIFACTS_DIR=/artifacts/fast_int_set_large +mkdir $ARTIFACTS_DIR +GO_TEST_JSON_OUTPUT_FILE=$ARTIFACTS_DIR/test.json.txt +exit_status_large=0 +$BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci --config=ci --artifacts $ARTIFACTS_DIR \ test //pkg/sql/opt:opt_test -- \ - --define gotags=bazel,crdb_test,fast_int_set_large -mkdir /artifacts/fast_int_set_large -for FILE in $(ls artifacts | grep -v '^fast_int_set_large$'); do mv /artifacts/$FILE /artifacts/fast_int_set_large; done + --define gotags=bazel,crdb_test,fast_int_set_large \ + --test_env=GO_TEST_JSON_OUTPUT_FILE=$GO_TEST_JSON_OUTPUT_FILE || $exit_status_large=$? +process_test_json \ + $BAZEL_BIN/pkg/cmd/testfilter/testfilter_/testfilter \ + $BAZEL_BIN/pkg/cmd/github-post/github-post_/github-post \ + $ARTIFACTS_DIR \ + $GO_TEST_JSON_OUTPUT_FILE \ + $exit_status_large tc_end_block "Run opt tests with fast_int_set_large" # NOTE(ricky): Running both tests in the same configuration with different # gotags thrashes the cache. These tests are pretty quick so it shouldn't # matter now but it is something to keep an eye on. tc_start_block "Run opt tests with fast_int_set_small" +ARTIFACTS_DIR=/artifacts/fast_int_set_small +mkdir $ARTIFACTS_DIR +GO_TEST_JSON_OUTPUT_FILE=$ARTIFACTS_DIR/test.json.txt +exit_status_small=0 $BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci --config=ci \ test //pkg/sql/opt:opt_test -- \ - --define gotags=bazel,crdb_test,fast_int_set_small -mkdir /artifacts/fast_int_set_small -for FILE in $(ls artifacts | grep -v '^fast_int_set'); do mv /artifacts/$FILE /artifacts/fast_int_set_small; done + --define gotags=bazel,crdb_test,fast_int_set_small \ + --test_env=GO_TEST_JSON_OUTPUT_FILE=$GO_TEST_JSON_OUTPUT_FILE || $exit_status_small=$? +process_test_json \ + $BAZEL_BIN/pkg/cmd/testfilter/testfilter_/testfilter \ + $BAZEL_BIN/pkg/cmd/github-post/github-post_/github-post \ + $ARTIFACTS_DIR \ + $GO_TEST_JSON_OUTPUT_FILE \ + $exit_status_large tc_end_block "Run opt tests with fast_int_set_small" + +if [ $exit_status_large -ne 0 ] +then + exit $exit_status_large +fi + +if [ $exit_status_small -ne 0 ] +then + exit $exit_status_small +fi diff --git a/build/teamcity/cockroach/nightlies/random_syntax_tests_impl.sh b/build/teamcity/cockroach/nightlies/random_syntax_tests_impl.sh index 4145a6b57235..da9f2d516e01 100755 --- a/build/teamcity/cockroach/nightlies/random_syntax_tests_impl.sh +++ b/build/teamcity/cockroach/nightlies/random_syntax_tests_impl.sh @@ -2,9 +2,22 @@ set -xeuo pipefail -bazel build //pkg/cmd/bazci --config=ci +dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))" +source "$dir/teamcity-bazel-support.sh" + +bazel build //pkg/cmd/bazci //pkg/cmd/github-post //pkg/cmd/testfilter --config=ci BAZEL_BIN=$(bazel info bazel-bin --config=ci) +GO_TEST_JSON_OUTPUT_FILE=/artifacts/test.json.txt +exit_status=0 $BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci --config=ci \ test //pkg/sql/tests:tests_test -- \ --test_arg -rsg=5m --test_arg -rsg-routines=8 --test_arg -rsg-exec-timeout=1m \ - --test_timeout 3600 --test_filter 'TestRandomSyntax' + --test_timeout 3600 --test_filter 'TestRandomSyntax' \ + --test_env=GO_TEST_JSON_OUTPUT_FILE=$GO_TEST_JSON_OUTPUT_FILE || exit_status=$? +process_test_json \ + $BAZEL_BIN/pkg/cmd/testfilter/testfilter_/testfilter \ + $BAZEL_BIN/pkg/cmd/github-post/github-post_/github-post \ + /artifacts \ + $GO_TEST_JSON_OUTPUT_FILE \ + $exit_status +exit $exit_status diff --git a/build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh b/build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh index 2bd508dca348..ada65331c5f7 100755 --- a/build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh +++ b/build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh @@ -2,10 +2,23 @@ set -xeuo pipefail -bazel build //pkg/cmd/bazci --config=ci +dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))" +source "$dir/teamcity-bazel-support.sh" + +bazel build //pkg/cmd/bazci //pkg/cmd/github-post //pkg/cmd/testfilter --config=ci BAZEL_BIN=$(bazel info bazel-bin --config=ci) +GO_TEST_JSON_OUTPUT_FILE=/artifacts/test.json.txt +exit_status=0 $BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci --config=ci \ test //pkg/sql/logictest:logictest_test -- \ --test_arg -bigtest --test_arg -flex-types \ --define gotags=bazel,crdb_test_off --test_timeout 86400 \ - --test_filter '^TestSqlLiteLogic$|^TestTenantSQLLiteLogic$' + --test_filter '^TestSqlLiteLogic$|^TestTenantSQLLiteLogic$' \ + --test_env=GO_TEST_JSON_OUTPUT_FILE=$GO_TEST_JSON_OUTPUT_FILE || $exit_status=$? +process_test_json \ + $BAZEL_BIN/pkg/cmd/testfilter/testfilter_/testfilter \ + $BAZEL_BIN/pkg/cmd/github-post/github-post_/github-post \ + /artifacts \ + $GO_TEST_JSON_OUTPUT_FILE \ + $exit_status +exit $exit_status