From 8e58c1ee8e56bfbb7d7bae4b2df731ed55e1658f Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 13 Nov 2020 12:11:26 +1100 Subject: [PATCH 1/2] sql: rework SHOW REGIONS to SHOW REGIONS FROM CLUSTER Release note (sql change): SHOW REGIONS functionality is now deferred to SHOW REGIONS FROM CLUSTER. --- docs/generated/sql/bnf/stmt_block.bnf | 2 +- .../interactive_tests/test_demo_node_cmds.tcl | 2 +- pkg/sql/delegate/show_regions.go | 12 ++++++- .../logictest/testdata/logic_test/multiregion | 2 +- pkg/sql/parser/parse_test.go | 2 +- pkg/sql/parser/sql.y | 7 +++-- pkg/sql/sem/tree/show.go | 6 ++-- pkg/sql/sqltelemetry/show.go | 31 ++++++++++--------- 8 files changed, 40 insertions(+), 24 deletions(-) diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index 53a460beed56..e759d3330ff3 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -664,7 +664,7 @@ show_range_for_row_stmt ::= | 'SHOW' 'RANGE' 'FROM' 'INDEX' table_index_name 'FOR' 'ROW' '(' expr_list ')' show_regions_stmt ::= - 'SHOW' 'REGIONS' + 'SHOW' 'REGIONS' 'FROM' 'CLUSTER' | 'SHOW' 'REGIONS' 'FROM' 'DATABASE' database_name show_roles_stmt ::= diff --git a/pkg/cli/interactive_tests/test_demo_node_cmds.tcl b/pkg/cli/interactive_tests/test_demo_node_cmds.tcl index 0b4827326185..81c1cef41cb3 100644 --- a/pkg/cli/interactive_tests/test_demo_node_cmds.tcl +++ b/pkg/cli/interactive_tests/test_demo_node_cmds.tcl @@ -82,7 +82,7 @@ eexpect "internal server error: tier must be in the form \"key=value\" not \"bla send "\\demo add region=ca-central,zone=a\r" eexpect "node 6 has been added with locality \"region=ca-central,zone=a\"" -send "show regions;\r" +send "show regions from cluster;\r" eexpect "ca-central | \{a\}" eexpect "us-east1 | \{b,c,d\}" eexpect "us-west1 | \{a,b\}" diff --git a/pkg/sql/delegate/show_regions.go b/pkg/sql/delegate/show_regions.go index 98274b54cc18..3ce76fddcc5c 100644 --- a/pkg/sql/delegate/show_regions.go +++ b/pkg/sql/delegate/show_regions.go @@ -13,11 +13,21 @@ package delegate import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" ) // delegateShowRanges implements the SHOW REGIONS statement. func (d *delegator) delegateShowRegions(n *tree.ShowRegions) (tree.Statement, error) { - sqltelemetry.IncrementShowCounter(sqltelemetry.Regions) + + if n.Database != "" { + sqltelemetry.IncrementShowCounter(sqltelemetry.RegionsFromDatabase) + return nil, unimplemented.New( + "show regions from database", + "SHOW REGIONS FROM DATABASE not yet implemented", + ) + } + sqltelemetry.IncrementShowCounter(sqltelemetry.RegionsFromCluster) + // TODO (storm): Change this so that it doesn't use hard-coded strings and is // more flexible for custom named sub-regions. query := ` diff --git a/pkg/sql/logictest/testdata/logic_test/multiregion b/pkg/sql/logictest/testdata/logic_test/multiregion index b592e64bf530..3363ff8cd18d 100644 --- a/pkg/sql/logictest/testdata/logic_test/multiregion +++ b/pkg/sql/logictest/testdata/logic_test/multiregion @@ -1,7 +1,7 @@ # LogicTest: multiregion-9node-3region-3azs query TT colnames -SHOW REGIONS +SHOW REGIONS FROM CLUSTER ---- region zones test1 {test1-az1,test1-az2,test1-az3} diff --git a/pkg/sql/parser/parse_test.go b/pkg/sql/parser/parse_test.go index 22420c8deb5f..2f18400c5115 100644 --- a/pkg/sql/parser/parse_test.go +++ b/pkg/sql/parser/parse_test.go @@ -645,7 +645,7 @@ func TestParse(t *testing.T) { {`SHOW RANGES FROM INDEX t@i`}, {`SHOW RANGES FROM INDEX d.i`}, {`SHOW RANGES FROM INDEX i`}, - {`SHOW REGIONS`}, + {`SHOW REGIONS FROM CLUSTER`}, {`SHOW REGIONS FROM DATABASE d`}, {`SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE d.t`}, {`SHOW ZONE CONFIGURATIONS`}, diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index d1b35dfee8bb..5fcea7b37edc 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -5124,10 +5124,10 @@ show_ranges_stmt: // %Help: SHOW REGIONS - shows regions // %Category: DDL // %Text: -// SHOW REGIONS -// SHOW REGIONS FOR DATABASE +// SHOW REGIONS FROM CLUSTER +// SHOW REGIONS FROM DATABASE show_regions_stmt: - SHOW REGIONS + SHOW REGIONS FROM CLUSTER { $$.val = &tree.ShowRegions{} } @@ -5137,6 +5137,7 @@ show_regions_stmt: Database: tree.Name($5), } } +| SHOW REGIONS error // SHOW HELP: SHOW REGIONS show_locality_stmt: SHOW LOCALITY diff --git a/pkg/sql/sem/tree/show.go b/pkg/sql/sem/tree/show.go index 4d1faacc96ef..240845ab7343 100644 --- a/pkg/sql/sem/tree/show.go +++ b/pkg/sql/sem/tree/show.go @@ -285,10 +285,12 @@ type ShowRegions struct { // Format implements the NodeFormatter interface. func (node *ShowRegions) Format(ctx *FmtCtx) { - ctx.WriteString("SHOW REGIONS") + ctx.WriteString("SHOW REGIONS ") if node.Database != "" { - ctx.WriteString(" FROM DATABASE ") + ctx.WriteString("FROM DATABASE ") node.Database.Format(ctx) + } else { + ctx.WriteString("FROM CLUSTER") } } diff --git a/pkg/sql/sqltelemetry/show.go b/pkg/sql/sqltelemetry/show.go index ec7a8dc4044a..05683785293c 100644 --- a/pkg/sql/sqltelemetry/show.go +++ b/pkg/sql/sqltelemetry/show.go @@ -24,8 +24,10 @@ const ( _ ShowTelemetryType = iota // Ranges represents the SHOW RANGES command. Ranges - // Regions represents the SHOW REGIONS command. - Regions + // RegionsFromCluster represents the SHOW REGIONS FROM CLUSTER command. + RegionsFromCluster + // RegionsFromDatabase represents the SHOW REGIONS FROM DATABASE command. + RegionsFromDatabase // Partitions represents the SHOW PARTITIONS command. Partitions // Locality represents the SHOW LOCALITY command. @@ -49,18 +51,19 @@ const ( ) var showTelemetryNameMap = map[ShowTelemetryType]string{ - Ranges: "ranges", - Partitions: "partitions", - Locality: "locality", - Create: "create", - RangeForRow: "rangeforrow", - Regions: "regions", - Queries: "queries", - Indexes: "indexes", - Constraints: "constraints", - Jobs: "jobs", - Roles: "roles", - Schedules: "schedules", + Ranges: "ranges", + Partitions: "partitions", + Locality: "locality", + Create: "create", + RangeForRow: "rangeforrow", + RegionsFromCluster: "regions_from_cluster", + RegionsFromDatabase: "regions_from_database", + Queries: "queries", + Indexes: "indexes", + Constraints: "constraints", + Jobs: "jobs", + Roles: "roles", + Schedules: "schedules", } func (s ShowTelemetryType) String() string { From ab0b87adee4d505f1670596e2274c89d3231aefe Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 12 Nov 2020 07:18:09 +0100 Subject: [PATCH 2/2] roachpb: remove SetInner in favor of MustSetInner As of a recent commit, `ErrorDetail.SetInner` became unused, and we can switch to a `MustSetInner` pattern for `ErrorDetail`. Since the codegen involved is shared with {Request,Response}Union, those lose the `SetInner` setter as well; we were always asserting on the returned bool there anyway so this isn't changing anything. Release note: None --- .../kvcoord/txn_interceptor_committer.go | 2 +- .../kvcoord/txn_interceptor_span_refresher.go | 2 +- pkg/roachpb/api.go | 20 ---------------- pkg/roachpb/api_test.go | 2 +- pkg/roachpb/batch_generated.go | 24 +++++++++---------- pkg/roachpb/errors.go | 10 ++++---- pkg/roachpb/gen_batch.go | 18 +++++++------- 7 files changed, 28 insertions(+), 50 deletions(-) diff --git a/pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go b/pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go index efbf7f87d1ea..e725324d6dd2 100644 --- a/pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go +++ b/pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go @@ -163,7 +163,7 @@ func (tc *txnCommitter) SendLocked( // Make a copy of the EndTxn, since we're going to change it below to // disable the parallel commit. etCpy := *et - ba.Requests[len(ba.Requests)-1].SetInner(&etCpy) + ba.Requests[len(ba.Requests)-1].MustSetInner(&etCpy) et = &etCpy } } diff --git a/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go b/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go index 4009a4373f4c..6043188d86f8 100644 --- a/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go +++ b/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go @@ -185,7 +185,7 @@ func (sr *txnSpanRefresher) SendLocked( isReissue := et.DeprecatedCanCommitAtHigherTimestamp if isReissue { etCpy := *et - ba.Requests[len(ba.Requests)-1].SetInner(&etCpy) + ba.Requests[len(ba.Requests)-1].MustSetInner(&etCpy) et = &etCpy } et.DeprecatedCanCommitAtHigherTimestamp = ba.CanForwardReadTimestamp diff --git a/pkg/roachpb/api.go b/pkg/roachpb/api.go index aa796141ab51..931f0c1596cb 100644 --- a/pkg/roachpb/api.go +++ b/pkg/roachpb/api.go @@ -597,26 +597,6 @@ func (sr *ReverseScanResponse) Verify(req Request) error { return nil } -// MustSetInner sets the Request contained in the union. It panics if the -// request is not recognized by the union type. The RequestUnion is reset -// before being repopulated. -func (ru *RequestUnion) MustSetInner(args Request) { - ru.Reset() - if !ru.SetInner(args) { - panic(errors.AssertionFailedf("%T excludes %T", ru, args)) - } -} - -// MustSetInner sets the Response contained in the union. It panics if the -// response is not recognized by the union type. The ResponseUnion is reset -// before being repopulated. -func (ru *ResponseUnion) MustSetInner(reply Response) { - ru.Reset() - if !ru.SetInner(reply) { - panic(errors.AssertionFailedf("%T excludes %T", ru, reply)) - } -} - // Method implements the Request interface. func (*GetRequest) Method() Method { return Get } diff --git a/pkg/roachpb/api_test.go b/pkg/roachpb/api_test.go index d74be872d8e0..6c26f6d638ac 100644 --- a/pkg/roachpb/api_test.go +++ b/pkg/roachpb/api_test.go @@ -224,7 +224,7 @@ func TestMustSetInner(t *testing.T) { req := RequestUnion{} res := ResponseUnion{} - // GetRequest is checked first in the generated code for SetInner. + // GetRequest is checked first in the generated code for MustSetInner. req.MustSetInner(&GetRequest{}) res.MustSetInner(&GetResponse{}) req.MustSetInner(&EndTxnRequest{}) diff --git a/pkg/roachpb/batch_generated.go b/pkg/roachpb/batch_generated.go index db4de754ddb7..5f0b865c6e3c 100644 --- a/pkg/roachpb/batch_generated.go +++ b/pkg/roachpb/batch_generated.go @@ -263,8 +263,9 @@ func (ru ResponseUnion) GetInner() Response { } } -// SetInner sets the error in the union. -func (ru *ErrorDetail) SetInner(r error) bool { +// MustSetInner sets the error in the union. +func (ru *ErrorDetail) MustSetInner(r error) { + ru.Reset() var union isErrorDetail_Value switch t := r.(type) { case *NotLeaseHolderError: @@ -324,14 +325,14 @@ func (ru *ErrorDetail) SetInner(r error) bool { case *IndeterminateCommitError: union = &ErrorDetail_IndeterminateCommit{t} default: - return false + panic(fmt.Sprintf("unsupported type %T for %T", r, ru)) } ru.Value = union - return true } -// SetInner sets the Request in the union. -func (ru *RequestUnion) SetInner(r Request) bool { +// MustSetInner sets the Request in the union. +func (ru *RequestUnion) MustSetInner(r Request) { + ru.Reset() var union isRequestUnion_Value switch t := r.(type) { case *GetRequest: @@ -423,14 +424,14 @@ func (ru *RequestUnion) SetInner(r Request) bool { case *AdminVerifyProtectedTimestampRequest: union = &RequestUnion_AdminVerifyProtectedTimestamp{t} default: - return false + panic(fmt.Sprintf("unsupported type %T for %T", r, ru)) } ru.Value = union - return true } -// SetInner sets the Response in the union. -func (ru *ResponseUnion) SetInner(r Response) bool { +// MustSetInner sets the Response in the union. +func (ru *ResponseUnion) MustSetInner(r Response) { + ru.Reset() var union isResponseUnion_Value switch t := r.(type) { case *GetResponse: @@ -520,10 +521,9 @@ func (ru *ResponseUnion) SetInner(r Response) bool { case *AdminVerifyProtectedTimestampResponse: union = &ResponseUnion_AdminVerifyProtectedTimestamp{t} default: - return false + panic(fmt.Sprintf("unsupported type %T for %T", r, ru)) } ru.Value = union - return true } type reqCounts [44]int32 diff --git a/pkg/roachpb/errors.go b/pkg/roachpb/errors.go index 6f1b10fd43a0..6a0a04994974 100644 --- a/pkg/roachpb/errors.go +++ b/pkg/roachpb/errors.go @@ -219,6 +219,9 @@ func (e *internalError) Error() string { } // ErrorDetailInterface is an interface for each error detail. +// These must not be implemented by anything other than our protobuf-backed error details +// as we rely on a 1:1 correspondence between the interface and what can be stored via +// `Error.SetDetail`. type ErrorDetailInterface interface { error protoutil.Message @@ -307,12 +310,7 @@ func (e *Error) SetDetail(detail ErrorDetailInterface) { } else { e.TransactionRestart = TransactionRestart_NONE } - // If the specific error type exists in the detail union, set it. - if !e.Detail.SetInner(detail) { - if e.TransactionRestart != TransactionRestart_NONE { - panic(errors.AssertionFailedf("transactionRestartError %T must be an ErrorDetail", detail)) - } - } + e.Detail.MustSetInner(detail) e.checkTxnStatusValid() } diff --git a/pkg/roachpb/gen_batch.go b/pkg/roachpb/gen_batch.go index 649eea337069..483b7f25013f 100644 --- a/pkg/roachpb/gen_batch.go +++ b/pkg/roachpb/gen_batch.go @@ -108,10 +108,11 @@ func (ru %[1]s) GetInner() %[2]s { `) } -func genSetInner(w io.Writer, unionName, variantName string, variants []variantInfo) { +func genMustSetInner(w io.Writer, unionName, variantName string, variants []variantInfo) { fmt.Fprintf(w, ` -// SetInner sets the %[2]s in the union. -func (ru *%[1]s) SetInner(r %[2]s) bool { +// MustSetInner sets the %[2]s in the union. +func (ru *%[1]s) MustSetInner(r %[2]s) { + ru.Reset() var union is%[1]s_Value switch t := r.(type) { `, unionName, variantName) @@ -123,10 +124,9 @@ func (ru *%[1]s) SetInner(r %[2]s) bool { } fmt.Fprint(w, ` default: - return false + panic(fmt.Sprintf("unsupported type %T for %T", r, ru)) } ru.Value = union - return true } `) } @@ -160,10 +160,10 @@ import ( genGetInner(f, "RequestUnion", "Request", reqVariants) genGetInner(f, "ResponseUnion", "Response", resVariants) - // Generate SetInner methods. - genSetInner(f, "ErrorDetail", "error", errVariants) - genSetInner(f, "RequestUnion", "Request", reqVariants) - genSetInner(f, "ResponseUnion", "Response", resVariants) + // Generate MustSetInner methods. + genMustSetInner(f, "ErrorDetail", "error", errVariants) + genMustSetInner(f, "RequestUnion", "Request", reqVariants) + genMustSetInner(f, "ResponseUnion", "Response", resVariants) fmt.Fprintf(f, ` type reqCounts [%d]int32