Merge cockroachdb#108121 cockroachdb#108201 cockroachdb#108427

108121: sql: rewind txn sequence number in internal executor r=rafiss a=rafiss

If the internal executor is used by a user-initiated query, then it shares the same transaction as the user's query. In that case, it's important to step back the sequence number so that a single statement does not read its own writes.

fixes cockroachdb#86162
Release note: None

108201: roachtest: suppress grafana link in issue help for non GCE r=smg260 a=smg260

We don't want to show a grafana when the issues originates from non GCE tests/clusters, since we don't scrape from those yet.

Epic: none

Release note: None

108427: roachtest: provision 250 MB/s for 8TB restore test r=msbutler a=pavelkalinnikov

The `restore/tpce/8TB/aws/nodes=10/cpus=8` test maxes out the default 125 MB/s EBS throughput. This commit provisions throughput to be 250 MB/s so that the test doesn't work at the edge of overload.

See cockroachdb#107609 (comment) for the before/after comparison.

Touches cockroachdb#106496
Fixes cockroachdb#107609
Epic: none
Release note: none

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Miral Gadani <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>

Author: 4 people

pkg/cmd/roachtest/github.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/cmd/internal/issues"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/internal/team"
 	rperrors "github.com/cockroachdb/cockroach/pkg/roachprod/errors"
