Merge cockroachdb#119605 cockroachdb#120418 cockroachdb#120424 cockro…

…achdb#120438

119605: cli,server: increase `GOGC` by default to 300%, add CLI flag r=nvanbenschoten a=nvanbenschoten

Closes cockroachdb#115164.

This commit introduces a new `--go-gc-percent` flag to CLI `start` command which controls the garbage collection target percentage on the Go runtime. The default value for the flag if neither the --go-gc-percent flag nor the GOGC env var is set is 300%.

To avoid introducing new OOMs, we only switch to this new default (up from 100%) if a soft memory limit is set, either manually or automatically.

Release note (cli change): A new flag `--go-gc-percent` is introduced to `start` command. It controls the garbage collection target percentage on the Go runtime, mirroring the existing GOGC environment variable. A garbage collection is triggered when the ratio of freshly allocated data to live data remaining after the previous collection reaches this percentage. If left unspecified and a Go soft memory limit is configured (i.e. not explicitly disabled via --max-go-memory nor GOMEMLIMIT), the GC target percentage defaults to 300%. If set to a negative value, disables the target percentage garbage collection heuristic, leaving only the soft memory limit heuristic to trigger garbage collection.


120418: parser: fix link for ALTER PARTITION r=rafiss a=rafiss

fixes cockroachdb#120096
Release note: None

120424: lint_urls: add test filter so we don't run all tests r=rafiss a=rickystewart

Epic: none
Release note: None

120438: rttanalysis: remove extra shards from test r=rafiss a=rickystewart

There is 1 test in this package, so having 16 shards makes no sense.

Epic: CRDB-8308
Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

build/teamcity/cockroach/nightlies/lint_urls_impl.sh

@@ -16,6 +16,7 @@ bazel build //pkg/cmd/bazci --config=ci
 $(bazel info bazel-bin --config=ci)/pkg/cmd/bazci/bazci_/bazci -- \
     test //pkg/testutils/lint:lint_test \
     --config=ci --define gotags=bazel,gss,nightly,lint \
+    --test_filter=TestNightlyLint \
     --test_env=CC=$(which gcc) \
     --test_env=CXX=$(which gcc) \
     --test_env=HOME \

pkg/bench/rttanalysis/BUILD.bazel

