Merge #88944 #89184 #89189

88944: sql/covering: make OverlapCoveringMerge benchmark deterministic r=cucaroach a=cucaroach Previously we seeded RNG with time and benchmark results varied widely. In order to make sure this doesn't make noise when comparing different versions make the seed a constant. Fixes: #88919 Release note: None 89184: sql: delete function name key from schema if no overload left after drop r=chengxiong-ruan a=chengxiong-ruan Backport fixes: #89046 Previously we just remove an overload from the slice when dropping a function. This is problematic if there's zero overloads left after the drop because it pretends that there is some function with the name but actually nothing. So we need to delete the key if there is not overload for the name. Release note: None Release justification: GA blocker bug fix. 89189: opgen: ensure GC jobs for temp indexes have proper description r=postamar a=postamar Fixes #82169. Release note: None Co-authored-by: Tommy Reilly <[email protected]> Co-authored-by: Chengxiong Ruan <[email protected]> Co-authored-by: Marius Posta <[email protected]>
cockroachdb · Oct 3, 2022 · 9794be4 · 9794be4
4 parents b5e54ff + dc2daea + 5ae9e2c + e1cfe36
commit 9794be4
Show file tree

Hide file tree

Showing 162 changed files with 1,056 additions and 248 deletions.
diff --git a/pkg/ccl/schemachangerccl/testdata/end_to_end/create_index b/pkg/ccl/schemachangerccl/testdata/end_to_end/create_index
@@ -385,7 +385,7 @@ upsert descriptor #104
   -  version: "7"
   +  version: "8"
 write *eventpb.FinishSchemaChange to event log
-create job #2 (non-cancelable: true): "GC for "
+create job #2 (non-cancelable: true): "GC for removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id, name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))"
   descriptor IDs: [104]
 update progress of schema change job #1: "all stages completed"
 set schema change job #1 to non-cancellable

diff --git a/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index b/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index
@@ -421,6 +421,9 @@ EXPLAIN (ddl, verbose) CREATE INDEX id1
             │
             ├── • CreateGCJobForIndex
             │     IndexID: 3
+            │     StatementForDropJob:
+            │       Statement: removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent

diff --git a/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_1_of_7 b/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_1_of_7
@@ -121,8 +121,8 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 1 of 7;
             │     IndexID: 2
             │     StatementForDropJob:
             │       Rollback: true
-            │       Statement: CREATE INDEX id1 ON defaultdb.public.t1 (id, name) STORING (money) PARTITION
-            │         BY LIST (id) (PARTITION p1 VALUES IN (1))
+            │       Statement: removed secondary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent
@@ -131,6 +131,10 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 1 of 7;
             │
             ├── • CreateGCJobForIndex
             │     IndexID: 3
+            │     StatementForDropJob:
+            │       Rollback: true
+            │       Statement: removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent

diff --git a/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_2_of_7 b/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_2_of_7
@@ -113,8 +113,8 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 2 of 7;
     │       │     IndexID: 2
     │       │     StatementForDropJob:
     │       │       Rollback: true
-    │       │       Statement: CREATE INDEX id1 ON defaultdb.public.t1 (id, name) STORING (money) PARTITION
-    │       │         BY LIST (id) (PARTITION p1 VALUES IN (1))
+    │       │       Statement: removed secondary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+    │       │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
     │       │     TableID: 104
     │       │
     │       ├── • MakeIndexAbsent
@@ -155,6 +155,10 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 2 of 7;
             │
             ├── • CreateGCJobForIndex
             │     IndexID: 3
+            │     StatementForDropJob:
+            │       Rollback: true
+            │       Statement: removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent

diff --git a/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_3_of_7 b/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_3_of_7
@@ -113,8 +113,8 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 3 of 7;
     │       │     IndexID: 2
     │       │     StatementForDropJob:
     │       │       Rollback: true
