Merge cockroachdb#85419 cockroachdb#85689 cockroachdb#85939 cockroach…

…db#86065

85419: tracing: fix lazytag export for Jaegar/otel r=aadityasondhi a=aadityasondhi

Previously, lazytags would not render in the otel export to
Jaeger. This was because otel doesn't support an on-demand
nature of the lazytags in its interface.

This change manually adds lazytags as otel tags before an otel
span is finished, allowing them to render in Jaeger after they
are exported.

Resolves: cockroachdb#85166

Release note: None

Release justification: bug fixes

85689: lint: add a linter to prohibit map[...]bool in favor of map[...]struct{} r=yuzefovich a=yuzefovich

The linter is currently limited to `sql/opt/norm` package. This commit
was prompted by an article mentioned in the Golang Weekly newsletter.
The rationale is that `map[...]struct{}` is more efficient, but in some
cases the bool map value is actually desired, so this commit introduces
an opt-out `//nolint:maptobool` comment.

Release justification: low-risk performance improvement.

Release note: None

85939: clusterversion,kvserver,sql: remove AutoStatsTableSettings r=msirek a=celiala

This commit removes the 22.1 `AutoStatsTableSettings` version gate.

Cleanup was done following guidance from [21.2 cleanup](cockroachdb#74270 (comment)):

> For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

> However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses cockroachdb#80663

Release note: None
Release justification: remove old version gates.

86065: admission: carve out some new files r=irfansharif a=irfansharif

Pure code movement. Trying to see what components in this package could be
reasoned about independently to readers less familiar with this package.
Some of these may benefit from being pulled into sub-packages, but for now
separate files are sufficient. We're hurting some git history tracking
with this PR, but hopefully they lead to more understandability with future
work. It's also better to do it now before the release branch is cut to
make backports easier. If we pull them into sub packages next, git tracking
will then come for free.

```
admission: move ioLoadListener into its own file
admission: move GrantCoordinator into its own file
admission: move kvSlotAdjuster into its own file
admission: move sqlNodeCPUOverloadIndicator into its own file
admission: move tokensLinearModel into its own file
admission: add top-level admission.go
admission: move store{AdmissionStats,RequestEstimates}..
admission: segment test files by central type
```

I'm not breaking things up into subpackages in this PR, but this paves the
way for that. The structure I'm imagining is roughly:

    pkg/util/admission/
    ├── admission.go
    ├── admissionpb/
    ├── diskbandwidthlimiter/
    ├── diskloadwatcher/
    ├── grantcoordinator/
    ├── granter/
    ├── ioloadlistener/
    ├── kvslotadjuster/
    ├── sqlcpuoverloadindicator/
    ├── storeworkqueue/
    └── tokenlinearmodel/

Where the sub-packages house concrete implementations of interfaces
defined in admission.go, corresponding tests, and constructors for those
types. They depend on the base level pkg/util/admission package and talk
to the other subpackages through interfaces. The existing interfaces are
already written in a manner to make this happen. For other examples of
this pattern, look at pkg/{upgrade,spanconfig}.

Release justification: low-risk / high-benefit (pure code movement, will make backports easier)


Co-authored-by: Aaditya Sondhi <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Celia La <[email protected]>
Co-authored-by: irfan sharif <[email protected]>

pkg/clusterversion/cockroach_versions.go

@@ -196,9 +196,6 @@ const (
 	// EnableNewStoreRebalancer enables the new store rebalancer introduced in
 	// 22.1.
 	EnableNewStoreRebalancer
-	// AutoStatsTableSettings is the version where we allow auto stats related
-	// table settings.
-	AutoStatsTableSettings
 	// EnableNewChangefeedOptions enables the usage of new changefeed options
 	// such as end_time, initial_scan_only, and setting the value of initial_scan
 	// to 'yes|no|only'
@@ -388,10 +385,6 @@ var versionsSingleton = keyedVersions{
 		Key:     EnableNewStoreRebalancer,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 96},
 	},
-	{
-		Key:     AutoStatsTableSettings,
-		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 100},
-	},
 	{
 		Key:     EnableNewChangefeedOptions,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 106},

pkg/kv/kvserver/allocator/allocatorimpl/BUILD.bazel

@@ -11,7 +11,6 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/allocatorimpl",
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/clusterversion",
         "//pkg/gossip",
         "//pkg/kv/kvserver/allocator",
         "//pkg/kv/kvserver/allocator/storepool",

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go

