Merge #64306 #64441 #64444 #64483

64306: pgcode: remove a couple of deprecated errors r=yuzefovich a=yuzefovich Release note: None 64441: ccl/sqlproxyccl: fix a broken test in bazel r=darinpp a=darinpp This fixes #61915 by adding the required data files. Release note: None 64444: opt: do not provide project ordering for columns removed when simplified r=mgartner a=mgartner Previously, `projectCanProvideOrdering` did not simplify an ordering base on an FD set if the original ordering could be provided, while `projectBuildChildReqOrdering` always attempted to simplify an ordering. In #64342 the behavior of `OrderingChoice.Simplify` changed to remove columns in groups that are not known to be equivalent by the FD. As a result, `projectCanProvideOrdering` could return true in cases where the simplified ordering, with some columns removed, could not be provided, causing `ordering.projectBuildChildReqOrdering` to panic. This commit updates `ordering.projectCanProvideOrdering` so that the ordering is always simplified, matching the behavior of `ordering.projectBuildChildReqOrdering`. Fixes #64399 Release note: None 64483: bazel: prefer `exec_tools` to `tools` in `BUILD` files r=rail a=rickystewart The [documentation](https://docs.bazel.build/versions/master/be/general.html#genrule_args) specifies that `exec_tools` is preferred to `tools` wherever possible. It doesn't matter in our case, so let's just standardize on `exec_tools` everywhere. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Darin Peshev <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
cockroachdb · Apr 30, 2021 · fc86aa6 · fc86aa6
5 parents 42e2e42 + a99064b + 1176126 + 6d36413 + f43d7b4
commit fc86aa6
Show file tree

Hide file tree

Showing 19 changed files with 69 additions and 39 deletions.
diff --git a/pkg/ccl/sqlproxyccl/BUILD.bazel b/pkg/ccl/sqlproxyccl/BUILD.bazel
@@ -39,6 +39,11 @@ go_test(
         "proxy_test.go",
         "server_test.go",
     ],
