Merge #24758

24758: cherry-pick 2.0: sql: fix the handling of current_schema and search_path r=knz a=knz

Picks #24718.

cc @cockroachdb/release 

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>

docs/generated/sql/functions.md

@@ -807,9 +807,9 @@ Compatible elements: hour, minute, second, millisecond, microsecond.</p>
 </span></td></tr>
 <tr><td><code>current_database() &rarr; <a href="string.html">string</a></code></td><td><span class="funcdesc"><p>Returns the current database.</p>
 </span></td></tr>
-<tr><td><code>current_schema() &rarr; <a href="string.html">string</a></code></td><td><span class="funcdesc"><p>Returns the current schema. This function is provided for compatibility with PostgreSQL. For a new CockroachDB application, consider using current_database() instead.</p>
+<tr><td><code>current_schema() &rarr; <a href="string.html">string</a></code></td><td><span class="funcdesc"><p>Returns the current schema.</p>
 </span></td></tr>
-<tr><td><code>current_schemas(include_pg_catalog: <a href="bool.html">bool</a>) &rarr; <a href="string.html">string</a>[]</code></td><td><span class="funcdesc"><p>Returns the current search path for unqualified names.</p>
+<tr><td><code>current_schemas(include_pg_catalog: <a href="bool.html">bool</a>) &rarr; <a href="string.html">string</a>[]</code></td><td><span class="funcdesc"><p>Returns the valid schemas in the search path.</p>
 </span></td></tr>
 <tr><td><code>current_user() &rarr; <a href="string.html">string</a></code></td><td><span class="funcdesc"><p>Returns the current user. This function is provided for compatibility with PostgreSQL.</p>
 </span></td></tr>

pkg/sql/distsqlrun/server.go

@@ -529,6 +529,13 @@ func (ep *dummyEvalPlanner) ParseQualifiedTableName(
 	return nil, errEvalPlanner
 }
 
