Merge #85277 #86126 #86233 #86278 #86300 #86318 #86331

85277: roachtest: add mixed version testing for CDC. r=srosenberg a=renatolabs

This commits adds the `cdc/mixed-versions` roachtest. It builds on top
of the existing infrastructure for mixed version testing in order to
create a test scenario that reproduces the issue observed by DoorDash
when upgrading from 21.2 to 22.1.0.

Following the pattern present in other mixed version tests, this test
starts the cluster at the previous version, upgrades the binaries to
the current version, rolls it back, and then finally performs the
binary upgrade again and allowing the upgrade to finalize.

This roachtest uses the `FingerprintValidator` infrastructure used in
CDC unit tests (and also in the currently skipped `cdc/bank`
roachtest). This validator gives us strong guarantees that the events
produced by a changefeed are correct even in the face of ongoing
upgrades.

This test fails roachtest on v22.1.0; once the upgrade to v22.1 is
finalized, the Kafka consumer won't see further updates, and the test
eventually times out.

Release justification: test-only changes.
Release note: None.

86126: go.mod: upgrade twpayne/go-geom r=rafiss a=otan

This fixes ARM specific bugs.

Release note (bug fix): Intersection spatial operations would previously
potentially return incorrect results on ARM. This is now resolved.

Release justification: bug fix for existing functionality

Resolves #72226

86233: sql: remove super regions version and gates r=postamar a=RichardJCai

Release justification: Only removing old version.
Release note: None

86278: sql: udf logic tests not to skip dropping databases r=chengxiong-ruan a=chengxiong-ruan

Now that we support drop functions in legacy schema changer
and declarative schema changer falls back to legacy when seeing
any function descriptor, we can move udf logic tests back
to normal configs.

Release note: None.
Release justification: test only change.

86300: roachtest: own a few tests to test-eng r=srosenberg a=tbg

See commits.

Closes #82060.

Release justification: testing

86318: scplan: make EXPLAIN target output stable r=postamar a=postamar

This commit does not make any functional changes. It merely ensures that
the diagrams generated by an EXPLAIN (DDL) statement list the schema
change targets in a predefined order: first the to-public targets, then
the transient targets, and finally the to-absent targets.

This helps readability and makes the declarative schema changer
data-driven test output more stable.

Release justification: low-impact, test-only change
Release note: None

86331: roachtest: enable better vmodules for drain/quit roachtests r=kvoli a=aayushshah15

This should help in debugging failures like #85203 when they happen again.

Release note: None

Release justification: testing only

Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: richardjcai <[email protected]>
Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Aayush Shah <[email protected]>

go.mod

