Merge #40880 #40910

40880: sqltelemetry,sql: Add telemetry for partitioning related improvements r=rohany a=rohany

This PR adds telemetry for the ALTER PARTITION FROM INDEX t@* command,
as well as sets up some boilerplate code for adding more partitioning
specific telemetry counters.

We only have one thing that we are recording telemetry of relating to partitioning right now, but I thought it was good to just set up the boilerplate so that we could add some more counters in the future if desired.

Work for https://github.com/cockroachlabs/registration/issues/228.

Release justification: Low risk monitoring improvement.

Release note: None

40910: row: check for uninitialized KVFetcher r=jordanlewis a=yuzefovich

Previously, when GetRangesInfo() and GetBytesRead() was called on
row.Fetcher before the underlying KVFetcher is initialized, it
would panic due to NPE. Now this is fixed. The problem was introduced
by the recent refactor of moving CFetcher from sql/row into sql/colexec.

Fixes: #39013.

Release justification: Release blocker.

Release note: None

Co-authored-by: Rohan Yadav <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

pkg/ccl/logictestccl/testdata/logic_test/zone

@@ -638,6 +638,11 @@ CREATE INDEX x2 ON t2 (x) PARTITION BY LIST (x) (
 statement ok
 ALTER PARTITION p1 OF INDEX t2@* CONFIGURE ZONE USING num_replicas = 1
 
+query T
+SELECT feature_name FROM crdb_internal.feature_usage WHERE feature_name='sql.partitioning.alter-all-partitions' AND usage_count > 0
+----
+sql.partitioning.alter-all-partitions
+
 statement error index "t2" does not exist\nHINT: try specifying the index as <tablename>@<indexname>
 ALTER PARTITION p1 OF INDEX t2 CONFIGURE ZONE USING num_replicas = 1
 

pkg/sql/row/fetcher.go

@@ -1423,7 +1423,7 @@ func (rf *Fetcher) PartialKey(nCols int) (roachpb.Key, error) {
 // GetRangesInfo returns information about the ranges where the rows came from.
 // The RangeInfo's are deduped and not ordered.
 func (rf *Fetcher) GetRangesInfo() []roachpb.RangeInfo {
-	f := rf.kvFetcher.kvBatchFetcher
+	f := rf.kvFetcher
 	if f == nil {
 		// Not yet initialized.
 		return nil
@@ -1433,7 +1433,12 @@ func (rf *Fetcher) GetRangesInfo() []roachpb.RangeInfo {
 
 // GetBytesRead returns total number of bytes read by the underlying KVFetcher.
 func (rf *Fetcher) GetBytesRead() int64 {
-	return rf.kvFetcher.bytesRead
+	f := rf.kvFetcher
+	if f == nil {
+		// Not yet initialized.
+		return 0
+	}
+	return f.bytesRead
 }
 
 // Only unique secondary indexes have extra columns to decode (namely the

pkg/sql/row/fetcher_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/stretchr/testify/assert"
 )
 
 type initFetcherArgs struct {
@@ -1052,3 +1053,12 @@ func TestRowFetcherReset(t *testing.T) {
 func idLookupKey(tableID TableID, indexID sqlbase.IndexID) uint64 {
 	return (uint64(tableID) << 32) | uint64(indexID)
 }
+
+func TestFetcherUninitialized(t *testing.T) {
+	// Regression test for #39013: make sure it's okay to call GetRangesInfo and
+	// GetBytesReader even before the fetcher was fully initialized.
+	var fetcher Fetcher
+
+	assert.Nil(t, fetcher.GetRangesInfo())
+	assert.Zero(t, fetcher.GetBytesRead())
+}

pkg/sql/set_zone_config.go

@@ -24,6 +24,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
@@ -270,6 +271,7 @@ func (n *setZoneConfigNode) startExec(params runParams) error {
 	// of them.
 	var specifiers []tree.ZoneSpecifier
 	if n.zoneSpecifier.TargetsPartition() && n.allIndexes {
+		sqltelemetry.IncrementPartitioningCounter(sqltelemetry.AlterAllPartitions)
 		for _, idx := range table.AllNonDropIndexes() {
 			if p := idx.FindPartitionByName(string(n.zoneSpecifier.Partition)); p != nil {
 				zs := n.zoneSpecifier

pkg/sql/sqltelemetry/partitioning.go

@@ -0,0 +1,49 @@
+// Copyright 2019 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 sqltelemetry
+
+import (
+	"fmt"
+
+	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
+)
+
+// PartitioningTelemetryType is an enum used to represent the different partitioning related operations
+// that we are recording telemetry for.
+type PartitioningTelemetryType int
+
+const (
+	_ PartitioningTelemetryType = iota
+	// AlterAllPartitions represents an ALTER ALL PARTITIONS statement (ALTER PARTITION OF INDEX t@*)
+	AlterAllPartitions
+)
+
+var partitioningTelemetryMap = map[PartitioningTelemetryType]string{
+	AlterAllPartitions: "alter-all-partitions",
+}
+
+func (p PartitioningTelemetryType) String() string {
+	return partitioningTelemetryMap[p]
+}
+
+var partitioningTelemetryCounters map[PartitioningTelemetryType]telemetry.Counter
+
+func init() {
+	partitioningTelemetryCounters = make(map[PartitioningTelemetryType]telemetry.Counter)
+	for ty, s := range partitioningTelemetryMap {
+		partitioningTelemetryCounters[ty] = telemetry.GetCounterOnce(fmt.Sprintf("sql.partitioning.%s", s))
+	}
+}
+
+// IncrementPartitioningCounter is used to increment the telemetry counter for a particular partitioning operation.
+func IncrementPartitioningCounter(partitioningType PartitioningTelemetryType) {
+	telemetry.Inc(partitioningTelemetryCounters[partitioningType])
+}