+// Implements the tree.EvalDatabase interface.
+func (ep *dummyEvalPlanner) LookupSchema(
+	ctx context.Context, dbName, scName string,
+) (bool, tree.SchemaMeta, error) {
+	return false, nil, errEvalPlanner
+}
+
 // Implements the tree.EvalDatabase interface.
 func (ep *dummyEvalPlanner) ResolveTableName(ctx context.Context, tn *tree.TableName) error {
 	return errEvalPlanner
@@ -559,7 +566,14 @@ func (so *dummySequenceOperators) ParseQualifiedTableName(
 
 // Implements the tree.EvalDatabase interface.
 func (so *dummySequenceOperators) ResolveTableName(ctx context.Context, tn *tree.TableName) error {
-	return errEvalPlanner
+	return errSequenceOperators
+}
+
+// Implements the tree.EvalDatabase interface.
+func (so *dummySequenceOperators) LookupSchema(
+	ctx context.Context, dbName, scName string,
+) (bool, tree.SchemaMeta, error) {
+	return false, nil, errSequenceOperators
 }
 
 // Implements the tree.SequenceOperators interface.

pkg/sql/logictest/testdata/logic_test/builtin_function

@@ -1176,12 +1176,12 @@ SET SEARCH_PATH=test,pg_catalog
 query T
 SELECT CURRENT_SCHEMAS(true)
 ----
-{"test","pg_catalog"}
+{"pg_catalog"}
 
 query T
 SELECT CURRENT_SCHEMAS(false)
 ----
-{"test","pg_catalog"}
+{"pg_catalog"}
 
 statement ok
 RESET SEARCH_PATH

pkg/sql/logictest/testdata/logic_test/select_search_path

@@ -121,3 +121,39 @@ query I
 SELECT COUNT(DISTINCT(1)) FROM pg_tables
 ----
 1
+
+# Test that current_schema reports the first valid entry in search_path, or
+# NULL if there is no such entry.
+
+statement ok
+SET search_path = nonexistent, public
+
+query T
+SELECT current_schema
+----
+public
+
+statement ok
+SET search_path = nonexistent
+
+query T
+SELECT current_schema
+----
+NULL
+
+# Test that current_schemas only reports the valid entries in
+# search_path.
+
+statement ok
+SET search_path = nonexistent, public
+
+query T
+SELECT current_schemas(false)
+----
+{"public"}
+
+# Test that object creation targets the first valid entry in
+# search_path, not just the first entry.
+
+statement ok
+CREATE TABLE sometable(x INT); SELECT * FROM public.sometable

pkg/sql/sem/builtins/builtins.go

@@ -2196,6 +2196,7 @@ CockroachDB supports the following flags:
 
 	// Metadata functions.
 
+	// https://www.postgresql.org/docs/10/static/functions-info.html
 	"version": {
 		tree.Builtin{
 			Types:      tree.ArgTypes{},
@@ -2208,6 +2209,7 @@ CockroachDB supports the following flags:
 		},
 	},
 
+	// https://www.postgresql.org/docs/10/static/functions-info.html
 	"current_database": {
 		tree.Builtin{
 			Types:      tree.ArgTypes{},
@@ -2223,52 +2225,77 @@ CockroachDB supports the following flags:
 		},
 	},
 
+	// https://www.postgresql.org/docs/10/static/functions-info.html
+	//
+	// Note that in addition to what the pg doc says ("current_schema =
+	// first item in search path"), the pg server actually skips over
+	// non-existent schemas in the search path to determine
+	// current_schema. This is not documented but can be verified by a
+	// SQL client against a pg server.
 	"current_schema": {
 		tree.Builtin{
 			Types:      tree.ArgTypes{},
 			ReturnType: tree.FixedReturnType(types.String),
 			Category:   categorySystemInfo,
-			Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
-				hasFirst, first := ctx.SessionData.SearchPath.FirstSpecified()
-				if !hasFirst {
-					return tree.DNull, nil
+			Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
+				ctx := evalCtx.Ctx()
+				curDb := evalCtx.SessionData.Database
+				iter := evalCtx.SessionData.SearchPath.IterWithoutImplicitPGCatalog()
+				for scName, ok := iter(); ok; scName, ok = iter() {
+					if found, _, err := evalCtx.Planner.LookupSchema(ctx, curDb, scName); found || err != nil {
+						if err != nil {
+							return nil, err
+						}
+						return tree.NewDString(scName), nil
+					}
 				}
-				return tree.NewDString(first), nil
+				return tree.DNull, nil
 			},
-			Info: "Returns the current schema. This function is provided for " +
-				"compatibility with PostgreSQL. For a new CockroachDB application, " +
-				"consider using current_database() instead.",
+			Info: "Returns the current schema.",
 		},
 	},
 
-	// For now, schemas are the same as databases. So, current_schemas
-	// returns the current database (if one has been set by the user)
-	// and the session's database search path.
+	// https://www.postgresql.org/docs/10/static/functions-info.html
+	//
+	// Note that in addition to what the pg doc says ("current_schemas =
+	// items in search path with or without pg_catalog depending on
+	// argument"), the pg server actually skips over non-existent
+	// schemas in the search path to compute current_schemas. This is
+	// not documented but can be verified by a SQL client against a pg
+	// server.
 	"current_schemas": {
 		tree.Builtin{
 			Types:      tree.ArgTypes{{"include_pg_catalog", types.Bool}},
 			ReturnType: tree.FixedReturnType(types.TArray{Typ: types.String}),
 			Category:   categorySystemInfo,
-			Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
+			Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
+				ctx := evalCtx.Ctx()
+				curDb := evalCtx.SessionData.Database
 				includePgCatalog := *(args[0].(*tree.DBool))
 				schemas := tree.NewDArray(types.String)
 				var iter func() (string, bool)
 				if includePgCatalog {
-					iter = ctx.SessionData.SearchPath.Iter()
+					iter = evalCtx.SessionData.SearchPath.Iter()
 				} else {
-					iter = ctx.SessionData.SearchPath.IterWithoutImplicitPGCatalog()
+					iter = evalCtx.SessionData.SearchPath.IterWithoutImplicitPGCatalog()
 				}
-				for p, ok := iter(); ok; p, ok = iter() {
-					if err := schemas.Append(tree.NewDString(p)); err != nil {
-						return nil, err
+				for scName, ok := iter(); ok; scName, ok = iter() {
+					if found, _, err := evalCtx.Planner.LookupSchema(ctx, curDb, scName); found || err != nil {
+						if err != nil {
+							return nil, err
+						}
+						if err := schemas.Append(tree.NewDString(scName)); err != nil {
+							return nil, err
+						}
 					}
 				}
 				return schemas, nil
 			},
-			Info: "Returns the current search path for unqualified names.",
+			Info: "Returns the valid schemas in the search path.",
 		},
 	},
 
+	// https://www.postgresql.org/docs/10/static/functions-info.html
 	"current_user": {
 		tree.Builtin{
 			Types:      tree.ArgTypes{},

pkg/sql/sem/tree/eval.go

@@ -2200,6 +2200,10 @@ type EvalDatabase interface {
 	// sets it on the returned TableName.
 	// It returns an error if the table doesn't exist.
 	ResolveTableName(ctx context.Context, tn *TableName) error
+
+	// LookupSchema looks up the schema with the given name in the given
+	// database.
+	LookupSchema(ctx context.Context, dbName, scName string) (found bool, scMeta SchemaMeta, err error)
 }
 
 // EvalPlanner is a limited planner that can be used from EvalContext.

pkg/sql/sem/tree/name_resolution.go

@@ -257,7 +257,6 @@ func (t *TableName) ResolveExisting(
 ) (bool, NameResolutionResult, error) {
 	if t.ExplicitSchema {
 		if t.ExplicitCatalog {
-			// log.VEventf(ctx, 2, "full name already resolved: %q", t)
 			// Already 3 parts: nothing to search. Delegate to the resolver.
 			return r.LookupObject(ctx, t.Catalog(), t.Schema(), t.Table())
 		}
@@ -273,7 +272,6 @@ func (t *TableName) ResolveExisting(
 			if err == nil {
 				t.CatalogName = Name(curDb)
 			}
-			// log.VEventf(ctx, 2, "two part lookup: %+v // %v", t, err)
 			return found, objMeta, err
 		}
 		// No luck so far. Compatibility with CockroachDB v1.1: try D.public.T instead.
@@ -283,7 +281,6 @@ func (t *TableName) ResolveExisting(
 				t.SchemaName = PublicSchemaName
 				t.ExplicitCatalog = true
 			}
-			// log.VEventf(ctx, 2, "two part lookup compat: %+v // %v", t, err)
 			return found, objMeta, err
 		}
 		// Welp, really haven't found anything.
@@ -298,7 +295,6 @@ func (t *TableName) ResolveExisting(
 				t.CatalogName = Name(curDb)
 				t.SchemaName = Name(next)
 			}
-			// log.VEventf(ctx, 2, "one part lookup: %+v // %v", t, err)
 			return found, objMeta, err
 		}
 	}
@@ -312,7 +308,6 @@ func (t *TableName) ResolveTarget(
 ) (found bool, scMeta SchemaMeta, err error) {
 	if t.ExplicitSchema {
 		if t.ExplicitCatalog {
-			// log.VEventf(ctx, 2, "full target already resolved: %q", t)
 			// Already 3 parts: nothing to do.
 			return r.LookupSchema(ctx, t.Catalog(), t.Schema())
 		}
@@ -322,7 +317,6 @@ func (t *TableName) ResolveTarget(
 			if err == nil {
 				t.CatalogName = Name(curDb)
 			}
-			// log.VEventf(ctx, 2, "two part target lookup: %+v // %v", t, err)
 			return found, scMeta, err
 		}
 		// No luck so far. Compatibility with CockroachDB v1.1: use D.public.T instead.
@@ -332,22 +326,22 @@ func (t *TableName) ResolveTarget(
 				t.SchemaName = PublicSchemaName
 				t.ExplicitCatalog = true
 			}
-			// log.VEventf(ctx, 2, "two part target lookup compat: %+v // %v", t, err)
 			return found, scMeta, err
 		}
 		// Welp, really haven't found anything.
 		return false, nil, nil
 	}
 
-	// This is a naked table name. Use the current schema = the first item in the search path.
-	hasFirst, firstSchema := searchPath.FirstSpecified()
-	if hasFirst {
-		if found, scMeta, err = r.LookupSchema(ctx, curDb, firstSchema); found || err != nil {
+	// This is a naked table name. Use the current schema = the first
+	// valid item in the search path.
+	iter := searchPath.IterWithoutImplicitPGCatalog()
+	for scName, ok := iter(); ok; scName, ok = iter() {
+		if found, scMeta, err = r.LookupSchema(ctx, curDb, scName); found || err != nil {
 			if err == nil {
 				t.CatalogName = Name(curDb)
-				t.SchemaName = Name(firstSchema)
+				t.SchemaName = Name(scName)
 			}
-			// log.VEventf(ctx, 2, "one part target lookup: %+v // %v", t, err)
+			break
 		}
 	}
 	return found, scMeta, err
@@ -384,14 +378,16 @@ func (tp *TableNamePrefix) Resolve(
 		// No luck.
 		return false, nil, nil
 	}
-	// This is a naked table name. Use the current schema = the first item in the search path.
-	hasFirst, firstSchema := searchPath.FirstSpecified()
-	if hasFirst {
-		if found, scMeta, err = r.LookupSchema(ctx, curDb, firstSchema); found || err != nil {
+	// This is a naked table name. Use the current schema = the first
+	// valid item in the search path.
+	iter := searchPath.IterWithoutImplicitPGCatalog()
+	for scName, ok := iter(); ok; scName, ok = iter() {
+		if found, scMeta, err = r.LookupSchema(ctx, curDb, scName); found || err != nil {
 			if err == nil {
 				tp.CatalogName = Name(curDb)
-				tp.SchemaName = Name(firstSchema)
+				tp.SchemaName = Name(scName)
 			}
+			break
 		}
 	}
 	return found, scMeta, err

pkg/sql/sem/tree/name_resolution_test.go

@@ -693,7 +693,8 @@ func TestResolveTablePatternOrName(t *testing.T) {
 		{`pg_tables`, ``, mpath("public", "pg_catalog"), true, `pg_tables`, `"".pg_catalog.pg_tables`, `.pg_catalog[0]`, ``},
 		{`pg_tables`, ``, mpath(), true, `pg_tables`, `"".pg_catalog.pg_tables`, `.pg_catalog[0]`, ``},
 
-		{`blix`, ``, mpath("public", "pg_catalog"), false, ``, ``, ``, `prefix or object not found`},
+		{`blix`, ``, mpath("public"), false, ``, ``, ``, `prefix or object not found`},
+		{`blix`, ``, mpath("public", "pg_catalog"), false, `blix`, `"".pg_catalog.blix`, `.pg_catalog`, ``},
 
 		// Names of length 2.
 
@@ -747,7 +748,8 @@ func TestResolveTablePatternOrName(t *testing.T) {
 
 		// Patterns of length 1.
 
-		{`*`, ``, mpath("public", "pg_catalog"), false, ``, ``, ``, `prefix or object not found`},
+		{`*`, ``, mpath("public"), false, ``, ``, ``, `prefix or object not found`},
+		{`*`, ``, mpath("public", "pg_catalog"), false, `*`, `"".pg_catalog.*`, `.pg_catalog`, ``},
 
 		// Patterns of length 2.
 

pkg/sql/sessiondata/search_path.go

@@ -43,16 +43,6 @@ func MakeSearchPath(paths []string) SearchPath {
 	}
 }
 
-// FirstSpecified returns true and the first element if the list of
-// specified items is non-empty, or false and an empty string
-// otherwise.  Used by current_schema().
-func (s SearchPath) FirstSpecified() (bool, string) {
-	if len(s.paths) == 0 {
-		return false, ""
-	}
-	return true, s.paths[0]
-}
-
 // Iter returns an iterator through the search path. We must include the
 // implicit pg_catalog at the beginning of the search path, unless it has been
 // explicitly set later by the user.