Merge #67526 #67824 #67825 #67894 #67916 #67961 #67962 #68065

67526: roachtest: make timeout obvious in posted issues r=stevendanna a=tbg

When a test times out, roachtest will rip the cluster out from under it
to try to force it to terminate. This is essentially guaranteed to
produce a posted issue that sweeps the original reason of the failure
(the timeout) under the rug. Instead, such issues now plainly state
that there was a timeout and refer the readers to the artifacts.

See here for an example issue without this fix: #67464

cc @dt, who pointed this out [internally]

[internally]: https://cockroachlabs.slack.com/archives/C023S0V4YEB/p1626098863019500

Release note: None


67824: dev: teach `dev` how to do cross builds r=rail a=rickystewart

Closes #67709.

Release note: None

67825: changefeedccl: immediately stop sending webhook sink rows upon error r=spiffyyeng a=spiffyyeng

Previously, the sink waited until flushing to acknowledge HTTP errors, leaving
any messages between the initial error and flush to potentially be out of
order. Now, errors are checked before each message is sent and the sink is
restarted if one is detected to maintain ordering.

Resolves #67772

Release note: None

67894: sql: add support for unique expression indexes r=mgartner a=mgartner

Release note: None

67916: roachtest: fix replicagc-changed-peers r=aliher1911 a=tbg

The test ends up in the following situation:

n1: down, no replicas
n2: down, no replicas
n3: alive, with constraint that wants all replicas to move,
    and there may be a few ranges still on n3
n4-n6: alive

where the ranges predominantly 3x-replicated.

The test is then verifying that the replica count (as in, replicas on
n3, in contrast to replicas assigned via the meta ranges) on n3 drops to
zero.

However, system ranges cannot move in this configuration. The number of
cluster nodes is six (decommission{ing,ed} nodes would be excluded, but
no nodes are decommission{ing,ed} here) and so the system ranges operate
at a replication factor of five. There are only four live nodes here, so
if n3 is still a member of any system ranges, they will stay there and
the test fails.

This commit attempts to rectify that by making sure that while n3 is
down earlier in the test, all replicas are moved from it. That was
always the intent of the test, which is concerned with n3 realizing
that replicas have moved elsewhere and initiating replicaGC; however
prior to this commit it was always left to chance whether n3 would
or would not have replicas assigned to it by the time the test moved
to the stage above. The reason the test wasn't previously waiting
for all replicas to be moved off n3 while it was down was that it
required checking the meta ranges, which wasn't necessary for the
other two nodes.

This commit passed all five runs of
replicagc-changed-peers/restart=false, so I think it reliably addresses
the problem.

There is still the lingering question of why this is failing only now
(note that both flavors of the test failed on master last night, so
I doubt it is rare). We just merged
#67319 which is likely
somehow related.

Fixes #67910.
Fixes #67914.

Release note: None


67961: bazel: use `action_config`s over `tool_path`s in cross toolchains r=rail a=rickystewart

This doesn't change much in practice, but does allow us to use the
actual `g++` compiler for C++ compilation, which wasn't the case
before.