@@ -58,7 +58,7 @@ require (
 	github.com/coreos/go-oidc v2.2.1+incompatible
 	github.com/dave/dst v0.24.0
 	github.com/docker/distribution v2.7.1+incompatible
-	github.com/docker/docker v20.10.8+incompatible
+	github.com/docker/docker v20.10.17+incompatible
 	github.com/docker/go-connections v0.4.0
 	github.com/dustin/go-humanize v1.0.0
 	github.com/edsrzf/mmap-go v1.0.0
@@ -139,8 +139,8 @@ require (
 	github.com/spf13/afero v1.6.0
 	github.com/spf13/cobra v1.2.1
 	github.com/spf13/pflag v1.0.5
-	github.com/stretchr/testify v1.7.0
-	github.com/twpayne/go-geom v1.4.1
+	github.com/stretchr/testify v1.8.0
+	github.com/twpayne/go-geom v1.4.2
 	github.com/wadey/gocovmerge v0.0.0-20160331181800-b5bfa59ec0ad
 	github.com/xdg-go/pbkdf2 v1.0.0
 	github.com/xdg-go/scram v1.0.2
@@ -156,11 +156,11 @@ require (
 	golang.org/x/crypto v0.0.0-20220518034528-6f7dac969898
 	golang.org/x/exp v0.0.0-20220104160115-025e73f80486
 	golang.org/x/lint v0.0.0-20210508222113-6edffad5e616
-	golang.org/x/net v0.0.0-20220524220425-1d687d428aca
+	golang.org/x/net v0.0.0-20220722155237-a158d28d115b
 	golang.org/x/oauth2 v0.0.0-20220411215720-9780585627b5
 	golang.org/x/perf v0.0.0-20180704124530-6e6d33e29852
 	golang.org/x/sync v0.0.0-20220513210516-0976fa681c29
-	golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a
+	golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f
 	golang.org/x/term v0.0.0-20210927222741-03fcf44c2211
 	golang.org/x/text v0.3.7
 	golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac
@@ -170,7 +170,7 @@ require (
 	google.golang.org/grpc v1.46.0
 	google.golang.org/protobuf v1.28.0
 	gopkg.in/yaml.v2 v2.4.0
-	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
+	gopkg.in/yaml.v3 v3.0.1
 	honnef.co/go/tools v0.3.2
 	vitess.io/vitess v0.0.0-00010101000000-000000000000
 )
@@ -191,7 +191,7 @@ require (
 	github.com/Masterminds/goutils v1.1.0 // indirect
 	github.com/Masterminds/semver v1.5.0 // indirect
 	github.com/Masterminds/sprig v2.22.0+incompatible // indirect
-	github.com/Microsoft/go-winio v0.5.1 // indirect
+	github.com/Microsoft/go-winio v0.5.2 // indirect
 	github.com/PuerkitoBio/purell v1.1.1 // indirect
 	github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 // indirect
 	github.com/abbot/go-http-auth v0.4.1-0.20181019201920-860ed7f246ff // indirect
@@ -209,7 +209,7 @@ require (
 	github.com/aws/smithy-go v1.11.2 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/bgentry/go-netrc v0.0.0-20140422174119-9fd32a8b3d3d // indirect
-	github.com/cenkalti/backoff/v4 v4.1.2 // indirect
+	github.com/cenkalti/backoff/v4 v4.1.3 // indirect
 	github.com/cespare/xxhash/v2 v2.1.2 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/dimchansky/utfbom v1.1.1 // indirect
@@ -252,7 +252,7 @@ require (
 	github.com/hashicorp/hcl v1.0.0 // indirect
 	github.com/huandu/xstrings v1.3.0 // indirect
 	github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639 // indirect
-	github.com/imdario/mergo v0.3.12 // indirect
+	github.com/imdario/mergo v0.3.13 // indirect
 	github.com/inconshreveable/mousetrap v1.0.0 // indirect
 	github.com/jackc/chunkreader/v2 v2.0.1 // indirect
 	github.com/jackc/pgio v1.0.0 // indirect
@@ -284,8 +284,8 @@ require (
 	github.com/minio/sha256-simd v1.0.0 // indirect
 	github.com/mitchellh/copystructure v1.0.0 // indirect
 	github.com/mitchellh/go-homedir v1.1.0 // indirect
-	github.com/mitchellh/mapstructure v1.4.1 // indirect
-	github.com/moby/term v0.0.0-20201216013528-df9cb8a40635 // indirect
+	github.com/mitchellh/mapstructure v1.5.0 // indirect
+	github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6 // indirect
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/morikuni/aec v1.0.0 // indirect
@@ -307,7 +307,7 @@ require (
 	github.com/rs/xid v1.3.0 // indirect
 	github.com/russross/blackfriday v1.6.0 // indirect
 	github.com/russross/blackfriday/v2 v2.1.0 // indirect
-	github.com/sirupsen/logrus v1.8.1 // indirect
+	github.com/sirupsen/logrus v1.9.0 // indirect
 	github.com/slok/go-http-metrics v0.10.0 // indirect
 	github.com/spf13/cast v1.3.1 // indirect
 	github.com/spf13/jwalterweatherman v1.1.0 // indirect

pkg/BUILD.bazel

@@ -358,9 +358,7 @@ ALL_TESTS = [
     "//pkg/sql/logictest/tests/fakedist-disk:fakedist-disk_test",
     "//pkg/sql/logictest/tests/fakedist-vec-off:fakedist-vec-off_test",
     "//pkg/sql/logictest/tests/fakedist:fakedist_test",
-    "//pkg/sql/logictest/tests/local-mixed-21.2-22.1:local-mixed-21_2-22_1_test",
     "//pkg/sql/logictest/tests/local-mixed-22.1-22.2:local-mixed-22_1-22_2_test",
-    "//pkg/sql/logictest/tests/local-udf:local-udf_test",
     "//pkg/sql/logictest/tests/local-v1.1-at-v1.0-noupgrade:local-v1_1-at-v1_0-noupgrade_test",
     "//pkg/sql/logictest/tests/local-vec-off:local-vec-off_test",
     "//pkg/sql/logictest/tests/local:local_test",
@@ -377,7 +375,6 @@ ALL_TESTS = [
     "//pkg/sql/opt/exec/execbuilder/tests/fakedist-disk:fakedist-disk_test",
     "//pkg/sql/opt/exec/execbuilder/tests/fakedist-vec-off:fakedist-vec-off_test",
     "//pkg/sql/opt/exec/execbuilder/tests/fakedist:fakedist_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-udf:local-udf_test",
     "//pkg/sql/opt/exec/execbuilder/tests/local-vec-off:local-vec-off_test",
     "//pkg/sql/opt/exec/execbuilder/tests/local:local_test",
     "//pkg/sql/opt/exec/execbuilder:execbuilder_test",
@@ -1450,9 +1447,7 @@ GO_TARGETS = [
     "//pkg/sql/logictest/tests/fakedist-disk:fakedist-disk_test",
     "//pkg/sql/logictest/tests/fakedist-vec-off:fakedist-vec-off_test",
     "//pkg/sql/logictest/tests/fakedist:fakedist_test",
-    "//pkg/sql/logictest/tests/local-mixed-21.2-22.1:local-mixed-21_2-22_1_test",
     "//pkg/sql/logictest/tests/local-mixed-22.1-22.2:local-mixed-22_1-22_2_test",
-    "//pkg/sql/logictest/tests/local-udf:local-udf_test",
     "//pkg/sql/logictest/tests/local-v1.1-at-v1.0-noupgrade:local-v1_1-at-v1_0-noupgrade_test",
     "//pkg/sql/logictest/tests/local-vec-off:local-vec-off_test",
     "//pkg/sql/logictest/tests/local:local_test",
@@ -1478,7 +1473,6 @@ GO_TARGETS = [
     "//pkg/sql/opt/exec/execbuilder/tests/fakedist-disk:fakedist-disk_test",
     "//pkg/sql/opt/exec/execbuilder/tests/fakedist-vec-off:fakedist-vec-off_test",
     "//pkg/sql/opt/exec/execbuilder/tests/fakedist:fakedist_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-udf:local-udf_test",
     "//pkg/sql/opt/exec/execbuilder/tests/local-vec-off:local-vec-off_test",
     "//pkg/sql/opt/exec/execbuilder/tests/local:local_test",
     "//pkg/sql/opt/exec/execbuilder:execbuilder",
@@ -2566,9 +2560,7 @@ GET_X_DATA_TARGETS = [
     "//pkg/sql/logictest/tests/fakedist-disk:get_x_data",
     "//pkg/sql/logictest/tests/fakedist-vec-off:get_x_data",
     "//pkg/sql/logictest/tests/local:get_x_data",
-    "//pkg/sql/logictest/tests/local-mixed-21.2-22.1:get_x_data",
     "//pkg/sql/logictest/tests/local-mixed-22.1-22.2:get_x_data",
-    "//pkg/sql/logictest/tests/local-udf:get_x_data",
     "//pkg/sql/logictest/tests/local-v1.1-at-v1.0-noupgrade:get_x_data",
     "//pkg/sql/logictest/tests/local-vec-off:get_x_data",
     "//pkg/sql/logictest/tests/multiregion-9node-3region-3azs:get_x_data",
@@ -2589,7 +2581,6 @@ GET_X_DATA_TARGETS = [
     "//pkg/sql/opt/exec/execbuilder/tests/fakedist-disk:get_x_data",
     "//pkg/sql/opt/exec/execbuilder/tests/fakedist-vec-off:get_x_data",
     "//pkg/sql/opt/exec/execbuilder/tests/local:get_x_data",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-udf:get_x_data",
     "//pkg/sql/opt/exec/execbuilder/tests/local-vec-off:get_x_data",
     "//pkg/sql/opt/exec/explain:get_x_data",
     "//pkg/sql/opt/idxconstraint:get_x_data",

pkg/ccl/changefeedccl/cdctest/BUILD.bazel

@@ -31,6 +31,7 @@ go_library(
         "//pkg/util/hlc",
         "//pkg/util/log",
         "//pkg/util/randutil",
+        "//pkg/util/retry",
         "//pkg/util/syncutil",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",

pkg/ccl/changefeedccl/cdctest/nemeses.go

@@ -173,7 +173,7 @@ func RunNemesis(f TestFeedFactory, db *gosql.DB, isSinkless bool) (Validator, er
 	if err != nil {
 		return nil, err
 	}
-	fprintV, err := NewFingerprintValidator(db, `foo`, scratchTableName, foo.Partitions(), ns.maxTestColumnCount)
+	fprintV, err := NewFingerprintValidator(db, `foo`, scratchTableName, foo.Partitions(), ns.maxTestColumnCount, false)
 	if err != nil {
 		return nil, err
 	}

pkg/ccl/changefeedccl/cdctest/validator.go

@@ -15,9 +15,11 @@ import (
 	"fmt"
 	"sort"
 	"strings"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
+	"github.com/cockroachdb/cockroach/pkg/util/retry"
 	"github.com/cockroachdb/errors"
 )
 
@@ -63,6 +65,8 @@ var _ Validator = &orderValidator{}
 var _ Validator = &noOpValidator{}
 var _ StreamValidator = &orderValidator{}
 
+var retryDuration = 2 * time.Minute
+
 type noOpValidator struct{}
 
 // NoteRow accepts a changed row entry.
@@ -343,6 +347,11 @@ type fingerprintValidator struct {
 	fprintTestColumns int
 	buffer            []validatorRow
 
+	// shouldRetry indicates whether row updates should be retried (for
+	// a fixed duration). Typically used when the transient errors are
+	// expected (e.g., if performing an upgrade)
+	shouldRetry bool
+
 	failures []string
 }
 
@@ -353,7 +362,11 @@ type fingerprintValidator struct {
 // will modify `fprint`'s schema to add `maxTestColumnCount` columns to avoid having to
 // accommodate schema changes on the fly.
 func NewFingerprintValidator(
-	sqlDB *gosql.DB, origTable, fprintTable string, partitions []string, maxTestColumnCount int,
+	sqlDB *gosql.DB,
+	origTable, fprintTable string,
+	partitions []string,
+	maxTestColumnCount int,
+	shouldRetry bool,
 ) (Validator, error) {
 	// Fetch the primary keys though information_schema schema inspections so we
 	// can use them to construct the SQL for DELETEs and also so we can verify
@@ -395,6 +408,7 @@ func NewFingerprintValidator(
 		primaryKeyCols:    primaryKeyCols,
 		fprintOrigColumns: fprintOrigColumns,
 		fprintTestColumns: maxTestColumnCount,
+		shouldRetry:       shouldRetry,
 	}
 	v.partitionResolved = make(map[string]hlc.Timestamp)
 	for _, partition := range partitions {
@@ -475,10 +489,16 @@ func (v *fingerprintValidator) applyRowUpdate(row validatorRow) (_err error) {
 			return err
 		}
 
-		if string(primaryKeyJSON) != row.key {
+		rowKey := row.key
+		if len(primaryKeyDatums) > 1 {
+			// format the key using the Go marshaller; otherwise, differences
+			// in formatting could lead to the comparison below failing
+			rowKey = asGoJSON(row.key)
+		}
+		if string(primaryKeyJSON) != rowKey {
 			v.failures = append(v.failures,
 				fmt.Sprintf(`key %s did not match expected key %s for value %s`,
-					row.key, primaryKeyJSON, row.value))
+					rowKey, primaryKeyJSON, row.value))
 		}
 	} else {
 		// DELETE
@@ -491,8 +511,11 @@ func (v *fingerprintValidator) applyRowUpdate(row validatorRow) (_err error) {
 			args = append(args, datum)
 		}
 	}
-	_, err := v.sqlDB.Exec(stmtBuf.String(), args...)
-	return err
+
+	return v.maybeRetry(func() error {
+		_, err := v.sqlDB.Exec(stmtBuf.String(), args...)
+		return err
+	})
 }
 
 // NoteResolved implements the Validator interface.
@@ -533,6 +556,9 @@ func (v *fingerprintValidator) NoteResolved(partition string, resolved hlc.Times
 			break
 		}
 		row := v.buffer[0]
+		// NOTE: changes to the validator's state before `applyRowUpdate`
+		// are safe because, if the operation can fail, the caller should
+		// be setting the `shouldRetry` field accordingly
 		v.buffer = v.buffer[1:]
 
 		// If we've processed all row updates belonging to the previous row's timestamp,
@@ -568,15 +594,19 @@ func (v *fingerprintValidator) NoteResolved(partition string, resolved hlc.Times
 
 func (v *fingerprintValidator) fingerprint(ts hlc.Timestamp) error {
 	var orig string
-	if err := v.sqlDB.QueryRow(`SELECT IFNULL(fingerprint, 'EMPTY') FROM [
+	if err := v.maybeRetry(func() error {
+		return v.sqlDB.QueryRow(`SELECT IFNULL(fingerprint, 'EMPTY') FROM [
 		SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE ` + v.origTable + `
-	] AS OF SYSTEM TIME '` + ts.AsOfSystemTime() + `'`).Scan(&orig); err != nil {
+	] AS OF SYSTEM TIME '` + ts.AsOfSystemTime() + `'`).Scan(&orig)
+	}); err != nil {
 		return err
 	}
 	var check string
-	if err := v.sqlDB.QueryRow(`SELECT IFNULL(fingerprint, 'EMPTY') FROM [
+	if err := v.maybeRetry(func() error {
+		return v.sqlDB.QueryRow(`SELECT IFNULL(fingerprint, 'EMPTY') FROM [
 		SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE ` + v.fprintTable + `
-	]`).Scan(&check); err != nil {
+	]`).Scan(&check)
+	}); err != nil {
 		return err
 	}
 	if orig != check {
@@ -591,6 +621,17 @@ func (v *fingerprintValidator) Failures() []string {
 	return v.failures
 }
 
+// maybeRetry will retry the function passed if the fingerprint was
+// created with `shouldRetry` set to `true`. Every access to `sqlDB`
+// should be made my closures passed to this function
+func (v *fingerprintValidator) maybeRetry(f func() error) error {
+	if v.shouldRetry {
+		return retry.ForDuration(retryDuration, f)
+	}
+
+	return f()
+}
+
 // Validators abstracts over running multiple `Validator`s at once on the same
 // feed.
 type Validators []Validator
@@ -731,3 +772,22 @@ func fetchPrimaryKeyCols(sqlDB *gosql.DB, tableStr string) ([]string, error) {
 	}
 	return primaryKeyCols, nil
 }
+
+// asGoJSON tries to unmarshal the given string as JSON; if
+// successful, the struct is marshalled back to JSON. This is to
+// enforce the default formatting of the standard library marshaller,
+// allowing comparisons of JSON strings when we don't control the
+// formatting of the strings.
+func asGoJSON(s string) string {
+	var obj interface{}
+	if err := gojson.Unmarshal([]byte(s), &obj); err != nil {
+		return s
+	}
+
+	blob, err := gojson.Marshal(obj)
+	if err != nil {
+		return s
+	}
+
+	return string(blob)
+}