Merge #56600 #56627

56600: roachpb: remove SetInner in favor of MustSetInner r=nvanbenschoten a=tbg

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



56627: sql: rework SHOW REGIONS to SHOW REGIONS FROM CLUSTER r=ajstorm a=otan

Resolves #56331 

Release note (sql change): SHOW REGIONS functionality is now deferred to
SHOW REGIONS FROM CLUSTER.

Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>

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 ::=

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\}"

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
 		}
 	}

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

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 }
 

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{})

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()
 }
 

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

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 := `

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}

pkg/sql/parser/parse_test.go

@@ -646,7 +646,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`},

pkg/sql/parser/sql.y

@@ -5125,10 +5125,10 @@ show_ranges_stmt:
 // %Help: SHOW REGIONS - shows regions
 // %Category: DDL
 // %Text:
-// SHOW REGIONS
-// SHOW REGIONS FOR DATABASE <database>
+// SHOW REGIONS FROM CLUSTER
+// SHOW REGIONS FROM DATABASE <database>
 show_regions_stmt:
-  SHOW REGIONS
+  SHOW REGIONS FROM CLUSTER
   {
     $$.val = &tree.ShowRegions{}
   }
@@ -5138,6 +5138,7 @@ show_regions_stmt:
       Database: tree.Name($5),
     }
   }
+| SHOW REGIONS error // SHOW HELP: SHOW REGIONS
 
 show_locality_stmt:
   SHOW LOCALITY

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")
 	}
 }
 

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 {