Merge #55808 #56334 #56345 #56525 #56589 #56597 #56598 #56602

55808: kvserver: skip non-live nodes when considering candidates for transfers r=tbg a=knz

(This PR is forked off #55460 to simplify the discussion. I believe there's no discussion left here? Maybe I can merge it directly?)

Fixes #55440.

Prior to this patch, 3 components could attempt to transfer a replica
to a node currently being drained:

- the store rebalancer, which rebalances replicas based on disk
  usage and QPS.
- the allocator, to place new replicas.
- the allocator, to rebalance replicas depending on load.

This commit introduces a consideration for node liveness when building
the list of candidates, to detect whether a target node is
acceptable. Any node that is not LIVE according to its liveness status
is not considered for a transfer.

Release note (bug fix): In some cases CockroachDB would attempt to
transfer ranges to nodes in the process of being decommissioned or
being shut down; this could cause disruption the moment the node
did actually terminate. This bug has been fixed. It had been
introduced some time before v2.0.

56334: kvserver: use messages on NotLeaseholderErrors everywhere r=andreimatei a=andreimatei

NLHE permits custom messages in it, but the field was rarely used. This
patch makes every instance where we instantiate the error provide a
message, since this error comes from a wide variety of conditions.

Release note: None

56345: opt: increase cost for table descriptor fetch during virtual scan r=rytaft a=rytaft

This commit bumps the cost of each virtual scan to `25*randIOCostFactor`
from its previous value of `10*randIOCostFactor`. This new value threads
the needle so that a lookup join will still be chosen if the predicate
is very selective, but the plan for the PGJDBC query identified in #55140
no longer includes lookup joins.

Fixes #55140

Release note (performance improvement): Adjusted the cost model in
the optimizer so that the optimizer is less likely to plan a lookup
join into a virtual table. Performing a lookup join into a virtual
table is expensive, so this change will generally result in better
performance for queries involving joins with virtual tables.

56525: bazel: Move third party repositories to c-deps/REPOSITORIES.bzl r=otan a=alan-mas

bazel: Move third party repositories to c-deps/REPOSITORIES.bzl

This is one of the Bazel re-factoring that we are working on
and it is about to move third party repositories out of root WORKSPACE. 
fixes #56053

Best practices is to separate external dependencies and it also
hides the repo WORKSPACE from being used by other directories.

We are creating a new .bzl file inside c-deps with all the external dependencies
and then load then inside our root WORKSPACE

Release note: None

56589: sql: resolve error due to drop table after schema change in same txn r=ajwerner a=jayshrivastava

Previously, if a drop table statement was executed in a transaction
following other schema changes to the table in the same transaction,
an error would occur. This error was due to the drop table statement
marking previous jobs as succeeded and then proceeding to modify them.
This change ensures that drop table statement will delete all existing
jobs from the job cache so that it does not interfere with previous jobs.

Release note (sql change): A table can successfully be dropped in
a transaction following other schema changes to the table in the
same transaction.

This resolves one of the issues in #56235

56597: colflow: fix recent misuse of two slices in the flow setup r=yuzefovich a=yuzefovich

We've recently added the reusing of metadataSourcesQueue and toClose
slices in order to reduce some allocations. However, the components that
are using those slices don't make a deep copy, and as a result, we
introduced a bug in which we were breaking the current contract. This
commit fixes the issue by going back to the old method (with slight
difference in that we currently delay any allocations unlike previously
when we allocated a slice with capacity of 1).

Release note: None (no release with this bug)

56598: tree: introduce concept of "default" collation r=rafiss a=otan

Resolves #54989


Release note (sql change): Introduced a pg_collation of "default".
Strings now return the "default" collation OID in the pg_attribute
table (this was previously en_US). The "default" collation is also
visible on the pg_collation virtual table.



56602: roachpb: remove various `(gogoproto.equal)` options r=nvanbenschoten a=tbg

The first commit explains why some cleanup was necessary,
the others are the result of spending a little extra time
cleaning up "unnecessarily".

There are plenty of Equals left to clean up, but the returns
were diminishing. The important part is that when additions
to the KV API are made, nobody will be forced to add the
`equal` option any more.

- roachpb: don't generate Equal() on Error
- roachpb: remove more `Equal` methods
- roachpb: remove (gogoproto.equal) from api.proto
- roachpb: mostly remove (gogoproto.equal) from data.proto
- roachpb: remove Value.Equal
- kvserverpb: remove Equal from ReplicatedEvalResult


Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
Co-authored-by: Alanmas <acostas.alan@gmail.com>
Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>

Author: 9 people

WORKSPACE

@@ -60,6 +60,12 @@ protobuf_deps()
 load("//:DEPS.bzl", "go_deps")
 go_deps()
 
+
+# Loading c-deps third party dependencies.
+load("//c-deps:REPOSITORIES.bzl", "c_deps")
+c_deps()
+
+
 # Load the bazel utility that lets us build C/C++ projects using cmake/make/etc.
 #
 # TODO(irfansharif): Point to an upstream SHA once it picks up Oliver's changes
@@ -71,45 +77,3 @@ git_repository(
 )
 load("@rules_foreign_cc//:workspace_definitions.bzl", "rules_foreign_cc_dependencies")
 rules_foreign_cc_dependencies()
-
-# Define the repositories for each of our c-dependencies. BUILD_ALL_CONTENT is
-# a shorthand to glob over all checked-in files.
-BUILD_ALL_CONTENT = """filegroup(name = "all", srcs = glob(["**"]), visibility = ["//visibility:public"])"""
-
-# Each of these c-dependencies map to one or more library definitions in the
-# top-level BUILD.bazel. Library definitions will list the following
-# definitions as sources. For certain libraries (like `libroachccl`), the
-# repository definitions are also listed as tool dependencies. This is because
-# building those libraries require certain checked out repositories being
-# placed relative to the source tree of the library itself.
-
-new_local_repository(
-    name = "libroach",
-    path = "c-deps/libroach",
-    build_file_content = BUILD_ALL_CONTENT,
-)
-
-new_local_repository(
-  name = "proj",
-  path = "c-deps/proj",
-  build_file_content = BUILD_ALL_CONTENT,
-)
-
-# For c-deps/protobuf, we elide a checked in generated file. Already generated
-# files are read-only in the bazel sandbox, so bazel is unable to regenerate
-# the same files, which the build process requires it to do so.
-BUILD_PROTOBUF_CONTENT = """filegroup(name = "all", srcs = glob(["**"], exclude=["src/google/protobuf/compiler/js/well_known_types_embed.cc"]), visibility = ["//visibility:public"])"""
-new_local_repository(
-   name = "protobuf",
-   path = "c-deps/protobuf",
-   build_file_content = BUILD_PROTOBUF_CONTENT,
-)
-
-# This is essentially the same above, we elide a generated file to avoid
-# permission issues when building jemalloc within the bazel sandbox.
-BUILD_JEMALLOC_CONTENT = """filegroup(name = "all", srcs = glob(["**"], exclude=["configure"]), visibility = ["//visibility:public"])"""
-new_local_repository(
-  name = "jemalloc",
-  path = "c-deps/jemalloc",
-  build_file_content = BUILD_JEMALLOC_CONTENT,
-)

c-deps/REPOSITORIES.bzl

@@ -0,0 +1,44 @@
+# c-deps repository definitions.
+
+# Define the repositories for each of our c-dependencies. BUILD_ALL_CONTENT is
+# a shorthand to glob over all checked-in files.
+BUILD_ALL_CONTENT = """filegroup(name = "all", srcs = glob(["**"]), visibility = ["//visibility:public"])"""
+
+# Each of these c-dependencies map to one or more library definitions in the
+# top-level BUILD.bazel. Library definitions will list the following
+# definitions as sources. For certain libraries (like `libroachccl`), the
+# repository definitions are also listed as tool dependencies. This is because
+# building those libraries require certain checked out repositories being
+# placed relative to the source tree of the library itself.
+
+# For c-deps/protobuf, we elide a checked in generated file. Already generated
+# files are read-only in the bazel sandbox, so bazel is unable to regenerate
+# the same files, which the build process requires it to do so.
+BUILD_PROTOBUF_CONTENT = """filegroup(name = "all", srcs = glob(["**"], exclude=["src/google/protobuf/compiler/js/well_known_types_embed.cc"]), visibility = ["//visibility:public"])"""
+
+# This is essentially the same above, we elide a generated file to avoid
+# permission issues when building jemalloc within the bazel sandbox.
+BUILD_JEMALLOC_CONTENT = """filegroup(name = "all", srcs = glob(["**"], exclude=["configure"]), visibility = ["//visibility:public"])"""
+
+# We do need to add native as new_local_repository is defined in Bazel core.
+def c_deps():
+    native.new_local_repository(
+        name = "libroach",
+        path = "c-deps/libroach",
+        build_file_content = BUILD_ALL_CONTENT,
+    )
+    native.new_local_repository(
+        name = "proj",
+        path = "c-deps/proj",
+        build_file_content = BUILD_ALL_CONTENT,
+    )
+    native.new_local_repository(
+        name = "protobuf",
+        path = "c-deps/protobuf",
+        build_file_content = BUILD_PROTOBUF_CONTENT,
+    )
+    native.new_local_repository(
+        name = "jemalloc",
+        path = "c-deps/jemalloc",
+        build_file_content = BUILD_JEMALLOC_CONTENT,
+    )

pkg/config/system_test.go

@@ -13,6 +13,7 @@ package config_test
 import (
 	"context"
 	"math"
+	"reflect"
 	"sort"
 	"testing"
 
@@ -139,7 +140,7 @@ func TestGet(t *testing.T) {
 	cfg := config.NewSystemConfig(zonepb.DefaultZoneConfigRef())
 	for tcNum, tc := range testCases {
 		cfg.Values = tc.values
-		if val := cfg.GetValue([]byte(tc.key)); !val.Equal(tc.value) {
+		if val := cfg.GetValue([]byte(tc.key)); !reflect.DeepEqual(val, tc.value) {
 			t.Errorf("#%d: expected=%s, found=%s", tcNum, tc.value, val)
 		}
 	}