-    │       │       Statement: CREATE INDEX id1 ON defaultdb.public.t1 (id, name) STORING (money) PARTITION
-    │       │         BY LIST (id) (PARTITION p1 VALUES IN (1))
+    │       │       Statement: removed secondary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+    │       │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
     │       │     TableID: 104
     │       │
     │       ├── • MakeIndexAbsent
@@ -155,6 +155,10 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 3 of 7;
             │
             ├── • CreateGCJobForIndex
             │     IndexID: 3
+            │     StatementForDropJob:
+            │       Rollback: true
+            │       Statement: removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent

diff --git a/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_4_of_7 b/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_4_of_7
@@ -113,8 +113,8 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 4 of 7;
     │       │     IndexID: 2
     │       │     StatementForDropJob:
     │       │       Rollback: true
-    │       │       Statement: CREATE INDEX id1 ON defaultdb.public.t1 (id, name) STORING (money) PARTITION
-    │       │         BY LIST (id) (PARTITION p1 VALUES IN (1))
+    │       │       Statement: removed secondary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+    │       │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
     │       │     TableID: 104
     │       │
     │       ├── • MakeIndexAbsent
@@ -155,6 +155,10 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 4 of 7;
             │
             ├── • CreateGCJobForIndex
             │     IndexID: 3
+            │     StatementForDropJob:
+            │       Rollback: true
+            │       Statement: removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent

diff --git a/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_5_of_7 b/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_5_of_7
@@ -155,8 +155,8 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 5 of 7;
             │     IndexID: 2
             │     StatementForDropJob:
             │       Rollback: true
-            │       Statement: CREATE INDEX id1 ON defaultdb.public.t1 (id, name) STORING (money) PARTITION
-            │         BY LIST (id) (PARTITION p1 VALUES IN (1))
+            │       Statement: removed secondary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent
@@ -165,6 +165,10 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 5 of 7;
             │
             ├── • CreateGCJobForIndex
             │     IndexID: 3
+            │     StatementForDropJob:
+            │       Rollback: true
+            │       Statement: removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent

diff --git a/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_6_of_7 b/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_6_of_7
@@ -155,8 +155,8 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 6 of 7;
             │     IndexID: 2
             │     StatementForDropJob:
             │       Rollback: true
-            │       Statement: CREATE INDEX id1 ON defaultdb.public.t1 (id, name) STORING (money) PARTITION
-            │         BY LIST (id) (PARTITION p1 VALUES IN (1))
+            │       Statement: removed secondary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent
@@ -165,6 +165,10 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 6 of 7;
             │
             ├── • CreateGCJobForIndex
             │     IndexID: 3
+            │     StatementForDropJob:
+            │       Rollback: true
+            │       Statement: removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent

diff --git a/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_7_of_7 b/pkg/ccl/schemachangerccl/testdata/explain_verbose/create_index.rollback_7_of_7
@@ -155,8 +155,8 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 7 of 7;
             │     IndexID: 2
             │     StatementForDropJob:
             │       Rollback: true
-            │       Statement: CREATE INDEX id1 ON defaultdb.public.t1 (id, name) STORING (money) PARTITION
-            │         BY LIST (id) (PARTITION p1 VALUES IN (1))
+            │       Statement: removed secondary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent
@@ -165,6 +165,10 @@ EXPLAIN (ddl, verbose) rollback at post-commit stage 7 of 7;
             │
             ├── • CreateGCJobForIndex
             │     IndexID: 3
+            │     StatementForDropJob:
+            │       Rollback: true
+            │       Statement: removed temporary index; CREATE INDEX id1 ON defaultdb.public.t1 (id,
+            │         name) STORING (money) PARTITION BY LIST (id) (PARTITION p1 VALUES IN (1))
             │     TableID: 104
             │
             ├── • MakeIndexAbsent