+    data = [
+        ":testserver.crt",
+        ":testserver.key",
+        ":testserver_config.cnf",
+    ],
     embed = [":sqlproxyccl"],
     tags = ["broken_in_bazel"],
     deps = [

diff --git a/pkg/cmd/roachprod/vm/aws/BUILD.bazel b/pkg/cmd/roachprod/vm/aws/BUILD.bazel
@@ -31,7 +31,7 @@ genrule(
     cmd = """
       $(location //pkg/cmd/roachprod/vm/aws/terraformgen) -o $@
     """,
-    tools = ["//pkg/cmd/roachprod/vm/aws/terraformgen"],
+    exec_tools = ["//pkg/cmd/roachprod/vm/aws/terraformgen"],
 )
 
 bindata(

diff --git a/pkg/col/coldata/BUILD.bazel b/pkg/col/coldata/BUILD.bazel
@@ -62,7 +62,7 @@ genrule(
         -fmt=false pkg/col/coldata/$@ > $@
       $(location @com_github_cockroachdb_gostdlib//x/tools/cmd/goimports) -w $@
     """,
-    tools = [
+    exec_tools = [
         "//pkg/sql/colexec/execgen/cmd/execgen",
         "@com_github_cockroachdb_gostdlib//x/tools/cmd/goimports",
     ],

diff --git a/pkg/geo/wkt/BUILD.bazel b/pkg/geo/wkt/BUILD.bazel
@@ -38,7 +38,7 @@ genrule(
     cat $(location wkt_generated.go) | sed -e 's/wktErrorVerbose = false/wktErrorVerbose = true/' > wkt_generated.go.tmp
     mv wkt_generated.go.tmp $(location wkt_generated.go)
     """,
-    tools = [
+    exec_tools = [
         "@org_golang_x_tools//cmd/goyacc",
     ],
 )
diff --git a/pkg/sql/logictest/testdata/logic_test/sqlsmith b/pkg/sql/logictest/testdata/logic_test/sqlsmith
@@ -157,3 +157,32 @@ query T
 SELECT ARRAY[1][NULL]
 ----
 NULL
+
+# Regression: #64399 (panic due to optimizer ordering bug)
+statement ok
+CREATE TABLE ab (
+  a INT,
+  b INT AS (a % 2) STORED,
+  INDEX (b)
+);
+CREATE TABLE cd (
+  c INT,
+  d INT AS (c % 2) VIRTUAL
+);
+
+statement ok
+SELECT NULL
+FROM ab AS ab1
+  JOIN cd AS cd1
+    JOIN cd AS cd2 ON cd1.c = cd2.c
+    JOIN ab AS ab2 ON
+      cd2.c = ab2.a
+      AND cd2.c = ab2.crdb_internal_mvcc_timestamp
+      AND cd1.c = ab2.a
+      AND cd1.c = ab2.b
+      AND cd1.d = ab2.b
+    ON ab1.b = cd1.d
+  JOIN cd AS cd3 ON
+    ab1.b = cd3.c
+    AND cd2.c = cd3.d
+    AND cd1.d = cd3.c
diff --git a/pkg/sql/opt/BUILD.bazel b/pkg/sql/opt/BUILD.bazel
@@ -73,7 +73,7 @@ genrule(
     cmd = """
       $(location //pkg/sql/opt/optgen/cmd/optgen) -out $@ ops $(locations :ops)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
 )
 
 # Define the generator for the enumeration of rulenames.
@@ -89,7 +89,7 @@ genrule(
       $(location //pkg/sql/opt/optgen/cmd/optgen) -out $@ rulenames \
           $(locations :ops) $(locations //pkg/sql/opt/norm:rules) $(locations //pkg/sql/opt/xform:rules)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
 )
 
 # Define the generator for the stringification of rulenames.
@@ -116,7 +116,7 @@ genrule(
       $(location @org_golang_x_tools//cmd/stringer:stringer) -output=$@ \
       -type=RuleName `dirname $(location :gen-rulenames)`/rule_name.go $(location :gen-rulenames)
     """,
-    tools = [
+    exec_tools = [
         "@go_sdk//:bin/go",
         "@org_golang_x_tools//cmd/stringer",
     ],

diff --git a/pkg/sql/opt/exec/BUILD.bazel b/pkg/sql/opt/exec/BUILD.bazel
@@ -42,5 +42,5 @@ genrule(
     cmd = """
       $(location //pkg/sql/opt/optgen/cmd/optgen) -out $@ execfactory $(locations :defs)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
 )
diff --git a/pkg/sql/opt/exec/explain/BUILD.bazel b/pkg/sql/opt/exec/explain/BUILD.bazel
@@ -76,5 +76,5 @@ genrule(
     cmd = """
       $(location //pkg/sql/opt/optgen/cmd/optgen) -out $@ execexplain $(locations //pkg/sql/opt/exec:defs)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
 )
diff --git a/pkg/sql/opt/memo/BUILD.bazel b/pkg/sql/opt/memo/BUILD.bazel
@@ -101,6 +101,6 @@ genrule(
     cmd = """
       $(location //pkg/sql/opt/optgen/cmd/optgen) -out $@ exprs $(locations //pkg/sql/opt:ops)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
     visibility = ["//visibility:public"],
 )
diff --git a/pkg/sql/opt/norm/BUILD.bazel b/pkg/sql/opt/norm/BUILD.bazel
@@ -104,5 +104,5 @@ genrule(
     cmd = """
       $(location //pkg/sql/opt/optgen/cmd/optgen) -out $@ factory $(locations //pkg/sql/opt:ops) $(locations :rules)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
 )
diff --git a/pkg/sql/opt/optgen/lang/BUILD.bazel b/pkg/sql/opt/optgen/lang/BUILD.bazel
@@ -68,7 +68,7 @@ genrule(
     cmd = """
       $(location //pkg/sql/opt/optgen/cmd/langgen) -out $@ exprs $(location lang.opt)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/langgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/langgen"],
 )
 
 genrule(
@@ -78,7 +78,7 @@ genrule(
     cmd = """
       $(location //pkg/sql/opt/optgen/cmd/langgen) -out $@ ops $(location lang.opt)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/langgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/langgen"],
 )
 
 stringer(

diff --git a/pkg/sql/opt/ordering/project.go b/pkg/sql/opt/ordering/project.go
@@ -22,18 +22,15 @@ func projectCanProvideOrdering(expr memo.RelExpr, required *physical.OrderingCho
 	proj := expr.(*memo.ProjectExpr)
 	inputCols := proj.Input.Relational().OutputCols
 
-	if required.CanProjectCols(inputCols) {
-		return true
-	}
-
-	// We may be able to "remap" columns using the internal FD set.
-	if fdSet := proj.InternalFDs(); required.CanSimplify(fdSet) {
-		simplified := required.Copy()
+	// Use a simplified ordering if it exists. This must be kept consistent with
+	// projectBuildChildReqOrdering, which always simplifies the ordering if
+	// possible.
+	simplified := *required
+	if fdSet := proj.InternalFDs(); simplified.CanSimplify(fdSet) {
+		simplified = required.Copy()
 		simplified.Simplify(fdSet)
-		return simplified.CanProjectCols(inputCols)
 	}
-
-	return false
+	return simplified.CanProjectCols(inputCols)
 }
 
 func projectBuildChildReqOrdering(

diff --git a/pkg/sql/opt/ordering/project_test.go b/pkg/sql/opt/ordering/project_test.go
@@ -39,7 +39,7 @@ func TestProject(t *testing.T) {
 
 	input := &testexpr.Instance{
 		Rel: &props.Relational{
-			OutputCols: opt.MakeColSet(1, 2, 3, 4),
+			OutputCols: opt.MakeColSet(1, 2, 3, 4, 6),
 			FuncDeps:   fds,
 		},
 	}
@@ -73,6 +73,14 @@ func TestProject(t *testing.T) {
 			req: "+5",
 			exp: "no",
 		},
+		{
+			// Regression test for #64399. projectCanProvideOrdering should not
+			// return true when the columns remaining in the ordering after
+			// simplification cannot be provided. This causes
+			// projectBuildChildReqOrdering to panic.
+			req: "+(5|6)",
+			exp: "no",
+		},
 	}
 	for _, tc := range testCases {
 		req := physical.ParseOrderingChoice(tc.req)

diff --git a/pkg/sql/opt/xform/BUILD.bazel b/pkg/sql/opt/xform/BUILD.bazel
@@ -106,5 +106,5 @@ genrule(
     cmd = """
       $(location //pkg/sql/opt/optgen/cmd/optgen) -out $@ explorer $(locations //pkg/sql/opt:ops) $(locations :rules)
     """,
-    tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
+    exec_tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
 )
diff --git a/pkg/sql/parser/BUILD.bazel b/pkg/sql/parser/BUILD.bazel
@@ -85,7 +85,7 @@ genrule(
           $(location sql.go) $(location @org_golang_x_tools//cmd/goyacc) \
           $(location @com_github_cockroachdb_gostdlib//x/tools/cmd/goimports)
     """,
-    tools = [
+    exec_tools = [
         ":sql-gen",
         "@com_github_cockroachdb_gostdlib//x/tools/cmd/goimports",
         "@org_golang_x_tools//cmd/goyacc",
@@ -110,7 +110,7 @@ genrule(
       $(location :help-gen-test) < $< >[email protected]
       mv -f [email protected] $@
     """,
-    tools = [
+    exec_tools = [
         ":help-gen-test",
     ],
 )

diff --git a/pkg/sql/pgwire/pgcode/codes.go b/pkg/sql/pgwire/pgcode/codes.go
@@ -374,17 +374,8 @@ var (
 	// RangeUnavailable signals that some data from the cluster cannot be
 	// accessed (e.g. because all replicas awol).
 	RangeUnavailable = MakeCode("58C00")
-	// DeprecatedRangeUnavailable is code that we used for RangeUnavailable until 19.2.
-	// 20.1 needs to recognize it coming from 19.2 nodes.
-	// TODO(andrei): remove in 20.2.
-	DeprecatedRangeUnavailable = MakeCode("XXC00")
 
 	// InternalConnectionFailure refers to a networking error encountered
 	// internally on a connection between different Cockroach nodes.
 	InternalConnectionFailure = MakeCode("58C01")
-	// DeprecatedInternalConnectionFailure is code that we used for
-	// InternalConnectionFailure until 19.2.
-	// 20.1 needs to recognize it coming from 19.2 nodes.
-	// TODO(andrei): remove in 20.2.
-	DeprecatedInternalConnectionFailure = ConnectionFailure
 )
diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go
@@ -181,10 +181,10 @@ func isPermanentSchemaChangeError(err error) bool {
 	}
 
 	switch pgerror.GetPGCode(err) {
-	case pgcode.SerializationFailure, pgcode.InternalConnectionFailure, pgcode.DeprecatedInternalConnectionFailure:
+	case pgcode.SerializationFailure, pgcode.InternalConnectionFailure:
 		return false
 
-	case pgcode.Internal, pgcode.RangeUnavailable, pgcode.DeprecatedRangeUnavailable:
+	case pgcode.Internal, pgcode.RangeUnavailable:
 		if strings.Contains(err.Error(), context.DeadlineExceeded.Error()) {
 			return false
 		}

diff --git a/pkg/sql/schemachange/BUILD.bazel b/pkg/sql/schemachange/BUILD.bazel
@@ -61,7 +61,7 @@ genrule(
        env PATH=$$GO_ABS_PATH HOME=$(GENDIR) \
        $(location @org_golang_x_tools//cmd/stringer:stringer) -output=$@ -type=ColumnConversionKind -trimprefix ColumnConversion $<
     """,
-    tools = [
+    exec_tools = [
         "@go_sdk//:bin/go",
         "@org_golang_x_tools//cmd/stringer",
     ],

diff --git a/pkg/util/encoding/BUILD.bazel b/pkg/util/encoding/BUILD.bazel
@@ -87,7 +87,7 @@ genrule(
        env PATH=$$GO_ABS_PATH HOME=$(GENDIR) \
          $(location @org_golang_x_tools//cmd/stringer:stringer) -output=$@ -type=Type encoding_tmp.go
     """,
-    tools = [
+    exec_tools = [
         "@go_sdk//:bin/go",
         "@org_golang_x_tools//cmd/stringer",
     ],