Merge #116013 #117743 #118001 #118687 #118746 #118758 #118770

116013: sql,opt: add hint for STRAIGHT join r=mgartner a=kevinmingtarja

Our current join hints (a INNER HASH JOIN b, a LEFT LOOKUP JOIN b, etc) fixes both the join order and the join algorithm. This commit adds the syntax and support for hinting the join order without hinting the join algorithm. This will be useful for the (few) cases where the optimizer processes the tables in a suboptimal order.

Resolves: #115308

Release note (sql change): It is now possible to hint to the optimizer that it should plan a straight join by using the syntax `... INNER STRAIGHT JOIN ...`. If the hint is provided, the optimizer will now fix the join order as given in the query, even if it estimates that a different plan using join reordering would have a lower cost.

117743: schemaworkload: Add metrics that keep track of workload operations r=fqazi a=rimadeodhar

This PR adds metrics that indicate operations performed by the schema change workload. These metrics are output in logs at the end of a workload run. These metrics will function as a sanity check to ensure that the workload is doing something meaningful and will need to be checked manually. The current set of metrics are fairly rudimentary but will help provide some initial insight into the efficacy of this workload.

Fixes: #81928
Epic: none
Release note: None

118001: spanconfig: correctly order updates when applying in the KVSubscriber r=arulajmani a=arulajmani

Previously, there was no ordering guarantee between KVSubscriber events at the same timestamp. As a result, if a batch of events included updates to overlapping spans at the same timestamp, we could apply additions before deletions -- this would cause the additions to get clobbered, which was not the intention. This could lead to missing span configurations, resulting in bugs such as the linked issue.

This patch fixes the issue by sorting deletions before additions if two span configuration events have the same timestamp.

Closes #110908

Release note (bug fix): Previously, altering from a Regional By Row table to a Regional By Table table could cause leaseholders to never move to the databse's primary region. This is now fixed.

118687: storage: check for replicated locks in MVCCClearTimeRange r=nvanbenschoten a=nvanbenschoten

Fixes #113116.

Previously, intents would be detected and returned in a `LockConflictError` from `MVCCClearTimeRange`. However, replicated locks with other strengths (shared, exclusive) would not. This commit fixes this, by calling ScanLocks at the beginning of the function.

In doing so, it also adjusts how `MVCCClearTimeRange` handles intents. Previously, the function would only return intents whose current timestamp was in the clearing time range. This was probably fine for intents at higher timestamps than the clearing time range. However, it is error-prone for intents at lower timestamps, as those could have later been resolved to a higher timestamp, into the cleared time range. In practice, this probably did not matter, because we only issue MVCCClearTimeRanges on offline keyspaces. For simplicity, we now return conflicts and kick off resolution for locks of any strength at any timestamp.

Release note: None

118746: cli: fix TestUnavailableZip failed r=koorosh a=koorosh

This patch fixes issue introduced in da0e542 where code (that handles unparsed entries due to errors) was added in "if error" block instead of below it. It was wrong because in case of timeout error, `entries` variable is always nil.
Now logic that handles partially failed entries parsing moved outside and invoked only if no timeout error happens.

Release note: None

Fixes: #117411

118758: build: ignore nogo rule `loopvarcapture` in external deps r=jayshrivastava a=jayshrivastava

This change updates the `loopvarcapture` nogo check to only check first-party code.

Release note: None
Epic: None

118770: bincheck: verbose mode r=celiala a=rail

This PR adds the -x flag to make the preparation steps visible in the logs.

Epic: none
Release note: None

Co-authored-by: Kevin Mingtarja <[email protected]>
Co-authored-by: rimadeodhar <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Andrii Vorobiov <[email protected]>
Co-authored-by: Jayant Shrivastava <[email protected]>
Co-authored-by: Rail Aliiev <[email protected]>

build/bazelutil/nogo_config.json

@@ -268,6 +268,12 @@
             "cockroach/bazel-out/.*/bin/pkg/.*$": "first-party code"
         }
     },