@@ -55,7 +55,6 @@ go_test(
     data = glob(["testdata/**"]),
     embed = [":rttanalysis"],
     exec_properties = {"test.Pool": "large"},
-    shard_count = 16,
     deps = [
         "//pkg/base",
         "//pkg/jobs/jobspb",

pkg/cli/cliflags/flags.go

@@ -191,6 +191,18 @@ metrics for the entire cluster. This capacity constraint does not affect
 SQL query execution.`,
 	}
 
+	GoGCPercent = FlagInfo{
+		Name: "go-gc-percent",
+		Description: `
+Garbage collection target percentage set on the Go runtime (which is also
+configurable via the GOGC environment variable, but --go-gc-percent has higher
+precedence if both are set). A garbage collection is triggered when the ratio of
+freshly allocated data to live data remaining after the previous collection
+reaches this percentage. If left unspecified, defaults to 300%. If set to a
+negative value, disables the target percentage garbage collection heuristic,
+leaving only the soft memory limit heuristic to trigger garbage collection.`,
+	}
+
 	SQLTempStorage = FlagInfo{
 		Name: "max-disk-temp-storage",
 		Description: `

pkg/cli/context.go

@@ -519,6 +519,10 @@ var startCtx struct {
 	goMemLimitValue          bytesOrPercentageValue
 	diskTempStorageSizeValue bytesOrPercentageValue
 	tsdbSizeValue            bytesOrPercentageValue
+
+	// goGCPercent is used to specify the runtime garbage collection target
+	// percentage. Also configurable with the GOGC environment variable.
+	goGCPercent int
 }
 
 // setStartContextDefaults set the default values in startCtx.  This
@@ -541,6 +545,7 @@ func setStartContextDefaults() {
 	startCtx.goMemLimitValue = makeBytesOrPercentageValue(&goMemLimit, memoryPercentResolver)
 	startCtx.diskTempStorageSizeValue = makeBytesOrPercentageValue(nil /* v */, nil /* percentResolver */)
 	startCtx.tsdbSizeValue = makeBytesOrPercentageValue(&serverCfg.TimeSeriesServerConfig.QueryMemoryMax, memoryPercentResolver)
+	startCtx.goGCPercent = 0
 }
 
 // drainCtx captures the command-line parameters of the `node drain`

pkg/cli/flags.go

@@ -512,6 +512,7 @@ func init() {
 		cliflagcfg.VarFlag(f, &startCtx.sqlSizeValue, cliflags.SQLMem)
 		cliflagcfg.VarFlag(f, &startCtx.goMemLimitValue, cliflags.GoMemLimit)
 		cliflagcfg.VarFlag(f, &startCtx.tsdbSizeValue, cliflags.TSDBMem)
+		cliflagcfg.IntFlag(f, &startCtx.goGCPercent, cliflags.GoGCPercent)
 		// N.B. diskTempStorageSizeValue.Resolve() will be called after the
 		// stores flag has been parsed and the storage device that a
 		// percentage refers to becomes known.

pkg/cli/interactive_tests/test_flags.tcl

@@ -165,7 +165,7 @@ end_test
 
 stop_server $argv
 
-start_test "Check that set GOMEMLIMIT env var without specifying --max-go-memory works"
+start_test "Check that setting GOMEMLIMIT env var without specifying --max-go-memory works"
 send "export GOMEMLIMIT=1GiB;\r"
 eexpect ":/# "
 send "$argv start-single-node --insecure --store=path=logs/mystore\r"
@@ -175,5 +175,15 @@ eexpect ":/# "
 stop_server $argv
 end_test
 
+start_test "Check that setting GOGC env var without specifying --go-gc-percent works"
+send "export GOGC=500;\r"
+eexpect ":/# "
+send "$argv start-single-node --insecure --store=path=logs/mystore\r"
+eexpect "node starting"
+interrupt
+eexpect ":/# "
+stop_server $argv
+end_test
+
 send "exit 0\r"
 eexpect eof

pkg/cli/start.go

@@ -489,16 +489,18 @@ func runStartInternal(
 	// GOMAXPROCS(0) by now.
 	cgroups.AdjustMaxProcs(ctx)
 
+	fs := cliflagcfg.FlagSetForCmd(cmd)
+
 	// Check the --join flag.
-	if fl := cliflagcfg.FlagSetForCmd(cmd).Lookup(cliflags.Join.Name); fl != nil && !fl.Changed {
+	if fl := fs.Lookup(cliflags.Join.Name); fl != nil && !fl.Changed {
 		err := errors.WithHint(
 			errors.New("no --join flags provided to 'cockroach start'"),
 			"Consider using 'cockroach init' or 'cockroach start-single-node' instead")
 		return err
 	}
 
 	// Check the --tenant-id-file flag.
-	if fl := cliflagcfg.FlagSetForCmd(cmd).Lookup(cliflags.TenantIDFile.Name); fl != nil && fl.Changed {
+	if fl := fs.Lookup(cliflags.TenantIDFile.Name); fl != nil && fl.Changed {
 		fileName := fl.Value.String()
 		serverCfg.DelayedSetTenantID = func(ctx context.Context) (roachpb.TenantID, error) {
 			return tenantIDFromFile(ctx, fileName, nil, nil, nil)
@@ -555,6 +557,42 @@ func runStartInternal(
 		return err
 	}
 
+	// Set the GC target percent on the Go runtime.
+	if err := func() error {
+		var goGCPercent int
+		if fs.Changed(cliflags.GoGCPercent.Name) {
+			goGCPercent = startCtx.goGCPercent
+		} else {
+			if _, envVarSet := envutil.ExternalEnvString("GOGC", 1); envVarSet {
+				// When --go-gc-percent is not specified but the env var is, we defer to
+				// the env var.
+				return nil
+			}
+			// When neither the --go-gc-percent flag nor the GOGC env var is set,
+			// increase the GC target percent to 300% (default 100%) to reduce the
+			// frequency of GC cycles. However, only do so if a soft memory limit is
+			// also configured, to avoid introducing OOMs.
+			goMemLimit := debug.SetMemoryLimit(-1 /* get without adjusting */)
+			if goMemLimit == math.MaxInt64 {
+				// If the soft memory limit is disabled, don't adjust the GC percent.
+				// Leave it at the default 100%.
+				return nil
+			}
+			goGCPercent = 300
+		}
+		var goGCPercentStr redact.RedactableString
+		if goGCPercent < 0 {
+			goGCPercentStr = `"off"`
+		} else {
+			goGCPercentStr = redact.Sprintf("%d%%", goGCPercent)
+		}
+		log.Ops.Infof(ctx, "GC target percentage of Go runtime is set to %s", goGCPercentStr)
+		debug.SetGCPercent(goGCPercent)
+		return nil
+	}(); err != nil {
+		return err
+	}
+
 	// Initialize the node's configuration from startup parameters.
 	// This also reads the part of the configuration that comes from
 	// environment variables.

pkg/sql/parser/sql.y

@@ -1930,7 +1930,7 @@ alter_table_stmt:
 //   USING <var> = COPY FROM PARENT [, ...]
 //   { TO | = } <expr>
 //
-// %SeeAlso: WEBDOCS/configure-zone.html
+// %SeeAlso: WEBDOCS/alter-partition.html
 alter_partition_stmt:
   alter_zone_partition_stmt
 | ALTER PARTITION error // SHOW HELP: ALTER PARTITION

pkg/testutils/lint/lint_test.go

@@ -540,7 +540,7 @@ func TestLint(t *testing.T) {
 					":!util/grpcutil",                        // GRPC_GO_* variables
 					":!roachprod",                            // roachprod requires AWS environment variables
 					":!cli/env.go",                           // The CLI needs the PGHOST variable.
-					":!cli/start.go",                         // The CLI needs the GOMEMLIMIT variable.
+					":!cli/start.go",                         // The CLI needs the GOMEMLIMIT and GOGC variables.
 					":!internal/codeowners/codeowners.go",    // For BAZEL_TEST.
 					":!internal/team/team.go",                // For BAZEL_TEST.
 					":!util/log/test_log_scope.go",           // For TEST_UNDECLARED_OUTPUT_DIR, REMOTE_EXEC