Merge #68929 #69359 #69516 #69566 #69567

68929: opt: add AOST to ANALYZE r=mgartner a=mgartner

A long running `ANALYZE` can result in a GC TTL error like:

    batch timestamp 1628283892.315459871,0 must be after replica GC threshold 1628283892.326056946,0

This commit prevents this error by adding `AS OF SYSTEM TIME '-0.001ms'`
automatically to all `ANALYZE` statements. This triggers use of
inconsistent scans. See the sql.stats.max_timestamp_age setting.

Fixes #68590

Release note (bug fix): Long running `ANALYZE` statements will no longer
result in GC TTL errors.

Release justification: This is a low-risk fix for the `ANALYZE`
statement.


69359: roachtest: clarify when to regenerate fixtures r=celiala a=celiala

As per noted in #69340 (comment), this PR rewords verMap comment slightly to clarify that fixture generation only needs to happen if a new key is added (e.g. for a new release).

Release justification: non-production code change
Release note: None

69516: roachtest: disable ballast during disk-stall roachtest r=nicktrav a=jbowens

The charybdefs FUSE filesystem used in the disk-stall roachtest slows
down file writes. Creating a default 1 GiB emergency ballast via the
FUSE filesystem was prohibitively slow.

Fix #64240.
Fix #58238.

Release justification: non-production code changes
Release note: none

69566: cli: make unsafe-remove-dead-replicas work with encrypted stores r=sumeerbhola a=sumeerbhola

Release justification: Low-risk bug fix to existing functionality.
Release note: None

69567: admission: fix overflow bug in ioLoadListener r=sumeerbhola a=sumeerbhola

Fixes #69461,#69463

Release justification: High-severity bug fix to new funtionality.

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Celia La <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>

pkg/cli/debug.go

@@ -1410,11 +1410,12 @@ var DebugCmdsForRocksDB = []*cobra.Command{
 	debugCheckStoreCmd,
 	debugCompactCmd,
 	debugGCCmd,
+	debugIntentCount,
 	debugKeysCmd,
 	debugRaftLogCmd,
 	debugRangeDataCmd,
 	debugRangeDescriptorsCmd,
-	debugIntentCount,
+	debugUnsafeRemoveDeadReplicasCmd,
 }
 
 // All other debug commands go here.