@@ -19,7 +19,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/storepool"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/constraint"
@@ -1604,12 +1603,8 @@ func (a *Allocator) leaseholderShouldMoveDueToPreferences(
 // stores as targets) when enforcementLevel is set to storeHealthNoAction or
 // storeHealthLogOnly. By default storeHealthBlockRebalanceTo is the action taken. When
 // there is a mixed version cluster, storeHealthNoAction is set instead.
-func (a *Allocator) StoreHealthOptions(ctx context.Context) StoreHealthOptions {
-	enforcementLevel := StoreHealthNoAction
-	if a.StorePool.St.Version.IsActive(ctx, clusterversion.AutoStatsTableSettings) {
-		enforcementLevel = StoreHealthEnforcement(l0SublevelsThresholdEnforce.Get(&a.StorePool.St.SV))
-	}
-
+func (a *Allocator) StoreHealthOptions(_ context.Context) StoreHealthOptions {
+	enforcementLevel := StoreHealthEnforcement(l0SublevelsThresholdEnforce.Get(&a.StorePool.St.SV))
 	return StoreHealthOptions{
 		EnforcementLevel:    enforcementLevel,
 		L0SublevelThreshold: l0SublevelsThreshold.Get(&a.StorePool.St.SV),

pkg/sql/alter_table.go

@@ -732,7 +732,6 @@ func (n *alterTableNode) startExec(params runParams) error {
 			}
 
 		case *tree.AlterTableSetStorageParams:
-			oldTableHasAutoStatsSettings := n.tableDesc.GetAutoStatsSettings() != nil
 			var ttlBefore *catpb.RowLevelTTL
 			if ttl := n.tableDesc.GetRowLevelTTL(); ttl != nil {
 				ttlBefore = protoutil.Clone(ttl).(*catpb.RowLevelTTL)
@@ -757,15 +756,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 				return err
 			}
 
-			newTableHasAutoStatsSettings := n.tableDesc.GetAutoStatsSettings() != nil
-			if err := checkDisallowedAutoStatsSettingChange(
-				params, oldTableHasAutoStatsSettings, newTableHasAutoStatsSettings,
-			); err != nil {
-				return err
-			}
-
 		case *tree.AlterTableResetStorageParams:
-			oldTableHasAutoStatsSettings := n.tableDesc.GetAutoStatsSettings() != nil
 			var ttlBefore *catpb.RowLevelTTL
 			if ttl := n.tableDesc.GetRowLevelTTL(); ttl != nil {
 				ttlBefore = protoutil.Clone(ttl).(*catpb.RowLevelTTL)
@@ -789,13 +780,6 @@ func (n *alterTableNode) startExec(params runParams) error {
 				return err
 			}
 
-			newTableHasAutoStatsSettings := n.tableDesc.GetAutoStatsSettings() != nil
-			if err := checkDisallowedAutoStatsSettingChange(
-				params, oldTableHasAutoStatsSettings, newTableHasAutoStatsSettings,
-			); err != nil {
-				return err
-			}
-
 		case *tree.AlterTableRenameColumn:
 			descChanged, err := params.p.renameColumn(params.ctx, n.tableDesc, t.Column, t.NewName)
 			if err != nil {
@@ -1992,21 +1976,6 @@ func handleTTLStorageParamChange(
 	return nil
 }
 
-func checkDisallowedAutoStatsSettingChange(
-	params runParams, oldTableHasAutoStatsSettings, newTableHasAutoStatsSettings bool,
-) error {
-	if !oldTableHasAutoStatsSettings && !newTableHasAutoStatsSettings {
-		// Do not have to do anything here.
-		return nil
-	}
-
-	if err := checkAutoStatsTableSettingsEnabledForCluster(params.ctx, params.p.ExecCfg().Settings); err != nil {
-		return err
-	}
-
-	return nil
-}
-
 // tryRemoveFKBackReferences determines whether the provided unique constraint
 // is used on the referencing side of a FK constraint. If so, it tries to remove
 // the references or find an alternate unique constraint that will suffice.

pkg/sql/create_table.go

@@ -1467,12 +1467,6 @@ func NewTableDesc(
 		primaryIndexColumnSet[string(regionalByRowCol)] = struct{}{}
 	}
 
-	if autoStatsSettings := desc.GetAutoStatsSettings(); autoStatsSettings != nil {
-		if err := checkAutoStatsTableSettingsEnabledForCluster(ctx, st); err != nil {
-			return nil, err
-		}
-	}
-
 	// Create the TTL automatic column (crdb_internal_expiration) if one does not already exist.
 	if ttl := desc.GetRowLevelTTL(); ttl != nil && ttl.HasDurationExpr() {
 		hasRowLevelTTLColumn := false
@@ -2419,16 +2413,6 @@ func newRowLevelTTLScheduledJob(
 	return sj, nil
 }
 
-func checkAutoStatsTableSettingsEnabledForCluster(ctx context.Context, st *cluster.Settings) error {
-	if !st.Version.IsActive(ctx, clusterversion.AutoStatsTableSettings) {
-		return pgerror.Newf(
-			pgcode.FeatureNotSupported,
-			"auto stats table settings are only available once the cluster is fully upgraded",
-		)
-	}
-	return nil
-}
-
 // CreateRowLevelTTLScheduledJob creates a new row-level TTL schedule.
 func CreateRowLevelTTLScheduledJob(
 	ctx context.Context,

pkg/sql/opt/norm/groupby_funcs.go

@@ -219,7 +219,7 @@ func (c *CustomFuncs) AreValuesDistinct(
 func (c *CustomFuncs) areRowsDistinct(
 	rows memo.ScalarListExpr, cols opt.ColList, groupingCols opt.ColSet, nullsAreDistinct bool,
 ) bool {
-	var seen map[string]bool
+	var seen map[string]struct{}
 	var encoded []byte
 	for _, scalar := range rows {
 		row := scalar.(*memo.TupleExpr)
@@ -263,7 +263,7 @@ func (c *CustomFuncs) areRowsDistinct(
 		}
 
 		if seen == nil {
-			seen = make(map[string]bool, len(rows))
+			seen = make(map[string]struct{}, len(rows))
 		}
 
 		// Determine whether key has already been seen.
@@ -274,7 +274,7 @@ func (c *CustomFuncs) areRowsDistinct(
 		}
 
 		// Add the key to the seen map.
-		seen[key] = true
+		seen[key] = struct{}{}
 	}
 
 	return true

pkg/testutils/lint/lint_test.go

@@ -2073,6 +2073,47 @@ func TestLint(t *testing.T) {
 		}
 	})
 
+	// TestMapToBool asserts that map[...]bool is not used. In most cases, such
+	// a map can be replaced with map[...]struct{} that is more efficient, and
+	// this linter nudges folks to do so. This linter can be disabled by
+	// '//nolint:maptobool' comment.
+	// TODO(yuzefovich): expand the scope where the linter is applied.
+	t.Run("TestMapToBool", func(t *testing.T) {
+		t.Parallel()
+		cmd, stderr, filter, err := dirCmd(
+			pkgDir,
+			"git",
+			"grep",
+			"-nE",
+			`map\[.*\]bool`,
+			"--",
+			"sql/opt/norm*.go",
+			":!*_test.go",
+		)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if err := cmd.Start(); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := stream.ForEach(stream.Sequence(
+			filter,
+			stream.GrepNot(`nolint:maptobool`),
+		), func(s string) {
+			t.Errorf("\n%s <- forbidden; use map[...]struct{} instead", s)
+		}); err != nil {
+			t.Error(err)
+		}
+
+		if err := cmd.Wait(); err != nil {
+			if out := stderr.String(); len(out) > 0 {
+				t.Fatalf("err=%s, stderr=%s", err, out)
+			}
+		}
+	})
+
 	// RoachVet is expensive memory-wise and thus should not run with t.Parallel().
 	// RoachVet includes all of the passes of `go vet` plus first-party additions.
 	// See pkg/cmd/roachvet.

pkg/util/admission/BUILD.bazel

@@ -4,10 +4,15 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 go_library(
     name = "admission",
     srcs = [
+        "admission.go",
         "disk_bandwidth.go",
-        "doc.go",
+        "grant_coordinator.go",
         "granter.go",
+        "io_load_listener.go",
+        "kv_slot_adjuster.go",
+        "sql_cpu_overload_indicator.go",
         "store_token_estimation.go",
+        "tokens_linear_model.go",
         "work_queue.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/util/admission",
@@ -35,7 +40,9 @@ go_test(
     srcs = [
         "disk_bandwidth_test.go",
         "granter_test.go",
+        "io_load_listener_test.go",
         "store_token_estimation_test.go",
+        "tokens_linear_model_test.go",
         "work_queue_test.go",
     ],
     data = glob(["testdata/**"]),