@@ -49,7 +50,7 @@ func roachtestPrefix(p string) string {
 
 // generateHelpCommand creates a HelpCommand for createPostRequest
 func generateHelpCommand(
-	clusterName string, start time.Time, end time.Time,
+	clusterName string, cloud string, start time.Time, end time.Time,
 ) func(renderer *issues.Renderer) {
 	return func(renderer *issues.Renderer) {
 		issues.HelpCommandAsLink(
@@ -60,8 +61,9 @@ func generateHelpCommand(
 			"How To Investigate (internal)",
 			"https://cockroachlabs.atlassian.net/l/c/SSSBr8c7",
 		)(renderer)
-		// An empty clusterName corresponds to a cluster creation failure
-		if clusterName != "" {
+		// An empty clusterName corresponds to a cluster creation failure.
+		// We only scrape metrics from GCE clusters for now.
+		if spec.GCE == cloud && clusterName != "" {
 			issues.HelpCommandAsLink(
 				"Grafana",
 				fmt.Sprintf("https://go.crdb.dev/p/roachfana/%s/%d/%d", clusterName, start.UnixMilli(), end.UnixMilli()),
@@ -223,7 +225,7 @@ func (g *githubIssues) createPostRequest(
 		Artifacts:       artifacts,
 		ExtraLabels:     labels,
 		ExtraParams:     clusterParams,
-		HelpCommand:     generateHelpCommand(issueClusterName, start, end),
+		HelpCommand:     generateHelpCommand(issueClusterName, spec.Cluster.Cloud, start, end),
 	}, nil
 }
 

pkg/cmd/roachtest/github_test.go

@@ -111,9 +111,14 @@ func TestGenerateHelpCommand(t *testing.T) {
 	end := time.Date(2023, time.July, 21, 16, 42, 13, 137, time.UTC)
 
 	r := &issues.Renderer{}
-	generateHelpCommand("foo-cluster", start, end)(r)
+	generateHelpCommand("foo-cluster", spec.GCE, start, end)(r)
 
 	echotest.Require(t, r.String(), filepath.Join("testdata", "help_command.txt"))
+
+	r = &issues.Renderer{}
+	generateHelpCommand("foo-cluster", spec.AWS, start, end)(r)
+
+	echotest.Require(t, r.String(), filepath.Join("testdata", "help_command_non_gce.txt"))
 }
 
 func TestCreatePostRequest(t *testing.T) {

pkg/cmd/roachtest/spec/cluster_spec.go

@@ -93,6 +93,10 @@ type ClusterSpec struct {
 	// generality. Make it easier to just inject cloud-specific arguments.
 	GCEMinCPUPlatform string
 	GCEVolumeType     string
+	// AWS-specific arguments.
+	//
+	// AWSVolumeThroughput is the min provisioned EBS volume throughput.
+	AWSVolumeThroughput int
 }
 
 // MakeClusterSpec makes a ClusterSpec.
@@ -140,11 +144,16 @@ func awsMachineSupportsSSD(machineType string) bool {
 	return false
 }
 
-func getAWSOpts(machineType string, zones []string, volumeSize int, localSSD bool) vm.ProviderOpts {
+func getAWSOpts(
+	machineType string, zones []string, volumeSize, ebsThroughput int, localSSD bool,
+) vm.ProviderOpts {
 	opts := aws.DefaultProviderOpts()
 	if volumeSize != 0 {
 		opts.DefaultEBSVolume.Disk.VolumeSize = volumeSize
 	}
+	if ebsThroughput != 0 {
+		opts.DefaultEBSVolume.Disk.Throughput = ebsThroughput
+	}
 	if localSSD {
 		opts.SSDMachineType = machineType
 	} else {
@@ -310,7 +319,8 @@ func (s *ClusterSpec) RoachprodOpts(
 	var providerOpts vm.ProviderOpts
 	switch s.Cloud {
 	case AWS:
-		providerOpts = getAWSOpts(machineType, zones, s.VolumeSize, createVMOpts.SSDOpts.UseLocalSSD)
+		providerOpts = getAWSOpts(machineType, zones, s.VolumeSize, s.AWSVolumeThroughput,
+			createVMOpts.SSDOpts.UseLocalSSD)
 	case GCE:
 		providerOpts = getGCEOpts(machineType, zones, s.VolumeSize, ssdCount,
 			createVMOpts.SSDOpts.UseLocalSSD, s.RAID0, s.TerminateOnMigration,

pkg/cmd/roachtest/testdata/help_command_non_gce.txt

@@ -0,0 +1,13 @@
+echo
+----
+----
+
+
+See: [roachtest README](https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/README.md)
+
+
+
+See: [How To Investigate \(internal\)](https://cockroachlabs.atlassian.net/l/c/SSSBr8c7)
+
+----
+----

pkg/cmd/roachtest/tests/restore.go

@@ -323,7 +323,10 @@ func registerRestore(r registry.Registry) {
 		},
 		{
 			// The nightly 8TB Restore test.
-			hardware: makeHardwareSpecs(hardwareSpecs{nodes: 10, volumeSize: 2000}),
+			// NB: bump disk throughput because this load saturates the default 125
+			// MB/s. See https://github.com/cockroachdb/cockroach/issues/107609.
+			hardware: makeHardwareSpecs(hardwareSpecs{nodes: 10, volumeSize: 2000,
+				ebsThroughput: 250 /* MB/s */}),
 			backup: makeRestoringBackupSpecs(backupSpecs{
 				version:  "v22.2.1",
 				workload: tpceRestore{customers: 500000}}),
@@ -466,6 +469,9 @@ type hardwareSpecs struct {
 	// volumeSize indicates the size of per node block storage (pd-ssd for gcs,
 	// ebs for aws). If zero, local ssd's are used.
 	volumeSize int
+	// ebsThroughput is the min provisioned throughput of the EBS volume, in MB/s.
+	// TODO(pavelkalinnikov): support provisioning throughput not only on EBS.
+	ebsThroughput int
 
 	// mem is the memory per cpu.
 	mem spec.MemPerCPU
@@ -494,6 +500,10 @@ func (hw hardwareSpecs) makeClusterSpecs(r registry.Registry, backupCloud string
 	}
 	s := r.MakeClusterSpec(hw.nodes+addWorkloadNode, clusterOpts...)
 
+	if hw.ebsThroughput != 0 {
+		s.AWSVolumeThroughput = hw.ebsThroughput
+	}
+
 	if backupCloud == spec.AWS && s.Cloud == spec.AWS && s.VolumeSize != 0 {
 		// Work around an issue that RAID0s local NVMe and GP3 storage together:
 		// https://github.com/cockroachdb/cockroach/issues/98783.
@@ -554,6 +564,9 @@ func makeHardwareSpecs(override hardwareSpecs) hardwareSpecs {
 	if override.volumeSize != 0 {
 		specs.volumeSize = override.volumeSize
 	}
+	if override.ebsThroughput != 0 {
+		specs.ebsThroughput = override.ebsThroughput
+	}
 	specs.zones = override.zones
 	specs.workloadNode = override.workloadNode
 	return specs

pkg/roachprod/vm/gce/gcloud.go

@@ -1009,6 +1009,9 @@ func (p *Provider) Create(
 			fmt.Sprintf("size=%dGB", providerOpts.PDVolumeSize),
 			"auto-delete=yes",
 		}
+		// TODO(pavelkalinnikov): support disk types with "provisioned-throughput"
+		// option, such as Hyperdisk Throughput:
+		// https://cloud.google.com/compute/docs/disks/add-hyperdisk#hyperdisk-throughput.
 		args = append(args, "--create-disk", strings.Join(pdProps, ","))
 		// Enable DISCARD commands for persistent disks, as is advised in:
 		// https://cloud.google.com/compute/docs/disks/optimizing-pd-performance#formatting_parameters.

pkg/sql/conn_executor_exec.go

@@ -961,7 +961,18 @@ func (ex *connExecutor) execStmtInOpenState(
 	// single/common function. That would be where the stepping mode
 	// gets enabled once for all SQL statements executed "underneath".
 	prevSteppingMode := ex.state.mu.txn.ConfigureStepping(ctx, kv.SteppingEnabled)
-	defer func() { _ = ex.state.mu.txn.ConfigureStepping(ctx, prevSteppingMode) }()
+	prevSeqNum := ex.state.mu.txn.GetReadSeqNum()
+	defer func() {
+		_ = ex.state.mu.txn.ConfigureStepping(ctx, prevSteppingMode)
+
+		// If this is an internal executor that is running on behalf of an outer
+		// txn, then we need to step back the txn so that the outer executor uses
+		// the proper sequence number.
+		if ex.executorType == executorTypeInternal && ex.extraTxnState.fromOuterTxn {
+			err := ex.state.mu.txn.SetReadSeqNum(prevSeqNum)
+			retEv, retPayload, retErr = makeErrEvent(err)
+		}
+	}()
 
 	// Then we create a sequencing point.
 	//

pkg/sql/conn_executor_test.go

@@ -450,6 +450,8 @@ SELECT IF(x::INT::FLOAT = x,
              'NOOPE', 'insert saw its own writes: ' || x::STRING || ' (it is halloween today)')::FLOAT)
        + 0.1
   FROM t.test
+  -- the function used here is implemented by using the internal executor.
+  WHERE has_table_privilege('root', ((x+.1)/(x+1) + 1)::int::oid, 'INSERT') IS NULL
 `); err != nil {
 		t.Fatal(err)
 	}