diff --git a/pkg/sql/catalog/schemadesc/schema_desc.go b/pkg/sql/catalog/schemadesc/schema_desc.go
@@ -425,6 +425,10 @@ func (desc *Mutable) RemoveFunction(name string, id descpb.ID) {
 				updated = append(updated, ol)
 			}
 		}
+		if len(updated) == 0 {
+			delete(desc.Functions, name)
+			return
+		}
 		desc.Functions[name] = descpb.SchemaDescriptor_Function{
 			Name:      name,
 			Overloads: updated,

diff --git a/pkg/sql/covering/BUILD.bazel b/pkg/sql/covering/BUILD.bazel
@@ -20,7 +20,6 @@ go_test(
     embed = [":covering"],
     deps = [
         "//pkg/util/leaktest",
-        "//pkg/util/timeutil",
         "@com_github_stretchr_testify//require",
     ],
 )

diff --git a/pkg/sql/covering/bench_test.go b/pkg/sql/covering/bench_test.go
@@ -16,7 +16,6 @@ import (
 	"math/rand"
 	"testing"
 
-	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/stretchr/testify/require"
 )
 
@@ -25,7 +24,7 @@ func BenchmarkOverlapCoveringMerge(b *testing.B) {
 		name   string
 		inputs []Covering
 	}
-	rand.Seed(timeutil.Now().Unix())
+	rand.Seed(0)
 
 	for _, numLayers := range []int{
 		1,      // single backup

diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial b/pkg/sql/logictest/testdata/logic_test/geospatial
@@ -5388,7 +5388,7 @@ SELECT public.ST_AsText('POINT(10.5 20.25)'::geometry)
 ----
 POINT (10.5 20.25)
 
-statement error unknown function: public.log\(\), but log\(\) exists
+statement error pq: unknown function: public.log\(\): function undefined
 SELECT public.log(10)
 
 query TT

diff --git a/pkg/sql/logictest/testdata/logic_test/udf b/pkg/sql/logictest/testdata/logic_test/udf
@@ -454,7 +454,7 @@ $$
 statement ok
 DROP FUNCTION f_test_drop(INT), f_test_drop(INT);
 
-statement error pq: function public.f_test_drop does not exist
+statement error pq: unknown function: public.f_test_drop\(\): function undefined
 SELECT @2 FROM [SHOW CREATE FUNCTION public.f_test_drop];
 
 query T
@@ -473,7 +473,7 @@ $$
 statement ok
 DROP FUNCTION f_test_drop(INT);
 
-statement error pq: function sc1.f_test_drop does not exist
+statement error pq: unknown function: sc1.f_test_drop\(\): function undefined
 SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f_test_drop];
 
 # If there are identical function signatures in different schemas, multiple drop
@@ -514,10 +514,10 @@ DROP FUNCTION f_test_drop();
 DROP FUNCTION f_test_drop();
 COMMIT;
 
-statement error pq: function public.f_test_drop does not exist
+statement error pq: unknown function: public.f_test_drop\(\): function undefined
 SELECT @2 FROM [SHOW CREATE FUNCTION public.f_test_drop];
 
-statement error pq: function sc1.f_test_drop does not exist
+statement error pq: unknown function: sc1.f_test_drop\(\): function undefined
 SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f_test_drop];
 
 statement ok
@@ -1044,7 +1044,7 @@ ALTER FUNCTION f_test_alter_name RENAME TO f_test_alter_name_same_in
 statement ok
 ALTER FUNCTION f_test_alter_name RENAME TO f_test_alter_name_new
 
-statement error pq: function f_test_alter_name does not exist
+statement error pq: unknown function: f_test_alter_name\(\): function undefined
 SELECT @2 FROM [SHOW CREATE FUNCTION f_test_alter_name];
 
 query T
@@ -1063,7 +1063,7 @@ $$
 statement ok
 ALTER FUNCTION f_test_alter_name_new RENAME to f_test_alter_name_diff_in
 