@@ -1428,7 +1429,6 @@ var debugCmds = append(DebugCmdsForRocksDB,
 	debugTimeSeriesDumpCmd,
 	debugSyncBenchCmd,
 	debugSyncTestCmd,
-	debugUnsafeRemoveDeadReplicasCmd,
 	debugEnvCmd,
 	debugZipCmd,
 	debugMergeLogsCmd,

pkg/cmd/roachtest/tests/disk_stall.go

@@ -106,7 +106,10 @@ func runDiskStalledDetection(
 	go func() {
 		t.WorkerStatus("running server")
 		out, err := c.RunWithBuffer(ctx, l, n,
-			fmt.Sprintf("timeout --signal 9 %ds env COCKROACH_ENGINE_MAX_SYNC_DURATION_DEFAULT=%s COCKROACH_LOG_MAX_SYNC_DURATION=%s "+
+			fmt.Sprintf("timeout --signal 9 %ds env "+
+				"COCKROACH_ENGINE_MAX_SYNC_DURATION_DEFAULT=%s "+
+				"COCKROACH_LOG_MAX_SYNC_DURATION=%s "+
+				"COCKROACH_AUTO_BALLAST=false "+
 				"./cockroach start-single-node --insecure --store {store-dir}/%s --log '{sinks: {stderr: {filter: INFO}}, file-defaults: {dir: \"{store-dir}/%s\"}}'",
 				int(dur.Seconds()), maxDataSync, maxLogSync, dataDir, logDir,
 			),

pkg/cmd/roachtest/tests/predecessor_version.go

@@ -28,11 +28,13 @@ func PredecessorVersion(buildVersion version.Version) (string, error) {
 
 	buildVersionMajorMinor := fmt.Sprintf("%d.%d", buildVersion.Major(), buildVersion.Minor())
 
-	// NB: you can update the values in this map to point at newer patch
-	// releases. You will need to run acceptance/version-upgrade with the
-	// checkpoint option enabled to create the missing store directory fixture
-	// (see runVersionUpgrade). The same is true for adding a new key to this
-	// map.
+	// You can update the values in verMap to point at newer patch releases.
+	//
+	// NB: If a new key was added (e.g. if you're releasing a new major
+	// release), you'll also need to regenerate fixtures. To regenerate
+	// fixtures, you will need to run acceptance/version-upgrade with the
+	// checkpoint option enabled to create the missing store directory
+	// fixture (see runVersionUpgrade).
 	verMap := map[string]string{
 		"21.2": "21.1.7",
 		"21.1": "20.2.12",

pkg/sql/distsql_plan_stats.go

@@ -44,7 +44,7 @@ const histogramSamples = 10000
 // stats collection, used when creating statistics AS OF SYSTEM TIME. The
 // timestamp is advanced during long operations as needed. See TableReaderSpec.
 //
-// The lowest TTL we recommend is 10 minutes. This value must be be lower than
+// The lowest TTL we recommend is 10 minutes. This value must be lower than
 // that.
 var maxTimestampAge = settings.RegisterDurationSetting(
 	"sql.stats.max_timestamp_age",

pkg/sql/opt/optbuilder/builder.go

@@ -333,8 +333,18 @@ func (b *Builder) buildStmt(
 		return b.buildCreateStatistics(stmt, inScope)
 
 	case *tree.Analyze:
-		// ANALYZE is syntactic sugar for CREATE STATISTICS.
-		return b.buildCreateStatistics(&tree.CreateStats{Table: stmt.Table}, inScope)
+		// ANALYZE is syntactic sugar for CREATE STATISTICS. We add AS OF SYSTEM
+		// TIME '-0.001ms' to trigger use of inconsistent scans. This prevents
+		// GC TTL errors during ANALYZE. See the sql.stats.max_timestamp_age
+		// setting.
+		return b.buildCreateStatistics(&tree.CreateStats{
+			Table: stmt.Table,
+			Options: tree.CreateStatsOptions{
+				AsOf: tree.AsOfClause{
+					Expr: tree.NewStrVal("-0.001ms"),
+				},
+			},
+		}, inScope)
 
 	case *tree.Export:
 		return b.buildExport(stmt, inScope)

pkg/sql/opt/optbuilder/testdata/misc_statements

@@ -189,4 +189,4 @@ build
 ANALYZE ab
 ----
 create-statistics
- └── CREATE STATISTICS "" FROM ab
+ └── CREATE STATISTICS "" FROM ab WITH OPTIONS AS OF SYSTEM TIME '-0.001ms'

pkg/util/admission/granter.go

@@ -1488,7 +1488,9 @@ func (io *ioLoadListener) pebbleMetricsTick(m pebble.Metrics) {
 // 1s.
 func (io *ioLoadListener) allocateTokensTick() {
 	var toAllocate int64
-	if io.totalTokens == unlimitedTokens {
+	// unlimitedTokens==MaxInt64, so avoid overflow in the rounding up
+	// calculation.
+	if io.totalTokens >= unlimitedTokens-(adjustmentInterval-1) {
 		toAllocate = io.totalTokens / adjustmentInterval
 	} else {
 		// Round up so that we don't accumulate tokens to give in a burst on the

pkg/util/admission/granter_test.go

@@ -13,6 +13,7 @@ package admission
 import (
 	"context"
 	"fmt"
+	"math"
 	"sort"
 	"strings"
 	"testing"
@@ -385,6 +386,26 @@ func TestIOLoadListener(t *testing.T) {
 		})
 }
 
+func TestIOLoadListenerOverflow(t *testing.T) {
+	req := &testRequesterForIOLL{}
+	kvGranter := &testGranterWithIOTokens{}
+	st := cluster.MakeTestingClusterSettings()
+	ioll := ioLoadListener{
+		settings:    st,
+		kvRequester: req,
+	}
+	ioll.mu.Mutex = &syncutil.Mutex{}
+	ioll.mu.kvGranter = kvGranter
+	for i := int64(0); i < adjustmentInterval; i++ {
+		// Override the totalTokens manually to trigger the overflow bug.
+		ioll.totalTokens = math.MaxInt64 - i
+		ioll.tokensAllocated = 0
+		for j := 0; j < adjustmentInterval; j++ {
+			ioll.allocateTokensTick()
+		}
+	}
+}
+
 // TODO(sumeer):
 // - Test metrics
 // - Test GrantCoordinator with multi-tenant configurations