From f73dae7e9586cc21a7cdb09541b7ee4bd855d325 Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Wed, 3 Mar 2021 22:15:25 -0500 Subject: [PATCH 1/3] builtins: implement ST_AddMeasure This patch implements the geometry builtin `ST_AddMeasure`. Release justification: low-risk update to new functionality Release note (sql change): The `ST_AddMeasure` function is now available for use. --- docs/generated/sql/functions.md | 2 + pkg/geo/geomfn/BUILD.bazel | 2 + pkg/geo/geomfn/add_measure.go | 131 ++++++++++ pkg/geo/geomfn/add_measure_test.go | 239 ++++++++++++++++++ .../testdata/logic_test/geospatial_zm | 13 + pkg/sql/sem/builtins/geo_builtins.go | 22 ++ 6 files changed, 409 insertions(+) create mode 100644 pkg/geo/geomfn/add_measure.go create mode 100644 pkg/geo/geomfn/add_measure_test.go diff --git a/docs/generated/sql/functions.md b/docs/generated/sql/functions.md index 5b6be0d9469b..02fb5429da07 100644 --- a/docs/generated/sql/functions.md +++ b/docs/generated/sql/functions.md @@ -1271,6 +1271,8 @@ the locality flag on node startup. Returns an error if no region is set.

postgis_wagyu_version() → string

Compatibility placeholder function with PostGIS. Returns a fixed string based on PostGIS 3.0.1, with minor edits.

 +st_addmeasure(geometry: geometry, start: float, end: float) → geometry

Returns a copy of a LineString or MultiLineString with measure coordinates linearly interpolated between the specified start and end values. Any existing M coordinates will be overwritten.

+ st_addpoint(line_string: geometry, point: geometry) → geometry

Adds a Point to the end of a LineString.

 st_addpoint(line_string: geometry, point: geometry, index: int) → geometry

Adds a Point to a LineString at the given 0-based index (-1 to append).