-statement error pq: function f_test_alter_name_new does not exist
+statement error pq: unknown function: f_test_alter_name_new\(\): function undefined
 SELECT @2 FROM [SHOW CREATE FUNCTION f_test_alter_name_new];
 
 query T
@@ -2501,3 +2501,19 @@ subtest variadic
 # Variadic UDFS are not currently supported.
 statement error pgcode 0A000 unimplemented: this syntax\nHINT.*\n.*88947
 CREATE FUNCTION rec(VARIADIC arr INT[]) RETURNS INT LANGUAGE SQL AS '1'
+
+subtest execute_dropped_function
+
+statement ok
+CREATE FUNCTION f_test_exec_dropped(a int) RETURNS INT LANGUAGE SQL AS $$ SELECT a $$;
+
+query I
+SELECT f_test_exec_dropped(123);
+----
+123
+
+statement ok
+DROP FUNCTION f_test_exec_dropped;
+
+statement error pq: unknown function: f_test_exec_dropped\(\): function undefined
+SELECT f_test_exec_dropped(321);
diff --git a/pkg/sql/schema_resolver.go b/pkg/sql/schema_resolver.go
@@ -425,7 +425,12 @@ func (sr *schemaResolver) ResolveFunction(
 		// name which is not lowercase. So here we try to lowercase the given
 		// function name and find a suggested function name if possible.
 		extraMsg := ""
-		lowerName := tree.MakeUnresolvedName(strings.ToLower(name.Parts[0]))
+		var lowerName tree.UnresolvedName
+		if fn.ExplicitSchema {
+			lowerName = tree.MakeUnresolvedName(strings.ToLower(name.Parts[0]), strings.ToLower(name.Parts[1]))
+		} else {
+			lowerName = tree.MakeUnresolvedName(strings.ToLower(name.Parts[0]))
+		}
 		if lowerName != *name {
 			alternative, err := sr.ResolveFunction(ctx, &lowerName, path)
 			if err == nil && alternative != nil {

diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go b/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go
@@ -54,12 +54,19 @@ func newLogEventOp(e scpb.Element, md *targetsWithElementMap) *scop.LogEvent {
 
 func statementForDropJob(e scpb.Element, md *targetsWithElementMap) scop.StatementForDropJob {
 	stmtID := md.Targets[md.elementToTarget[e]].Metadata.StatementID
+	stmt := redact.RedactableString(md.Statements[stmtID].RedactedStatement).StripMarkers()
+	switch e.(type) {
+	case *scpb.PrimaryIndex:
+		stmt = "removed primary index; " + stmt
+	case *scpb.SecondaryIndex:
+		stmt = "removed secondary index; " + stmt
+	case *scpb.TemporaryIndex:
+		stmt = "removed temporary index; " + stmt
+	}
 	return scop.StatementForDropJob{
 		// Using the redactable string but with stripped markers gives us a
 		// normalized and fully-qualified string value for display use.
-		Statement: redact.RedactableString(
-			md.Statements[stmtID].RedactedStatement,
-		).StripMarkers(),
+		Statement:   stmt,
 		StatementID: stmtID,
 		Rollback:    md.InRollback,
 	}

diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go
@@ -49,10 +49,11 @@ func init() {
 				}),
 			),
 			to(scpb.Status_ABSENT,
-				emit(func(this *scpb.TemporaryIndex) *scop.CreateGCJobForIndex {
+				emit(func(this *scpb.TemporaryIndex, md *targetsWithElementMap) *scop.CreateGCJobForIndex {
 					return &scop.CreateGCJobForIndex{
-						TableID: this.TableID,
-						IndexID: this.IndexID,
+						TableID:             this.TableID,
+						IndexID:             this.IndexID,
+						StatementForDropJob: statementForDropJob(this, md),
 					}
 				}),
 				emit(func(this *scpb.TemporaryIndex) *scop.MakeIndexAbsent {