The `tool_path` constructor is actually [deprecated](https://github.com/bazelbuild/bazel/blob/203aa773d7109a0bcd9777ba6270bd4fd0edb69f/tools/cpp/cc_toolchain_config_lib.bzl#L419)
in favor of `action_config`s, so this is future-proofing.

Release note: None

67962: bazel: start building geos in ci r=rail a=rickystewart

Only the most recent commit applies for this review --
the other is from #67961.

Closes #66388.

Release note: None

68065: cli: skip TestRemoveDeadReplicas r=irfansharif a=tbg

Refs: #50977

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None

Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Ryan Min <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>

build/bazelutil/bazelbuild.sh

@@ -11,4 +11,4 @@ fi
 bazel build //pkg/cmd/bazci --config=ci
 $(bazel info bazel-bin)/pkg/cmd/bazci/bazci_/bazci --compilation_mode opt \
 		       --config "$1" \
-		       build //pkg/cmd/cockroach-short
+		       build //pkg/cmd/cockroach-short //c-deps:libgeos

build/toolchains/crosstool-ng/BUILD.tmpl

@@ -42,7 +42,7 @@ filegroup(
 filegroup(
     name = "linker_files",
     srcs = [
-        "bin/%{target}-gcc",
+        "bin/%{target}-g++",
     ],
 )
 

build/toolchains/crosstool-ng/cc_toolchain_config.bzl.tmpl

@@ -1,63 +1,43 @@
 load("@bazel_tools//tools/build_defs/cc:action_names.bzl", "ACTION_NAMES")
 load("@bazel_tools//tools/cpp:cc_toolchain_config_lib.bzl",
+     "action_config",
      "feature",
      "flag_group",
      "flag_set",
-     "tool_path")
+     "tool")
 
 all_compile_actions = [
     ACTION_NAMES.c_compile,
     ACTION_NAMES.cpp_compile,
-    ACTION_NAMES.linkstamp_compile,
-    ACTION_NAMES.assemble,
-    ACTION_NAMES.preprocess_assemble,
-    ACTION_NAMES.cpp_header_parsing,
-    ACTION_NAMES.cpp_module_compile,
-    ACTION_NAMES.cpp_module_codegen,
-    ACTION_NAMES.clif_match,
-    ACTION_NAMES.lto_backend,
 ]
 
 all_link_actions = [
     ACTION_NAMES.cpp_link_executable,
-    ACTION_NAMES.cpp_link_dynamic_library,
-    ACTION_NAMES.cpp_link_nodeps_dynamic_library,
+]
+
+all_archive_actions = [
+    ACTION_NAMES.cpp_link_static_library,
 ]
 
 def _impl(ctx):
-    tool_paths = [
-        tool_path(
-            name = "gcc",
-            path = "bin/%{target}-gcc",
-        ),
-        tool_path(
-            name = "ld",
-            path = "bin/%{target}-ld",
-        ),
-        tool_path(
-            name = "cpp",
-            path = "bin/%{target}-g++",
-        ),
-        tool_path(
-            name = "gcov",
-            path = "bin/%{target}-gcov",
-        ),
-        tool_path(
-            name = "nm",
-            path = "bin/%{target}-nm",
+    action_configs = [
+        action_config(
+            action_name = ACTION_NAMES.c_compile,
+            tools = [tool(path="bin/%{target}-gcc")],
         ),
-        tool_path(
-            name = "objdump",
-            path = "bin/%{target}-objdump",
+        action_config(
+            action_name = ACTION_NAMES.cpp_compile,
+            tools = [tool(path="bin/%{target}-g++")],
         ),
-        tool_path(
-            name = "strip",
-            path = "bin/%{target}-strip",
+        action_config(
+            action_name = ACTION_NAMES.cpp_link_executable,
+            tools = [tool(path="bin/%{target}-g++")],
         ),
-        tool_path(
-            name = "ar",
-            path = "bin/%{target}-ar",
+        action_config(
+            action_name = ACTION_NAMES.cpp_link_static_library,
+            tools = [tool(path="bin/%{target}-ar")],
         ),
+
     ]
 
     opt_feature = feature(
@@ -95,6 +75,38 @@ def _impl(ctx):
     supports_pic_feature = feature(name = "supports_pic", enabled = True)
     supports_dynamic_linker_feature = feature(name = "supports_dynamic_linker", enabled = False)
 
+    default_archiver_flags = feature(
+        name = "archiver_flags",
+        enabled = True,
+        flag_sets = [
+            flag_set(
+                actions = all_archive_actions,
+                flag_groups = [
+                    flag_group(flags = ["rcsD"]),
+                    flag_group(
+                        flags = ["%{output_execpath}"],
+                        expand_if_available = "output_execpath",
+                    ),
+                ],
+            ),
+            flag_set(
+                actions = all_archive_actions,
+                flag_groups = [
+                    flag_group(
+                        iterate_over = "libraries_to_link",
+                        flag_groups = [
+                            flag_group(
+                                flags = ["%{libraries_to_link.name}"],
+                            ),
+                        ],
+                        expand_if_available = "libraries_to_link",
+                    ),
+                ],
+            ),
+        ],
+    )
+
+
     default_compile_flags = feature(
         name = "default_compile_flags",
         enabled = True,
@@ -137,6 +149,7 @@ def _impl(ctx):
         supports_dynamic_linker_feature,
         default_compile_flags,
         default_linker_flags,
+        default_archiver_flags,
     ]
 
     return cc_common.create_cc_toolchain_config_info(
@@ -150,7 +163,7 @@ def _impl(ctx):
         compiler = "clang",
         abi_version = "clang-10.0.0",
         abi_libc_version = "%{target}",
-        tool_paths = tool_paths,
+        action_configs = action_configs,
         cxx_builtin_include_directories = [
             "%sysroot%/usr/include",
             "%{repo_path}/%{target}/include/c++/6.5.0",

c-deps/BUILD.bazel

@@ -66,39 +66,47 @@ cmake(
     visibility = ["//visibility:public"],
 )
 
-# TODO(ricky): Still broken on Windows.
 # Define the targets for libgeos.
 cmake(
     name = "libgeos",
-    cache_entries = {
-        "CMAKE_BUILD_TYPE": "Release",
-        "CMAKE_C_FLAGS": "-fPIC",
-        "CMAKE_CXX_FLAGS": "-fPIC",
-    },
-    # As of this writing (2021-05-05), foreign_cc
-    # only knows about windows, darwin and linux.
-    cmake_options = select({
-        "@io_bazel_rules_go//go/platform:windows": ["-GNinja"],
-        "//conditions:default": ["-GUnix Makefiles"],
+    cache_entries = select({
+        "@io_bazel_rules_go//go/platform:windows": {
+            "CMAKE_BUILD_TYPE": "Release",
+            "CMAKE_C_FLAGS": "-fPIC",
+            "CMAKE_CXX_FLAGS": "-fPIC",
+            "CMAKE_SYSTEM_NAME": "Windows",
+        },
+        "@io_bazel_rules_go//go/platform:darwin": {
+            "CMAKE_BUILD_TYPE": "Release",
+            "CMAKE_C_FLAGS": "-fPIC",
+            "CMAKE_CXX_FLAGS": "-fPIC",
+            "CMAKE_SYSTEM_NAME": "Darwin",
+        },
+        "//conditions:default": {
+            "CMAKE_BUILD_TYPE": "Release",
+            "CMAKE_C_FLAGS": "-fPIC",
+            "CMAKE_CXX_FLAGS": "-fPIC",
+        },
     }),
+    cmake_options = ["-GUnix Makefiles"],
     lib_source = "@geos//:all",
     make_commands = [
         "mkdir -p libgeos/lib",
         "make --no-print-directory geos_c",
     ] + select({
         "@io_bazel_rules_go//go/platform:darwin": [
-            "cp -L $BUILD_TMPDIR/lib/libgeos.dylib libgeos/lib",
-            "cp -L $BUILD_TMPDIR/lib/libgeos_c.dylib libgeos/lib",
+            "cp -L lib/libgeos.dylib libgeos/lib",
+            "cp -L lib/libgeos_c.dylib libgeos/lib",
             # TODO(#bazel): install_name_tool is also required here for release.
         ],
         "@io_bazel_rules_go//go/platform:windows": [
             # NOTE: Windows ends up in bin/ on the BUILD_TMPDIR.
-            "cp -L $BUILD_TMPDIR/bin/libgeos.dll libgeos/lib",
-            "cp -L $BUILD_TMPDIR/bin/libgeos_c.dll libgeos/lib",
+            "cp -L bin/libgeos.dll libgeos/lib",
+            "cp -L bin/libgeos_c.dll libgeos/lib",
         ],
         "//conditions:default": [
-            "cp -L $BUILD_TMPDIR/lib/libgeos.so libgeos/lib",
-            "cp -L $BUILD_TMPDIR/lib/libgeos_c.so libgeos/lib",
+            "cp -L lib/libgeos.so libgeos/lib",
+            "cp -L lib/libgeos_c.so libgeos/lib",
             # TODO(#bazel): patchelf is also required here for release.
         ],
     }),

pkg/ccl/changefeedccl/changefeed_test.go

@@ -4011,7 +4011,7 @@ func TestChangefeedBackfillCheckpoint(t *testing.T) {
 
 	rnd, _ := randutil.NewPseudoRand()
 
-	var maxCheckopointSize int64
+	var maxCheckpointSize int64
 	testFn := func(t *testing.T, db *gosql.DB, f cdctest.TestFeedFactory) {
 		sqlDB := sqlutils.MakeSQLRunner(db)
 		sqlDB.Exec(t, `CREATE TABLE foo(key INT PRIMARY KEY DEFAULT unique_rowid(), val INT)`)
@@ -4055,7 +4055,7 @@ func TestChangefeedBackfillCheckpoint(t *testing.T) {
 		changefeedbase.FrontierCheckpointFrequency.Override(
 			context.Background(), &f.Server().ClusterSettings().SV, 10*time.Millisecond)
 		changefeedbase.FrontierCheckpointMaxBytes.Override(
-			context.Background(), &f.Server().ClusterSettings().SV, maxCheckopointSize)
+			context.Background(), &f.Server().ClusterSettings().SV, maxCheckpointSize)
 
 		registry := f.Server().JobRegistry().(*jobs.Registry)
 		foo := feed(t, f, `CREATE CHANGEFEED FOR foo WITH resolved='100ms'`)
@@ -4138,7 +4138,7 @@ func TestChangefeedBackfillCheckpoint(t *testing.T) {
 
 	// TODO(ssd): Tenant testing disabled because of use of DB()
 	for _, sz := range []int64{100 << 20, 100} {
-		maxCheckopointSize = sz
+		maxCheckpointSize = sz
 		t.Run(fmt.Sprintf("enterprise-limit=%s", humanize.Bytes(uint64(sz))), enterpriseTest(testFn, feedTestNoTenants))
 		t.Run(fmt.Sprintf("cloudstorage-limit=%s", humanize.Bytes(uint64(sz))), cloudStorageTest(testFn, feedTestNoTenants))
 		t.Run(fmt.Sprintf("kafka-limit=%s", humanize.Bytes(uint64(sz))), kafkaTest(testFn, feedTestNoTenants))
@@ -4196,3 +4196,35 @@ func TestCheckpointFrequency(t *testing.T) {
 	require.Equal(t, completionTime, js.lastProgressUpdate)
 	require.False(t, js.progressUpdatesSkipped)
 }
+
+func TestChangefeedOrderingWithErrors(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	testFn := func(t *testing.T, db *gosql.DB, f cdctest.TestFeedFactory) {
+		sqlDB := sqlutils.MakeSQLRunner(db)
+		sqlDB.Exec(t, `CREATE TABLE foo (a INT PRIMARY KEY, b STRING)`)
+
+		foo := feed(t, f, `CREATE CHANGEFEED FOR foo WITH updated`)
+		webhookFoo := foo.(*webhookFeed)
+		// retry, then fail, then restart changefeed and successfully send messages
+		webhookFoo.mockSink.SetStatusCodes(append(repeatStatusCode(
+			http.StatusInternalServerError,
+			defaultRetryConfig().MaxRetries+1),
+			[]int{http.StatusOK, http.StatusOK, http.StatusOK}...))
+		defer closeFeed(t, foo)
+
+		sqlDB.Exec(t, `INSERT INTO foo VALUES (1, 'a')`)
+		sqlDB.Exec(t, `UPSERT INTO foo VALUES (1, 'b')`)
+		sqlDB.Exec(t, `DELETE FROM foo WHERE a = 1`)
+		assertPayloadsPerKeyOrderedStripTs(t, foo, []string{
+			`foo: [1]->{"after": {"a": 1, "b": "a"}}`,
+			`foo: [1]->{"after": {"a": 1, "b": "b"}}`,
+			`foo: [1]->{"after": null}`,
+		})
+	}
+
+	// only used for webhook sink for now since it's the only testfeed where
+	// we can control the ordering of errors
+	t.Run(`webhook`, webhookTest(testFn))
+}