diff --git a/pkg/geo/geomfn/BUILD.bazel b/pkg/geo/geomfn/BUILD.bazel index 4adb29574cfa..77da72e670c4 100644 --- a/pkg/geo/geomfn/BUILD.bazel +++ b/pkg/geo/geomfn/BUILD.bazel @@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "geomfn", srcs = [ + "add_measure.go", "affine_transforms.go", "angle.go", "azimuth.go", @@ -58,6 +59,7 @@ go_test( name = "geomfn_test", size = "small", srcs = [ + "add_measure_test.go", "affine_transforms_test.go", "angle_test.go", "azimuth_test.go", diff --git a/pkg/geo/geomfn/add_measure.go b/pkg/geo/geomfn/add_measure.go new file mode 100644 index 000000000000..66035e7ef687 --- /dev/null +++ b/pkg/geo/geomfn/add_measure.go @@ -0,0 +1,131 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package geomfn + +import ( + "github.com/cockroachdb/cockroach/pkg/geo" + "github.com/cockroachdb/errors" + "github.com/twpayne/go-geom" +) + +// AddMeasure takes a LineString or MultiLineString and linearly interpolates measure values for each line. +func AddMeasure(geometry geo.Geometry, start float64, end float64) (geo.Geometry, error) { + t, err := geometry.AsGeomT() + if err != nil { + return geometry, err + } + + switch t := t.(type) { + case *geom.LineString: + newLineString, err := addMeasureToLineString(t, start, end) + if err != nil { + return geometry, err + } + return geo.MakeGeometryFromGeomT(newLineString) + case *geom.MultiLineString: + newMultiLineString, err := addMeasureToMultiLineString(t, start, end) + if err != nil { + return geometry, err + } + return geo.MakeGeometryFromGeomT(newMultiLineString) + default: + // Ideally we should return NULL here, but following PostGIS on this. + return geometry, errors.Newf("input geometry must be LINESTRING or MULTILINESTRING") + } +} + +// addMeasureToMultiLineString takes a MultiLineString and linearly interpolates measure values for each component line. +func addMeasureToMultiLineString( + multiLineString *geom.MultiLineString, start float64, end float64, +) (*geom.MultiLineString, error) { + newMultiLineString := + geom.NewMultiLineString(augmentLayoutWithM(multiLineString.Layout())).SetSRID(multiLineString.SRID()) + + // Create a copy of the MultiLineString with measures added to each component LineString. + for i := 0; i < multiLineString.NumLineStrings(); i++ { + newLineString, err := addMeasureToLineString(multiLineString.LineString(i), start, end) + if err != nil { + return multiLineString, err + } + err = newMultiLineString.Push(newLineString) + if err != nil { + return multiLineString, err + } + } + + return newMultiLineString, nil +} + +// addMeasureToLineString takes a LineString and linearly interpolates measure values. +func addMeasureToLineString( + lineString *geom.LineString, start float64, end float64, +) (*geom.LineString, error) { + newLineString := geom.NewLineString(augmentLayoutWithM(lineString.Layout())).SetSRID(lineString.SRID()) + + if lineString.Empty() { + return newLineString, nil + } + + // Extract the line's current points. + lineCoords := lineString.Coords() + + // Compute the length of the line as the sum of the distances between each pair of points. + // Also, fill in pointMeasures with the partial sums. + prevPoint := lineCoords[0] + lineLength := float64(0) + pointMeasures := make([]float64, lineString.NumCoords()) + for i := 0; i < lineString.NumCoords(); i++ { + curPoint := lineCoords[i] + distBetweenPoints := coordNorm(coordSub(prevPoint, curPoint)) + lineLength += distBetweenPoints + pointMeasures[i] = lineLength + prevPoint = curPoint + } + + // Compute the measures for each point. + for i := 0; i < lineString.NumCoords(); i++ { + // Handle special case where line is zero length. + if lineLength == 0 { + pointMeasures[i] = start + (end-start)*(float64(i)/float64(lineString.NumCoords()-1)) + } else { + pointMeasures[i] = start + (end-start)*(pointMeasures[i]/lineLength) + } + } + + // Replace M value if it exists, otherwise append it to each Coord. + for i := 0; i < lineString.NumCoords(); i++ { + if lineString.Layout().MIndex() == -1 { + lineCoords[i] = append(lineCoords[i], pointMeasures[i]) + } else { + lineCoords[i][lineString.Layout().MIndex()] = pointMeasures[i] + } + } + + // Create a new LineString with the measures tacked on. + _, err := newLineString.SetCoords(lineCoords) + if err != nil { + return lineString, err + } + + return newLineString, nil +} + +// augmentLayoutWithM takes a layout and returns a layout with the M dimension added. +func augmentLayoutWithM(layout geom.Layout) geom.Layout { + switch layout { + case geom.XY, geom.XYM: + return geom.XYM + case geom.XYZ, geom.XYZM: + return geom.XYZM + default: + return layout + } +} diff --git a/pkg/geo/geomfn/add_measure_test.go b/pkg/geo/geomfn/add_measure_test.go new file mode 100644 index 000000000000..ee521a9bf59a --- /dev/null +++ b/pkg/geo/geomfn/add_measure_test.go @@ -0,0 +1,239 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package geomfn + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/geo" + "github.com/stretchr/testify/require" + "github.com/twpayne/go-geom" +) + +func TestAddMeasure(t *testing.T) { + testCases := []struct { + desc string + input geom.T + start float64 + end float64 + expected geom.T + }{ + { + desc: "add measure to 2D linestring", + input: geom.NewLineStringFlat(geom.XY, []float64{0, 0, 1, 1, 2, 2}).SetSRID(4326), + start: 0, + end: 1, + expected: geom.NewLineStringFlat(geom.XYM, []float64{0, 0, 0, 1, 1, 0.5, 2, 2, 1}).SetSRID(4326), + }, + { + desc: "add measure to 2D linestring with start larger than end", + input: geom.NewLineStringFlat(geom.XY, []float64{0, 0, 1, 1, 2, 2}).SetSRID(26918), + start: 3, + end: 1, + expected: geom.NewLineStringFlat(geom.XYM, []float64{0, 0, 3, 1, 1, 2, 2, 2, 1}).SetSRID(26918), + }, + { + desc: "add measure to zero length 2D linestring", + input: geom.NewLineStringFlat(geom.XY, []float64{0, 0, 0, 0, 0, 0}), + start: 0, + end: 1, + expected: geom.NewLineStringFlat(geom.XYM, []float64{0, 0, 0, 0, 0, 0.5, 0, 0, 1}), + }, + { + desc: "add measure to 2D linestring with zero length segment", + input: geom.NewLineStringFlat(geom.XY, []float64{0, 0, 1, 1, 1, 1, 1, 1, 2, 2}), + start: 0, + end: 10, + expected: geom.NewLineStringFlat(geom.XYM, []float64{0, 0, 0, 1, 1, 5, 1, 1, 5, 1, 1, 5, 2, 2, 10}), + }, + { + desc: "add measure to 2D+M linestring", + input: geom.NewLineStringFlat(geom.XYM, []float64{0, 0, -25, 1, 1, -50, 2, 2, 0}), + start: 1, + end: 2, + expected: geom.NewLineStringFlat(geom.XYM, []float64{0, 0, 1, 1, 1, 1.5, 2, 2, 2}), + }, + { + desc: "add measure to 2D+M linestring with same start and end", + input: geom.NewLineStringFlat(geom.XYM, []float64{0, 0, -25, 1, 1, -50, 2, 2, 0}), + start: 100, + end: 100, + expected: geom.NewLineStringFlat(geom.XYM, []float64{0, 0, 100, 1, 1, 100, 2, 2, 100}), + }, + { + desc: "add measure to 3D linestring", + input: geom.NewLineStringFlat(geom.XYZ, []float64{0, 0, -25, 1, 1, -50, 2, 2, 0}), + start: 5, + end: 7, + expected: geom.NewLineStringFlat(geom.XYZM, []float64{0, 0, -25, 5, 1, 1, -50, 6, 2, 2, 0, 7}), + }, + { + desc: "add measure to 4D linestring", + input: geom.NewLineStringFlat(geom.XYZM, []float64{0, 0, -25, -25, 1, 1, -50, -50, 2, 2, 0, 0}), + start: 5, + end: 7, + expected: geom.NewLineStringFlat(geom.XYZM, []float64{0, 0, -25, 5, 1, 1, -50, 6, 2, 2, 0, 7}), + }, + { + desc: "add measure to empty 2D linestring", + input: geom.NewLineString(geom.XY).SetSRID(4326), + start: 0, + end: 1, + expected: geom.NewLineString(geom.XYM).SetSRID(4326), + }, + { + desc: "add measure to empty 2D+M linestring", + input: geom.NewLineString(geom.XYM), + start: 0, + end: 1, + expected: geom.NewLineString(geom.XYM), + }, + { + desc: "add measure to empty 3D linestring", + input: geom.NewLineString(geom.XYZ), + start: 0, + end: 1, + expected: geom.NewLineString(geom.XYZM), + }, + { + desc: "add measure to empty 4D linestring", + input: geom.NewLineString(geom.XYZM), + start: 0, + end: 1, + expected: geom.NewLineString(geom.XYZM), + }, + { + desc: "add measure to 2D multilinestring", + input: geom.NewMultiLineStringFlat(geom.XY, []float64{0, 0, 1, 1}, []int{4}), + start: 0, + end: 1, + expected: geom.NewMultiLineStringFlat(geom.XYM, []float64{0, 0, 0, 1, 1, 1}, []int{6}), + }, + { + desc: "add measure to 2D multilinestring with empty", + input: geom.NewMultiLineStringFlat(geom.XY, []float64{0, 0, 1, 1}, []int{4, 4}), + start: 0, + end: 1, + expected: geom.NewMultiLineStringFlat(geom.XYM, []float64{0, 0, 0, 1, 1, 1}, []int{6, 6}), + }, + { + desc: "add measure to 2D multilinestring with multiple linestrings", + input: geom.NewMultiLineStringFlat(geom.XY, []float64{0, 0, 1, 1, 2, 0, 1, 0, 0, 0}, []int{4, 10}), + start: 6, + end: 0, + expected: geom.NewMultiLineStringFlat( + geom.XYM, []float64{0, 0, 6, 1, 1, 0, 2, 0, 6, 1, 0, 3, 0, 0, 0}, []int{6, 15}), + }, + { + desc: "add measure to 2D+M multilinestring", + input: geom.NewMultiLineStringFlat(geom.XYM, []float64{0, 0, -5, 1, 1, 5}, []int{6}), + start: 0, + end: 1, + expected: geom.NewMultiLineStringFlat(geom.XYM, []float64{0, 0, 0, 1, 1, 1}, []int{6}), + }, + { + desc: "add measure to 3D multilinestring", + input: geom.NewMultiLineStringFlat(geom.XYZ, []float64{0, 0, -5, 1, 1, 5}, []int{6}), + start: 0, + end: 1, + expected: geom.NewMultiLineStringFlat(geom.XYZM, []float64{0, 0, -5, 0, 1, 1, 5, 1}, []int{8}), + }, + { + desc: "add measure to 4D multilinestring", + input: geom.NewMultiLineStringFlat(geom.XYZM, []float64{0, 0, -5, 23, 1, 1, 5, -23}, []int{8}), + start: 0, + end: 1, + expected: geom.NewMultiLineStringFlat(geom.XYZM, []float64{0, 0, -5, 0, 1, 1, 5, 1}, []int{8}), + }, + { + desc: "add measure to empty 2D multilinestring", + input: geom.NewMultiLineString(geom.XY), + start: 0, + end: 1, + expected: geom.NewMultiLineString(geom.XYM), + }, + { + desc: "add measure to empty 2D+M multilinestring", + input: geom.NewMultiLineString(geom.XYM), + start: 0, + end: 1, + expected: geom.NewMultiLineString(geom.XYM), + }, + { + desc: "add measure to empty 3D multilinestring", + input: geom.NewMultiLineString(geom.XYZ), + start: 0, + end: 1, + expected: geom.NewMultiLineString(geom.XYZM), + }, + { + desc: "add measure to empty 4D multilinestring", + input: geom.NewMultiLineString(geom.XYZM), + start: 0, + end: 1, + expected: geom.NewMultiLineString(geom.XYZM), + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + geometry, err := geo.MakeGeometryFromGeomT(tc.input) + require.NoError(t, err) + + got, err := AddMeasure(geometry, tc.start, tc.end) + require.NoError(t, err) + + want, err := geo.MakeGeometryFromGeomT(tc.expected) + require.NoError(t, err) + + require.Equal(t, want, got) + }) + } +} + +func TestAddMeasureError(t *testing.T) { + errorTestCases := []struct { + geomType string + input geom.T + }{ + { + geomType: "point", + input: geom.NewPointFlat(geom.XY, []float64{0, 0}), + }, + { + geomType: "polygon", + input: geom.NewPolygonFlat(geom.XY, []float64{0, 0, 1, 2, 2, 0, 0, 0}, []int{8}), + }, + { + geomType: "multipoint", + input: geom.NewMultiPointFlat(geom.XY, []float64{0, 0, 1, 1}), + }, + { + geomType: "multipolygon", + input: geom.NewMultiPolygonFlat(geom.XY, []float64{0, 0, 1, 2, 2, 0, 0, 0}, [][]int{{8}}), + }, + { + geomType: "geometrycollection", + input: geom.NewGeometryCollection().MustPush(geom.NewLineStringFlat(geom.XY, []float64{0, 0, 1, 1})), + }, + } + + for _, tc := range errorTestCases { + testName := "invalid attempt to add measure to a " + tc.geomType + t.Run(testName, func(t *testing.T) { + geometry, err := geo.MakeGeometryFromGeomT(tc.input) + require.NoError(t, err) + + _, err = AddMeasure(geometry, 0, 1) + require.EqualError(t, err, "input geometry must be LINESTRING or MULTILINESTRING") + }) + } +} diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial_zm b/pkg/sql/logictest/testdata/logic_test/geospatial_zm index c0c1f5b2d291..3be7d022baf3 100644 --- a/pkg/sql/logictest/testdata/logic_test/geospatial_zm +++ b/pkg/sql/logictest/testdata/logic_test/geospatial_zm @@ -286,6 +286,19 @@ POINT ZM (1 2 3 17) POINT ZM (1 2 3 4) GEOMETRYCOLLECTION ZM (POINT ZM EMPTY, LINESTRING ZM (1 2 7 17, 3 4 7 17)) +query T +SELECT st_astext(st_addmeasure(geom, 0, 10)) FROM +( VALUES + ('LINESTRING(0 0, 1 1, 2 2)'::geometry), + ('MULTILINESTRING((0 0, 1 1, 2 2), EMPTY)'::geometry) +) AS t(geom) +---- +LINESTRING M (0 0 0, 1 1 5, 2 2 10) +MULTILINESTRING M ((0 0 0, 1 1 5, 2 2 10), EMPTY) + +statement error input geometry must be LINESTRING or MULTILINESTRING +SELECT st_astext(st_addmeasure('POINT(0 0)'::geometry, 0, 1)) + query T SELECT ST_AsEWKT(ST_RotateX(ST_GeomFromEWKT('LINESTRING(1 2 3, 1 1 1)'), pi()/2)); ---- diff --git a/pkg/sql/sem/builtins/geo_builtins.go b/pkg/sql/sem/builtins/geo_builtins.go index dd0d6ed2dc64..109647e62255 100644 --- a/pkg/sql/sem/builtins/geo_builtins.go +++ b/pkg/sql/sem/builtins/geo_builtins.go @@ -2885,6 +2885,28 @@ The requested number of points must be not larger than 65336.`, tree.VolatilityImmutable, ), ), + "st_addmeasure": makeBuiltin( + defProps(), + tree.Overload{ + Types: tree.ArgTypes{{"geometry", types.Geometry}, {"start", types.Float}, {"end", types.Float}}, + ReturnType: tree.FixedReturnType(types.Geometry), + Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { + g := tree.MustBeDGeometry(args[0]) + start := tree.MustBeDFloat(args[1]) + end := tree.MustBeDFloat(args[2]) + + ret, err := geomfn.AddMeasure(g.Geometry, float64(start), float64(end)) + if err != nil { + return nil, err + } + return tree.NewDGeometry(ret), nil + }, + Info: infoBuilder{info: "Returns a copy of a LineString or MultiLineString with measure coordinates " + + "linearly interpolated between the specified start and end values. " + + "Any existing M coordinates will be overwritten."}.String(), + Volatility: tree.VolatilityImmutable, + }, + ), "st_lineinterpolatepoint": makeBuiltin( defProps(), lineInterpolatePointForRepeatOverload( From d4f9b0219779cb6afddb4a59fcee5cc88398af24 Mon Sep 17 00:00:00 2001 From: angelapwen Date: Tue, 2 Mar 2021 18:13:00 +0100 Subject: [PATCH 2/3] util/tracing,sql: add builtin to set trace spans' verbosity Previously there was no way to change a span's verbosity via the SQL shell. We want to be able to set a specific long-running span's verbosity on to retrieve its recordings. This patch adds a builtin, `crdb_internal.set_trace_verbose` that takes in a trace ID and a bool representing verbose or not verbose. It sets the verbosity of all spans in this trace. Note that we would prefer to toggle individual span verbosity, but this would require a registry of Span objects that is not added to the 21.1 release. If this Span registry were added in the future, we could access a Span given its span ID. Release justification: Adds a crdb_internal tool meant for on-call engineers, TSEs, etc to debug. Release note (sql change): Adds a new builtin that sets the verbosity of all spans in a given trace. Syntax: crdb_internal.set_trace_verbose($traceID,$verbosityAsBool). --- docs/generated/sql/functions.md | 2 + .../testdata/logic_test/builtin_function | 13 + pkg/sql/sem/builtins/builtins.go | 60 ++++ pkg/sql/tests/BUILD.bazel | 2 + pkg/sql/tests/tracing_sql_test.go | 91 +++++++ pkg/util/tracing/BUILD.bazel | 1 + pkg/util/tracing/crdbspan.go | 23 +- pkg/util/tracing/span.go | 237 +--------------- pkg/util/tracing/span_inner.go | 256 ++++++++++++++++++ 9 files changed, 456 insertions(+), 229 deletions(-) create mode 100644 pkg/sql/tests/tracing_sql_test.go create mode 100644 pkg/util/tracing/span_inner.go diff --git a/docs/generated/sql/functions.md b/docs/generated/sql/functions.md index 2b711fa691a6..3daaf2a23c3f 100644 --- a/docs/generated/sql/functions.md +++ b/docs/generated/sql/functions.md @@ -2706,6 +2706,8 @@ SELECT * FROM crdb_internal.check_consistency(true, ‘\x02’, ‘\x04’)

 crdb_internal.round_decimal_values(val: decimal[], scale: int) → decimal[]

This function is used internally to round decimal array values during mutations.

 +crdb_internal.set_trace_verbose(trace_id: int, verbosity: bool) → bool

Returns true if root span was found and verbosity was set, false otherwise.

+ crdb_internal.set_vmodule(vmodule_string: string) → int

Set the equivalent of the --vmodule flag on the gateway node processing this request; it affords control over the logging verbosity of different files. Example syntax: crdb_internal.set_vmodule('recordio=2,file=1,gfs*=3'). Reset with: crdb_internal.set_vmodule(''). Raising the verbosity can severely affect performance.

 crdb_internal.trace_id() → int

Returns the current trace ID or an error if no trace is open.

diff --git a/pkg/sql/logictest/testdata/logic_test/builtin_function b/pkg/sql/logictest/testdata/logic_test/builtin_function index 05806ebe9a5a..a53981e6b888 100644 --- a/pkg/sql/logictest/testdata/logic_test/builtin_function +++ b/pkg/sql/logictest/testdata/logic_test/builtin_function @@ -2960,3 +2960,16 @@ SELECT * FROM crdb_internal.payloads_for_trace(0) WHERE false ---- span_id payload_type payload_jsonb + +# switch users -- this one has no permissions so expect errors +user testuser + +query error insufficient privilege +SELECT * FROM crdb_internal.set_trace_verbose(0, false) + +user root + +query B +SELECT * FROM crdb_internal.set_trace_verbose(0, false) +WHERE false +---- diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index d42adf033199..e9007aaafa8e 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -58,6 +58,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/streaming" "github.com/cockroachdb/cockroach/pkg/util/duration" @@ -3637,6 +3638,65 @@ may increase either contention or retry errors, or both.`, }, ), + // Toggles all spans of the requested trace to verbose or non-verbose. + "crdb_internal.set_trace_verbose": makeBuiltin( + tree.FunctionProperties{Category: categorySystemInfo}, + tree.Overload{ + Types: tree.ArgTypes{ + {"trace_id", types.Int}, + {"verbosity", types.Bool}, + }, + ReturnType: tree.FixedReturnType(types.Bool), + Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { + // The user must be an admin to use this builtin. + isAdmin, err := ctx.SessionAccessor.HasAdminRole(ctx.Context) + if err != nil { + return nil, err + } + if !isAdmin { + if err := checkPrivilegedUser(ctx); err != nil { + return nil, err + } + } + + traceID := uint64(*(args[0].(*tree.DInt))) + verbosity := bool(*(args[1].(*tree.DBool))) + + const query = `SELECT span_id + FROM crdb_internal.node_inflight_trace_spans + WHERE trace_id = $1 + AND parent_span_id = 0` + + ie := ctx.InternalExecutor.(sqlutil.InternalExecutor) + row, err := ie.QueryRowEx( + ctx.Ctx(), + "crdb_internal.set_trace_verbose", + ctx.Txn, + sessiondata.NoSessionDataOverride, + query, + traceID, + ) + if err != nil { + return nil, err + } + if row == nil { + return tree.DBoolFalse, nil + } + rootSpanID := uint64(*row[0].(*tree.DInt)) + + rootSpan, found := ctx.Settings.Tracer.GetActiveSpanFromID(rootSpanID) + if !found { + return tree.DBoolFalse, nil + } + + rootSpan.SetVerboseRecursively(verbosity) + return tree.DBoolTrue, nil + }, + Info: "Returns true if root span was found and verbosity was set, false otherwise.", + Volatility: tree.VolatilityVolatile, + }, + ), + "crdb_internal.locality_value": makeBuiltin( tree.FunctionProperties{Category: categorySystemInfo}, tree.Overload{ diff --git a/pkg/sql/tests/BUILD.bazel b/pkg/sql/tests/BUILD.bazel index 4066569020e6..3440318a9d98 100644 --- a/pkg/sql/tests/BUILD.bazel +++ b/pkg/sql/tests/BUILD.bazel @@ -41,6 +41,7 @@ go_test( "split_test.go", "system_table_test.go", "table_split_test.go", + "tracing_sql_test.go", "virtual_table_test.go", ], data = glob(["testdata/**"]), @@ -89,6 +90,7 @@ go_test( "//pkg/util/randutil", "//pkg/util/syncutil", "//pkg/util/timeutil", + "//pkg/util/tracing", "//pkg/util/uuid", "@com_github_cockroachdb_cockroach_go//crdb", "@com_github_cockroachdb_datadriven//:datadriven", diff --git a/pkg/sql/tests/tracing_sql_test.go b/pkg/sql/tests/tracing_sql_test.go new file mode 100644 index 000000000000..41f64e360e9f --- /dev/null +++ b/pkg/sql/tests/tracing_sql_test.go @@ -0,0 +1,91 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package tests + +import ( + "context" + "fmt" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/tracing" + "github.com/stretchr/testify/require" +) + +func TestSetTraceSpansVerbosityBuiltin(t *testing.T) { + defer leaktest.AfterTest(t)() + s, db, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(context.Background()) + r := sqlutils.MakeSQLRunner(db) + + tr := s.Tracer().(*tracing.Tracer) + + // Try to toggle the verbosity of a trace that doesn't exist, returns false. + // NB: Technically this could return true in the unlikely scenario that there + // is a trace with ID of 0. + r.CheckQueryResults( + t, + "SELECT * FROM crdb_internal.set_trace_verbose(0, true)", + [][]string{{`false`}}, + ) + + root := tr.StartSpan("root", tracing.WithForceRealSpan()) + defer root.Finish() + require.False(t, root.IsVerbose()) + + child := tr.StartSpan("root.child", tracing.WithParentAndAutoCollection(root)) + defer child.Finish() + require.False(t, child.IsVerbose()) + + childChild := tr.StartSpan("root.child.child", tracing.WithParentAndAutoCollection(child)) + defer childChild.Finish() + require.False(t, childChild.IsVerbose()) + + // Toggle the trace's verbosity and confirm all spans are verbose. + traceID := root.TraceID() + query := fmt.Sprintf( + "SELECT * FROM crdb_internal.set_trace_verbose(%d, true)", + traceID, + ) + r.CheckQueryResults( + t, + query, + [][]string{{`true`}}, + ) + + require.True(t, root.IsVerbose()) + require.True(t, child.IsVerbose()) + require.True(t, childChild.IsVerbose()) + + // New child of verbose child span should also be verbose by default. + childNewChild := tr.StartSpan("root.child.newchild", tracing.WithParentAndAutoCollection(child)) + defer childNewChild.Finish() + require.True(t, childNewChild.IsVerbose()) + + // Toggle the trace's verbosity and confirm none of the spans are verbose. + query = fmt.Sprintf( + "SELECT * FROM crdb_internal.set_trace_verbose(%d, false)", + traceID, + ) + r.CheckQueryResults( + t, + query, + [][]string{{`true`}}, + ) + + require.False(t, root.IsVerbose()) + require.False(t, child.IsVerbose()) + require.False(t, childChild.IsVerbose()) + require.False(t, childNewChild.IsVerbose()) +} diff --git a/pkg/util/tracing/BUILD.bazel b/pkg/util/tracing/BUILD.bazel index 7be975183dd2..172d9d7ce5d8 100644 --- a/pkg/util/tracing/BUILD.bazel +++ b/pkg/util/tracing/BUILD.bazel @@ -11,6 +11,7 @@ go_library( "recording.go", "shadow.go", "span.go", + "span_inner.go", "span_options.go", "tags.go", "test_utils.go", diff --git a/pkg/util/tracing/crdbspan.go b/pkg/util/tracing/crdbspan.go index 4c3b4b6c5f9d..be8da15cf7ed 100644 --- a/pkg/util/tracing/crdbspan.go +++ b/pkg/util/tracing/crdbspan.go @@ -101,7 +101,7 @@ func (s *crdbSpan) enableRecording(parent *crdbSpan, recType RecordingType) { if parent != nil { parent.addChild(s) } - if recType == RecordingOff { + if recType == RecordingOff || s.recordingType() == recType { return } @@ -128,6 +128,9 @@ func (s *crdbSpan) resetRecording() { } func (s *crdbSpan) disableRecording() { + if s.recordingType() == RecordingOff { + return + } s.mu.Lock() defer s.mu.Unlock() oldRecType := s.mu.recording.recordingType.swap(RecordingOff) @@ -354,6 +357,24 @@ func (s *crdbSpan) addChild(child *crdbSpan) { s.mu.Unlock() } +// setVerboseRecursively sets the verbosity of the crdbSpan appropriately and +// recurses on its list of children. +func (s *crdbSpan) setVerboseRecursively(to bool) { + if to { + s.enableRecording(nil /* parent */, RecordingVerbose) + } else { + s.disableRecording() + } + + s.mu.Lock() + children := s.mu.recording.children + s.mu.Unlock() + + for _, child := range children { + child.setVerboseRecursively(to) + } +} + var sortPool = sync.Pool{ New: func() interface{} { return &Recording{} diff --git a/pkg/util/tracing/span.go b/pkg/util/tracing/span.go index d1ab1e66e749..75199bba0e93 100644 --- a/pkg/util/tracing/span.go +++ b/pkg/util/tracing/span.go @@ -12,16 +12,11 @@ package tracing import ( "fmt" - "strings" "sync/atomic" - "time" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb" - "github.com/cockroachdb/errors" opentracing "github.com/opentracing/opentracing-go" - otlog "github.com/opentracing/opentracing-go/log" - "golang.org/x/net/trace" ) const ( @@ -138,6 +133,12 @@ func (sp *Span) SetVerbose(to bool) { sp.i.SetVerbose(to) } +// SetVerboseRecursively is like SetVerbose, except it does so for all +// descendant spans as well. +func (sp *Span) SetVerboseRecursively(to bool) { + sp.i.SetVerboseRecursively(to) +} + // ResetRecording clears any previously recorded information. This doesn't // affect any auxiliary trace sinks such as net/trace or zipkin. func (sp *Span) ResetRecording() { @@ -201,20 +202,9 @@ func (sp *Span) SetBaggageItem(restrictedKey, value string) { sp.i.SetBaggageItem(restrictedKey, value) } -// TODO(tbg): move spanInner and its methods into a separate file. - -type spanInner struct { - tracer *Tracer // never nil - - // Internal trace Span; nil if not tracing to crdb. - // When not-nil, allocated together with the surrounding Span for - // performance. - crdb *crdbSpan - // x/net/trace.Trace instance; nil if not tracing to x/net/trace. - netTr trace.Trace - // External opentracing compatible tracer such as lightstep, zipkin, jaeger; - // zero if not using one. - ot otSpan +// TraceID retrieves a span's trace ID. +func (sp *Span) TraceID() uint64 { + return sp.i.TraceID() } // SpanMeta is information about a Span that is not local to this @@ -255,218 +245,9 @@ func (sm *SpanMeta) String() string { return fmt.Sprintf("[spanID: %d, traceID: %d]", sm.spanID, sm.traceID) } -func (s *spanInner) isNoop() bool { - return s.crdb == nil && s.netTr == nil && s.ot == (otSpan{}) -} - -func (s *spanInner) IsVerbose() bool { - return s.crdb.recordingType() == RecordingVerbose -} - -func (s *spanInner) SetVerbose(to bool) { - // TODO(tbg): when always-on tracing is firmly established, we can remove the ugly - // caveat that SetVerbose(true) is a panic on a noop span because there will be no - // noop span. - if s.isNoop() { - panic(errors.AssertionFailedf("SetVerbose called on NoopSpan; use the WithForceRealSpan option for StartSpan")) - } - if to { - s.crdb.enableRecording(nil /* parent */, RecordingVerbose) - } else { - s.crdb.disableRecording() - } -} - -func (s *spanInner) ResetRecording() { - s.crdb.resetRecording() -} - -func (s *spanInner) GetRecording() Recording { - // If the span is not verbose, optimize by avoiding the tags. - // This span is likely only used to carry payloads around. - wantTags := s.crdb.recordingType() == RecordingVerbose - return s.crdb.getRecording(s.tracer.TracingVerbosityIndependentSemanticsIsActive(), wantTags) -} - -func (s *spanInner) ImportRemoteSpans(remoteSpans []tracingpb.RecordedSpan) error { - return s.crdb.importRemoteSpans(remoteSpans) -} - -func (s *spanInner) Finish() { - if s == nil { - return - } - if s.isNoop() { - return - } - finishTime := time.Now() - - s.crdb.mu.Lock() - if alreadyFinished := s.crdb.mu.duration >= 0; alreadyFinished { - s.crdb.mu.Unlock() - - // External spans and net/trace are not always forgiving about spans getting - // finished twice, but it may happen so let's be resilient to it. - return - } - s.crdb.mu.duration = finishTime.Sub(s.crdb.startTime) - if s.crdb.mu.duration == 0 { - s.crdb.mu.duration = time.Nanosecond - } - s.crdb.mu.Unlock() - - if s.ot.shadowSpan != nil { - s.ot.shadowSpan.Finish() - } - if s.netTr != nil { - s.netTr.Finish() - } - s.tracer.activeSpans.Lock() - delete(s.tracer.activeSpans.m, s.crdb.spanID) - s.tracer.activeSpans.Unlock() -} - -func (s *spanInner) Meta() *SpanMeta { - var traceID uint64 - var spanID uint64 - var recordingType RecordingType - var baggage map[string]string - - if s.crdb != nil { - traceID, spanID = s.crdb.traceID, s.crdb.spanID - s.crdb.mu.Lock() - defer s.crdb.mu.Unlock() - n := len(s.crdb.mu.baggage) - // In the common case, we have no baggage, so avoid making an empty map. - if n > 0 { - baggage = make(map[string]string, n) - } - for k, v := range s.crdb.mu.baggage { - baggage[k] = v - } - recordingType = s.crdb.mu.recording.recordingType.load() - } - - var shadowTrTyp string - var shadowCtx opentracing.SpanContext - if s.ot.shadowSpan != nil { - shadowTrTyp, _ = s.ot.shadowTr.Type() - shadowCtx = s.ot.shadowSpan.Context() - } - - if traceID == 0 && - spanID == 0 && - shadowTrTyp == "" && - shadowCtx == nil && - recordingType == 0 && - baggage == nil { - return nil - } - return &SpanMeta{ - traceID: traceID, - spanID: spanID, - shadowTracerType: shadowTrTyp, - shadowCtx: shadowCtx, - recordingType: recordingType, - Baggage: baggage, - } -} - -func (s *spanInner) SetOperationName(operationName string) *spanInner { - if s.isNoop() { - return s - } - if s.ot.shadowSpan != nil { - s.ot.shadowSpan.SetOperationName(operationName) - } - s.crdb.operation = operationName - return s -} - -func (s *spanInner) SetTag(key string, value interface{}) *spanInner { - if s.isNoop() { - return s - } - return s.setTagInner(key, value, false /* locked */) -} - -func (s *spanInner) setTagInner(key string, value interface{}, locked bool) *spanInner { - if s.ot.shadowSpan != nil { - s.ot.shadowSpan.SetTag(key, value) - } - if s.netTr != nil { - s.netTr.LazyPrintf("%s:%v", key, value) - } - // The internal tags will be used if we start a recording on this Span. - if !locked { - s.crdb.mu.Lock() - defer s.crdb.mu.Unlock() - } - s.crdb.setTagLocked(key, value) - return s -} - // Structured is an opaque protobuf that can be attached to a trace via // `Span.RecordStructured`. This is the only kind of data a Span carries when // `trace.mode = background`. type Structured interface { protoutil.Message } - -func (s *spanInner) RecordStructured(item Structured) { - if s.isNoop() { - return - } - s.crdb.recordStructured(item) - if s.hasVerboseSink() { - // NB: TrimSpace avoids the trailing whitespace generated by the - // protobuf stringers. - s.Record(strings.TrimSpace(item.String())) - } -} - -func (s *spanInner) Record(msg string) { - s.Recordf("%s", msg) -} - -func (s *spanInner) Recordf(format string, args ...interface{}) { - if !s.hasVerboseSink() { - return - } - str := fmt.Sprintf(format, args...) - if s.ot.shadowSpan != nil { - s.ot.shadowSpan.LogFields(otlog.String(tracingpb.LogMessageField, str)) - } - if s.netTr != nil { - s.netTr.LazyPrintf(format, args) - } - s.crdb.record(str) -} - -// hasVerboseSink returns false if there is no reason to even evaluate Record -// because the result wouldn't be used for anything. -func (s *spanInner) hasVerboseSink() bool { - if s.netTr == nil && s.ot == (otSpan{}) && !s.IsVerbose() { - return false - } - return true -} - -func (s *spanInner) SetBaggageItem(restrictedKey, value string) *spanInner { - if s.isNoop() { - return s - } - s.crdb.setBaggageItemAndTag(restrictedKey, value) - if s.ot.shadowSpan != nil { - s.ot.shadowSpan.SetBaggageItem(restrictedKey, value) - s.ot.shadowSpan.SetTag(restrictedKey, value) - } - // NB: nothing to do for net/trace. - - return s -} - -// Tracer exports the tracer this span was created using. -func (s *spanInner) Tracer() *Tracer { - return s.tracer -} diff --git a/pkg/util/tracing/span_inner.go b/pkg/util/tracing/span_inner.go new file mode 100644 index 000000000000..79ad52e6be25 --- /dev/null +++ b/pkg/util/tracing/span_inner.go @@ -0,0 +1,256 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package tracing + +import ( + "fmt" + "strings" + "time" + + "github.com/cockroachdb/cockroach/pkg/util/timeutil" + "github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb" + "github.com/cockroachdb/errors" + "github.com/opentracing/opentracing-go" + otlog "github.com/opentracing/opentracing-go/log" + "golang.org/x/net/trace" +) + +type spanInner struct { + tracer *Tracer // never nil + + // Internal trace Span; nil if not tracing to crdb. + // When not-nil, allocated together with the surrounding Span for + // performance. + crdb *crdbSpan + // x/net/trace.Trace instance; nil if not tracing to x/net/trace. + netTr trace.Trace + // External opentracing compatible tracer such as lightstep, zipkin, jaeger; + // zero if not using one. + ot otSpan +} + +func (s *spanInner) TraceID() uint64 { + return s.crdb.traceID +} + +func (s *spanInner) isNoop() bool { + return s.crdb == nil && s.netTr == nil && s.ot == (otSpan{}) +} + +func (s *spanInner) IsVerbose() bool { + return s.crdb.recordingType() == RecordingVerbose +} + +func (s *spanInner) SetVerbose(to bool) { + // TODO(tbg): when always-on tracing is firmly established, we can remove the ugly + // caveat that SetVerbose(true) is a panic on a noop span because there will be no + // noop span. + if s.isNoop() { + panic(errors.AssertionFailedf("SetVerbose called on NoopSpan; use the WithForceRealSpan option for StartSpan")) + } + if to { + s.crdb.enableRecording(nil /* parent */, RecordingVerbose) + } else { + s.crdb.disableRecording() + } +} + +func (s *spanInner) SetVerboseRecursively(to bool) { + s.SetVerbose(to) + s.crdb.setVerboseRecursively(to) +} + +func (s *spanInner) ResetRecording() { + s.crdb.resetRecording() +} + +func (s *spanInner) GetRecording() Recording { + // If the span is not verbose, optimize by avoiding the tags. + // This span is likely only used to carry payloads around. + wantTags := s.crdb.recordingType() == RecordingVerbose + return s.crdb.getRecording(s.tracer.TracingVerbosityIndependentSemanticsIsActive(), wantTags) +} + +func (s *spanInner) ImportRemoteSpans(remoteSpans []tracingpb.RecordedSpan) error { + return s.crdb.importRemoteSpans(remoteSpans) +} + +func (s *spanInner) Finish() { + if s == nil { + return + } + if s.isNoop() { + return + } + finishTime := timeutil.Now() + + s.crdb.mu.Lock() + if alreadyFinished := s.crdb.mu.duration >= 0; alreadyFinished { + s.crdb.mu.Unlock() + + // External spans and net/trace are not always forgiving about spans getting + // finished twice, but it may happen so let's be resilient to it. + return + } + s.crdb.mu.duration = finishTime.Sub(s.crdb.startTime) + if s.crdb.mu.duration == 0 { + s.crdb.mu.duration = time.Nanosecond + } + s.crdb.mu.Unlock() + + if s.ot.shadowSpan != nil { + s.ot.shadowSpan.Finish() + } + if s.netTr != nil { + s.netTr.Finish() + } + s.tracer.activeSpans.Lock() + delete(s.tracer.activeSpans.m, s.crdb.spanID) + s.tracer.activeSpans.Unlock() +} + +func (s *spanInner) Meta() *SpanMeta { + var traceID uint64 + var spanID uint64 + var recordingType RecordingType + var baggage map[string]string + + if s.crdb != nil { + traceID, spanID = s.crdb.traceID, s.crdb.spanID + s.crdb.mu.Lock() + defer s.crdb.mu.Unlock() + n := len(s.crdb.mu.baggage) + // In the common case, we have no baggage, so avoid making an empty map. + if n > 0 { + baggage = make(map[string]string, n) + } + for k, v := range s.crdb.mu.baggage { + baggage[k] = v + } + recordingType = s.crdb.mu.recording.recordingType.load() + } + + var shadowTrTyp string + var shadowCtx opentracing.SpanContext + if s.ot.shadowSpan != nil { + shadowTrTyp, _ = s.ot.shadowTr.Type() + shadowCtx = s.ot.shadowSpan.Context() + } + + if traceID == 0 && + spanID == 0 && + shadowTrTyp == "" && + shadowCtx == nil && + recordingType == 0 && + baggage == nil { + return nil + } + return &SpanMeta{ + traceID: traceID, + spanID: spanID, + shadowTracerType: shadowTrTyp, + shadowCtx: shadowCtx, + recordingType: recordingType, + Baggage: baggage, + } +} + +func (s *spanInner) SetOperationName(operationName string) *spanInner { + if s.isNoop() { + return s + } + if s.ot.shadowSpan != nil { + s.ot.shadowSpan.SetOperationName(operationName) + } + s.crdb.operation = operationName + return s +} + +func (s *spanInner) SetTag(key string, value interface{}) *spanInner { + if s.isNoop() { + return s + } + return s.setTagInner(key, value, false /* locked */) +} + +func (s *spanInner) setTagInner(key string, value interface{}, locked bool) *spanInner { + if s.ot.shadowSpan != nil { + s.ot.shadowSpan.SetTag(key, value) + } + if s.netTr != nil { + s.netTr.LazyPrintf("%s:%v", key, value) + } + // The internal tags will be used if we start a recording on this Span. + if !locked { + s.crdb.mu.Lock() + defer s.crdb.mu.Unlock() + } + s.crdb.setTagLocked(key, value) + return s +} + +func (s *spanInner) RecordStructured(item Structured) { + if s.isNoop() { + return + } + s.crdb.recordStructured(item) + if s.hasVerboseSink() { + // NB: TrimSpace avoids the trailing whitespace generated by the + // protobuf stringers. + s.Record(strings.TrimSpace(item.String())) + } +} + +func (s *spanInner) Record(msg string) { + s.Recordf("%s", msg) +} + +func (s *spanInner) Recordf(format string, args ...interface{}) { + if !s.hasVerboseSink() { + return + } + str := fmt.Sprintf(format, args...) + if s.ot.shadowSpan != nil { + s.ot.shadowSpan.LogFields(otlog.String(tracingpb.LogMessageField, str)) + } + if s.netTr != nil { + s.netTr.LazyPrintf(format, args) + } + s.crdb.record(str) +} + +// hasVerboseSink returns false if there is no reason to even evaluate Record +// because the result wouldn't be used for anything. +func (s *spanInner) hasVerboseSink() bool { + if s.netTr == nil && s.ot == (otSpan{}) && !s.IsVerbose() { + return false + } + return true +} + +func (s *spanInner) SetBaggageItem(restrictedKey, value string) *spanInner { + if s.isNoop() { + return s + } + s.crdb.setBaggageItemAndTag(restrictedKey, value) + if s.ot.shadowSpan != nil { + s.ot.shadowSpan.SetBaggageItem(restrictedKey, value) + s.ot.shadowSpan.SetTag(restrictedKey, value) + } + // NB: nothing to do for net/trace. + + return s +} + +// Tracer exports the tracer this span was created using. +func (s *spanInner) Tracer() *Tracer { + return s.tracer +} From faf750065fea591f649ac9d61f1701f88f56c4bc Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Fri, 5 Mar 2021 22:15:59 +0100 Subject: [PATCH 3/3] kvnemesis: increase step count during TeamCity nightlies The nightly TeamCity stress tests currently run kvnemesis tests for 1 hour. However, each test run only has 50 steps (operations), which completes in seconds and has limited complexity. This commit makes the step count configurable via the environment variable `COCKROACH_KVNEMESIS_STEPS` (default 50), and sets it to 10000 for the nightly TeamCity tests (takes about 3 minutes to run locally). Release justification: non-production code changes Release note: None --- pkg/cmd/teamcity-trigger/main.go | 11 ++++++----- pkg/cmd/teamcity-trigger/main_test.go | 2 ++ pkg/kv/kvnemesis/BUILD.bazel | 1 + pkg/kv/kvnemesis/kvnemesis.go | 8 ++++++-- pkg/kv/kvnemesis/kvnemesis_test.go | 11 +++++++++-- 5 files changed, 24 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/teamcity-trigger/main.go b/pkg/cmd/teamcity-trigger/main.go index 3edbdcd3d3a5..7ee58578798d 100644 --- a/pkg/cmd/teamcity-trigger/main.go +++ b/pkg/cmd/teamcity-trigger/main.go @@ -81,11 +81,16 @@ func runTC(queueBuild func(string, map[string]string)) { // halve these values. parallelism := 4 - // Conditionally override stressflags. + opts := map[string]string{ + "env.PKG": importPath, + } + + // Conditionally override settings. switch importPath { case baseImportPath + "kv/kvnemesis": // Disable -maxruns for kvnemesis. Run for the full 1h. maxRuns = 0 + opts["env.COCKROACH_KVNEMESIS_STEPS"] = "10000" case baseImportPath + "sql/logictest": // Stress logic tests with reduced parallelism (to avoid overloading the // machine, see https://github.com/cockroachdb/cockroach/pull/10966). @@ -95,10 +100,6 @@ func runTC(queueBuild func(string, map[string]string)) { maxTime = 3 * time.Hour } - opts := map[string]string{ - "env.PKG": importPath, - } - opts["env.TESTTIMEOUT"] = testTimeout.String() // Run non-race build. diff --git a/pkg/cmd/teamcity-trigger/main_test.go b/pkg/cmd/teamcity-trigger/main_test.go index d32623544585..1eac86e6b467 100644 --- a/pkg/cmd/teamcity-trigger/main_test.go +++ b/pkg/cmd/teamcity-trigger/main_test.go @@ -64,11 +64,13 @@ func Example_runTC() { // Output: // github.com/cockroachdb/cockroach/pkg/kv/kvnemesis + // env.COCKROACH_KVNEMESIS_STEPS: 10000 // env.GOFLAGS: -parallel=4 // env.STRESSFLAGS: -maxruns 0 -maxtime 1h0m0s -maxfails 1 -p 4 // env.TESTTIMEOUT: 40m0s // // github.com/cockroachdb/cockroach/pkg/kv/kvnemesis + // env.COCKROACH_KVNEMESIS_STEPS: 10000 // env.GOFLAGS: -race -parallel=4 // env.STRESSFLAGS: -maxruns 0 -maxtime 1h0m0s -maxfails 1 -p 1 // env.TESTTIMEOUT: 40m0s diff --git a/pkg/kv/kvnemesis/BUILD.bazel b/pkg/kv/kvnemesis/BUILD.bazel index e0799da4cf21..2ed9174d2817 100644 --- a/pkg/kv/kvnemesis/BUILD.bazel +++ b/pkg/kv/kvnemesis/BUILD.bazel @@ -67,6 +67,7 @@ go_test( "//pkg/testutils/skip", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", + "//pkg/util/envutil", "//pkg/util/hlc", "//pkg/util/leaktest", "//pkg/util/log", diff --git a/pkg/kv/kvnemesis/kvnemesis.go b/pkg/kv/kvnemesis/kvnemesis.go index b2c374d5fa0f..e323f6b45339 100644 --- a/pkg/kv/kvnemesis/kvnemesis.go +++ b/pkg/kv/kvnemesis/kvnemesis.go @@ -36,9 +36,13 @@ func RunNemesis( rng *rand.Rand, ct ClosedTimestampTargetInterval, config GeneratorConfig, + numSteps int, dbs ...*kv.DB, ) ([]error, error) { - const concurrency, numSteps = 5, 30 + const concurrency = 5 + if numSteps <= 0 { + return nil, fmt.Errorf("numSteps must be >0, got %v", numSteps) + } g, err := MakeGenerator(config, newGetReplicasFn(dbs...)) if err != nil { @@ -57,7 +61,7 @@ func RunNemesis( workerFn := func(ctx context.Context, workerIdx int) error { workerName := fmt.Sprintf(`%d`, workerIdx) var buf strings.Builder - for atomic.AddInt64(&stepsStartedAtomic, 1) <= numSteps { + for atomic.AddInt64(&stepsStartedAtomic, 1) <= int64(numSteps) { step := g.RandStep(rng) recCtx, collect, cancel := tracing.ContextWithRecordingSpan( diff --git a/pkg/kv/kvnemesis/kvnemesis_test.go b/pkg/kv/kvnemesis/kvnemesis_test.go index 86c8153f3a9d..fd0bf325ed79 100644 --- a/pkg/kv/kvnemesis/kvnemesis_test.go +++ b/pkg/kv/kvnemesis/kvnemesis_test.go @@ -22,12 +22,19 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" + "github.com/cockroachdb/cockroach/pkg/util/envutil" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/randutil" "github.com/stretchr/testify/require" ) +var numSteps int + +func init() { + numSteps = envutil.EnvOrDefaultInt("COCKROACH_KVNEMESIS_STEPS", 50) +} + func TestKVNemesisSingleNode(t *testing.T) { defer leaktest.AfterTest(t)() skip.UnderRace(t) @@ -45,7 +52,7 @@ func TestKVNemesisSingleNode(t *testing.T) { config.NumNodes, config.NumReplicas = 1, 1 rng, _ := randutil.NewPseudoRand() ct := sqlClosedTimestampTargetInterval{sqlDBs: []*gosql.DB{sqlDB}} - failures, err := RunNemesis(ctx, rng, ct, config, db) + failures, err := RunNemesis(ctx, rng, ct, config, numSteps, db) require.NoError(t, err, `%+v`, err) for _, failure := range failures { @@ -78,7 +85,7 @@ func TestKVNemesisMultiNode(t *testing.T) { config.NumNodes, config.NumReplicas = numNodes, 3 rng, _ := randutil.NewPseudoRand() ct := sqlClosedTimestampTargetInterval{sqlDBs: sqlDBs} - failures, err := RunNemesis(ctx, rng, ct, config, dbs...) + failures, err := RunNemesis(ctx, rng, ct, config, numSteps, dbs...) require.NoError(t, err, `%+v`, err) for _, failure := range failures {