+    "loopvarcapture": {
+        "only_files": {
+            "cockroach/pkg/.*$": "first-party code",
+           "cockroach/bazel-out/.*/bin/pkg/.*$": "first-party code"
+       }
+    },
     "lostcancel": {
         "only_files": {
             "cockroach/pkg/.*$": "first-party code",

build/release/bincheck/download_binary.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-set -euo pipefail
+set -exuo pipefail
 
 download_and_extract() {
   cockroach_version=$1

build/release/bincheck/test-linux

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-set -euo pipefail
+set -exuo pipefail
 source ./download_binary.sh
 
 if [[ $# -ne 2 ]]

build/release/bincheck/test-macos-amd64

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-set -euo pipefail
+set -exuo pipefail
 source ./download_binary.sh
 
 # Verify arguments.

build/release/bincheck/test-macos-arm64

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-set -euo pipefail
+set -exuo pipefail
 source ./download_binary.sh
 
 # Verify arguments.

build/release/bincheck/test-windows

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-set -euo pipefail
+set -exuo pipefail
 source ./download_binary.sh
 
 if [[ $# -ne 2 ]]

docs/generated/sql/bnf/stmt_block.bnf

@@ -1436,6 +1436,7 @@ unreserved_keyword ::=
 	| 'STORE'
 	| 'STORED'
 	| 'STORING'
+	| 'STRAIGHT'
 	| 'STREAM'
 	| 'STRICT'
 	| 'SUBSCRIPTION'
@@ -4066,6 +4067,7 @@ bare_label_keywords ::=
 	| 'STORE'
 	| 'STORED'
 	| 'STORING'
+	| 'STRAIGHT'
 	| 'STREAM'
 	| 'STRICT'
 	| 'STRING'
@@ -4177,6 +4179,7 @@ opt_join_hint ::=
 	| 'MERGE'
 	| 'LOOKUP'
 	| 'INVERTED'
+	| 'STRAIGHT'
 	| 
 
 join_type ::=

pkg/BUILD.bazel

@@ -80,6 +80,7 @@ ALL_TESTS = [
     "//pkg/ccl/serverccl/statusccl:statusccl_test",
     "//pkg/ccl/serverccl:serverccl_test",
     "//pkg/ccl/spanconfigccl/spanconfigkvaccessorccl:spanconfigkvaccessorccl_test",
+    "//pkg/ccl/spanconfigccl/spanconfigkvsubscriberccl:spanconfigkvsubscriberccl_test",
     "//pkg/ccl/spanconfigccl/spanconfiglimiterccl:spanconfiglimiterccl_test",
     "//pkg/ccl/spanconfigccl/spanconfigreconcilerccl:spanconfigreconcilerccl_test",
     "//pkg/ccl/spanconfigccl/spanconfigsplitterccl:spanconfigsplitterccl_test",
@@ -917,6 +918,7 @@ GO_TARGETS = [
     "//pkg/ccl/serverccl:serverccl",
     "//pkg/ccl/serverccl:serverccl_test",
     "//pkg/ccl/spanconfigccl/spanconfigkvaccessorccl:spanconfigkvaccessorccl_test",
+    "//pkg/ccl/spanconfigccl/spanconfigkvsubscriberccl:spanconfigkvsubscriberccl_test",
     "//pkg/ccl/spanconfigccl/spanconfiglimiterccl:spanconfiglimiterccl_test",
     "//pkg/ccl/spanconfigccl/spanconfigreconcilerccl:spanconfigreconcilerccl_test",
     "//pkg/ccl/spanconfigccl/spanconfigsplitterccl:spanconfigsplitterccl_test",

pkg/ccl/multiregionccl/multiregionccltestutils/testutils.go

@@ -13,6 +13,7 @@ import (
 	gosql "database/sql"
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -26,6 +27,7 @@ type multiRegionTestClusterParams struct {
 	replicationMode base.TestClusterReplicationMode
 	useDatabase     string
 	settings        *cluster.Settings
+	scanInterval    time.Duration
 }
 
 // MultiRegionTestClusterParamsOption is an option that can be passed to
@@ -64,6 +66,14 @@ func WithSettings(settings *cluster.Settings) MultiRegionTestClusterParamsOption
 	}
 }
 
+// WithScanInterval is used to configure the scan interval for various KV
+// queues.
+func WithScanInterval(interval time.Duration) MultiRegionTestClusterParamsOption {
+	return func(params *multiRegionTestClusterParams) {
+		params.scanInterval = interval
+	}
+}
+
 // TestingCreateMultiRegionCluster creates a test cluster with numServers number
 // of nodes and the provided testing knobs applied to each of the nodes. Every
 // node is placed in its own locality, named "us-east1", "us-east2", and so on.
@@ -113,6 +123,7 @@ func TestingCreateMultiRegionClusterWithRegionList(
 				Locality: roachpb.Locality{
 					Tiers: []roachpb.Tier{{Key: "region", Value: region}},
 				},
+				ScanInterval: params.scanInterval,
 			}
 			totalServerCount++
 		}

pkg/ccl/spanconfigccl/spanconfigkvsubscriberccl/BUILD.bazel

@@ -0,0 +1,26 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_test")
+
+go_test(
+    name = "spanconfigkvsubscriberccl_test",
+    srcs = [
+        "kvsubscriber_test.go",
+        "main_test.go",
+    ],
+    deps = [
+        "//pkg/base",
+        "//pkg/ccl",
+        "//pkg/ccl/multiregionccl/multiregionccltestutils",
+        "//pkg/jobs",
+        "//pkg/kv/kvserver",
+        "//pkg/security/securityassets",
+        "//pkg/security/securitytest",
+        "//pkg/server",
+        "//pkg/testutils",
+        "//pkg/testutils/serverutils",
+        "//pkg/testutils/sqlutils",
+        "//pkg/testutils/testcluster",
+        "//pkg/util/leaktest",
+        "//pkg/util/log",
+        "//pkg/util/randutil",
+    ],
+)

pkg/ccl/spanconfigccl/spanconfigkvsubscriberccl/kvsubscriber_test.go

@@ -0,0 +1,79 @@
+// Copyright 2024 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package spanconfigkvsubscriberccl
+
+import (
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils"
+	"github.com/cockroachdb/cockroach/pkg/jobs"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+)
+
+// TestSpanConfigUpdatesApplyInCorrectOrder ensures the KVSubscriber applies
+// updates to its in-memory store in correct order. In particular, when there
+// are both deletions and additions at the same timestamp, the deletions should
+// be applied before the additions. This scenario is created by altering from a
+// regional by row table to a regional table -- doing so creates overlapping
+// updates with both deletions and additions with the same timestamo.
+//
+// Regression test for https://github.com/cockroachdb/cockroach/issues/110908.
+func TestSpanConfigUpdatesApplyInCorrectOrder(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	tc, _, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster(t, 3, base.TestingKnobs{
+		Store: &kvserver.StoreTestingKnobs{
+			DisableMergeQueue: true, // we don't want the partitions to merge away
+		},
+		JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(), // speed up schema changes.
+	}, multiregionccltestutils.WithScanInterval(50*time.Millisecond))
+	defer cleanup()
+	sqlDB := sqlutils.MakeSQLRunner(tc.ServerConn(0 /* idx */))
+
+	// Speed up the test.
+	sqlDB.Exec(t, `SET CLUSTER SETTING kv.closed_timestamp.target_duration = '50ms'`)
+	sqlDB.Exec(t, `SET CLUSTER SETTING kv.closed_timestamp.side_transport_interval = '50ms'`)
+	sqlDB.Exec(t, `SET CLUSTER SETTING kv.rangefeed.closed_timestamp_refresh_interval = '50ms'`)
+
+	sqlDB.Exec(t, `CREATE DATABASE mr PRIMARY REGION "us-east1" REGIONS "us-east2", "us-east3"; USE mr`)
+	sqlDB.Exec(t, `CREATE TABLE t() LOCALITY REGIONAL BY ROW`)
+
+	testutils.SucceedsSoon(t, func() error {
+		var count int
+		sqlDB.QueryRow(t,
+			"SELECT count(distinct lease_holder) from [show ranges from table t with details]",
+		).Scan(&count)
+		if count == 3 {
+			return nil
+		}
+		return fmt.Errorf("waiting for each region to pick up leaseholders; count %d", count)
+	})
+
+	sqlDB.Exec(t, `ALTER TABLE t SET LOCALITY REGIONAL BY TABLE`)
+	testutils.SucceedsSoon(t, func() error {
+		var count int
+		sqlDB.QueryRow(t,
+			"SELECT count(distinct lease_holder) from [show ranges from table t with details]",
+		).Scan(&count)
+		if count == 1 {
+			return nil
+		}
+		return fmt.Errorf(
+			"waiting for all partition leases to move to the primary region; number of regions %d", count,
+		)
+	})
+}

pkg/ccl/spanconfigccl/spanconfigkvsubscriberccl/main_test.go

@@ -0,0 +1,33 @@
+// Copyright 2024 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package spanconfigkvsubscriberccl
+
+import (
+	"os"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/ccl"
+	"github.com/cockroachdb/cockroach/pkg/security/securityassets"
+	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
+	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
+	"github.com/cockroachdb/cockroach/pkg/util/randutil"
+)
+
+//go:generate ../../../util/leaktest/add-leaktest.sh *_test.go
+
+func TestMain(m *testing.M) {
+	defer ccl.TestingEnableEnterprise()()
+	securityassets.SetLoader(securitytest.EmbeddedAssets)
+	randutil.SeedForTests()
+	serverutils.InitTestServerFactory(server.TestServerFactory)
+	serverutils.InitTestClusterFactory(testcluster.TestClusterFactory)
+	os.Exit(m.Run())
+}

pkg/cli/zip_per_node.go

@@ -472,19 +472,19 @@ func (zc *debugZipContext) collectPerNodeData(
 				if err := zc.z.createError(sf, name, requestErr); err != nil {
 					return err
 				}
-				// Log out the list of errors that occurred during log entries request.
-				if len(entries.ParseErrors) > 0 {
-					sf.shout("%d parsing errors occurred:", len(entries.ParseErrors))
-					for _, err := range entries.ParseErrors {
-						sf.shout("%s", err)
-					}
-					parseErr := fmt.Errorf("%d errors occurred:\n%s", len(entries.ParseErrors), strings.Join(entries.ParseErrors, "\n"))
-					if err := zc.z.createError(sf, name, parseErr); err != nil {
-						return err
-					}
-				}
 				continue
 			}
+			// Log the list of errors that occurred during log entries request.
+			if len(entries.ParseErrors) > 0 {
+				sf.shout("%d parsing errors occurred:", len(entries.ParseErrors))
+				for _, err := range entries.ParseErrors {
+					sf.shout("%s", err)
+				}
+				parseErr := fmt.Errorf("%d errors occurred:\n%s", len(entries.ParseErrors), strings.Join(entries.ParseErrors, "\n"))
+				if err := zc.z.createError(sf, name, parseErr); err != nil {
+					return err
+				}
+			}
 			sf.progress("writing output: %s", name)
 			warnRedactLeak := false
 			if err := func() error {

pkg/kv/kvserver/batcheval/cmd_revert_range.go

@@ -140,12 +140,13 @@ func RevertRange(
 
 	leftPeekBound, rightPeekBound := rangeTombstonePeekBounds(
 		args.Key, args.EndKey, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey())
+	maxLockConflicts := storage.MaxConflictsPerLockConflictError.Get(&cArgs.EvalCtx.ClusterSettings().SV)
 
 	log.VEventf(ctx, 2, "clearing keys with timestamp (%v, %v]", args.TargetTime, cArgs.Header.Timestamp)
 
 	resumeKey, err := storage.MVCCClearTimeRange(ctx, readWriter, cArgs.Stats, args.Key, args.EndKey,
 		args.TargetTime, cArgs.Header.Timestamp, leftPeekBound, rightPeekBound,
-		clearRangeThreshold, cArgs.Header.MaxSpanRequestKeys, maxRevertRangeBatchBytes)
+		clearRangeThreshold, cArgs.Header.MaxSpanRequestKeys, maxRevertRangeBatchBytes, maxLockConflicts)
 	if err != nil {
 		return result.Result{}, err
 	}

pkg/kv/kvserver/batcheval/cmd_revert_range_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -115,7 +116,12 @@ func TestCmdRevertRange(t *testing.T) {
 		EndKey:   roachpb.RKey(endKey),
 	}
 	cArgs := batcheval.CommandArgs{Header: kvpb.Header{RangeID: desc.RangeID, Timestamp: tsReq, MaxSpanRequestKeys: 2}}
-	evalCtx := &batcheval.MockEvalCtx{Desc: &desc, Clock: hlc.NewClockForTesting(nil), Stats: stats}
+	evalCtx := &batcheval.MockEvalCtx{
+		ClusterSettings: cluster.MakeTestingClusterSettings(),
+		Desc:            &desc,
+		Clock:           hlc.NewClockForTesting(nil),
+		Stats:           stats,
+	}
 	cArgs.EvalCtx = evalCtx.EvalContext()
 	afterStats, err := storage.ComputeStats(ctx, eng, keys.LocalMax, keys.MaxKey, 0)
 	require.NoError(t, err)
@@ -184,7 +190,6 @@ func TestCmdRevertRange(t *testing.T) {
 	); err != nil {
 		t.Fatal(err)
 	}
-	sumCIntent := hashRange(t, eng, startKey, endKey)
 
 	// Lay down more revisions (skipping even keys to avoid our intent on 0012).
 	for i := 7; i < keyCount+7; i += 2 {
@@ -201,17 +206,22 @@ func TestCmdRevertRange(t *testing.T) {
 	sumD := hashRange(t, eng, startKey, endKey)
 
 	// Re-set EvalCtx to pick up revised stats.
-	cArgs.EvalCtx = (&batcheval.MockEvalCtx{Desc: &desc, Clock: hlc.NewClockForTesting(nil), Stats: stats}).EvalContext( /* maxOffset */ )
+	cArgs.EvalCtx = (&batcheval.MockEvalCtx{
+		ClusterSettings: cluster.MakeTestingClusterSettings(),
+		Desc:            &desc,
+		Clock:           hlc.NewClockForTesting(nil),
+		Stats:           stats,
+	}).EvalContext( /* maxOffset */ )
 	for _, tc := range []struct {
 		name        string
 		ts          hlc.Timestamp
 		expectErr   bool
 		expectedSum []byte
 		resumes     int
 	}{
-		{"hit intent", tsB, true, nil, 2},
-		{"hit intent exactly", tsC, false, sumCIntent, 2},
-		{"clear above intent", tsC.Add(0, 1), false, sumCIntent, 2},
+		{"hit intent", tsB, true, nil, 0},
+		{"hit intent exactly", tsC, true, nil, 0},
+		{"clear above intent", tsC.Add(0, 1), true, nil, 0},
 		{"clear nothing above intent", tsD, false, sumD, 0},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -297,9 +307,10 @@ func TestCmdRevertRangeMVCCRangeTombstones(t *testing.T) {
 		var ms enginepb.MVCCStats
 		cArgs := batcheval.CommandArgs{
 			EvalCtx: (&batcheval.MockEvalCtx{
-				Desc:  &desc,
-				Clock: hlc.NewClockForTesting(nil),
-				Stats: ms,
+				ClusterSettings: cluster.MakeTestingClusterSettings(),
+				Desc:            &desc,
+				Clock:           hlc.NewClockForTesting(nil),
+				Stats:           ms,
 			}).EvalContext(),
 			Header: kvpb.Header{
 				RangeID:   desc.RangeID,