Merge #24718

24718: sql: fix the handling of current_schema and search_path r=knz a=knz

Fixes #24472.

Prior to this patch, CockroachDB was following the pg docs to the
letter regarding the relationship between `search_path` and the
"current schema": the current schema was taken to always be the first
item in `search_path`, as indicated in
https://www.postgresql.org/docs/9.6/static/functions-info.html
("current_schema returns the name of the schema that is first in the
search path (or a null value if the search path is empty).")

This in turn impacts the creation of objects (tables, views, etc) when
the name is unqualified: pg also specifies that such objects are to be
created in the current schema.

Unfortunately the pg doc is incorrect, as pg has a more nuanced
behavior: only the _valid_ schemas in `search_path` are considered to
determine the current schema in that way.

This was causing an unfortunate visible compatibility bug: if the user
specified an invalid schema in the search path, subsequent create
operations would fail with CockroachDB and succeed with pg.

This patch corrects this issue by adopting pg's behavior.

Ditto for `current_schemas`.

Release note (sql change, bug fix): the built-in functions
`current_schema` and `current_schemas` now only consider valid
schemas, like PostgreSQL does.

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

@@ -557,6 +557,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
@@ -587,7 +594,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

@@ -1182,12 +1182,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

@@ -2201,6 +2201,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

@@ -332,14 +332,16 @@ func (t *TableName) ResolveTarget(
 		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)
 			}
+			break
 		}
 	}
 	return found, scMeta, err
@@ -376,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.