sql: fix pg_table_is_visible to handle indexes

This uses the internal executor now to do the introspection using pg_class where everything can be looked up all at once. This should be faster compared to previous releases, since there have been improvements in the internal executor. Release note (bug fix): Fixed the pg_table_is_visible builtin function so it correctly reports visibility of indexes based on the current search_path.
cockroachdb · Oct 26, 2022 · a07c766 · a07c766
1 parent 0e7c295
commit a07c766
Show file tree

Hide file tree

Showing 6 changed files with 19 additions and 82 deletions.
diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go
@@ -65,13 +65,6 @@ func (so *DummySequenceOperators) SchemaExists(
 	return false, errors.WithStack(errSequenceOperators)
 }
 
-// IsTableVisible is part of the eval.DatabaseCatalog interface.
-func (so *DummySequenceOperators) IsTableVisible(
-	ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid,
-) (bool, bool, error) {
-	return false, false, errors.WithStack(errSequenceOperators)
-}
-
 // IsTypeVisible is part of the eval.DatabaseCatalog interface.
 func (so *DummySequenceOperators) IsTypeVisible(
 	ctx context.Context, curDB string, searchPath sessiondata.SearchPath, typeID oid.Oid,
@@ -345,13 +338,6 @@ func (ep *DummyEvalPlanner) SchemaExists(ctx context.Context, dbName, scName str
 	return false, errors.WithStack(errEvalPlanner)
 }
 
-// IsTableVisible is part of the eval.DatabaseCatalog interface.
-func (ep *DummyEvalPlanner) IsTableVisible(
-	ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid,
-) (bool, bool, error) {
-	return false, false, errors.WithStack(errEvalPlanner)
-}
-
 // IsTypeVisible is part of the eval.DatabaseCatalog interface.
 func (ep *DummyEvalPlanner) IsTypeVisible(
 	ctx context.Context, curDB string, searchPath sessiondata.SearchPath, typeID oid.Oid,

diff --git a/pkg/sql/importer/import_table_creation.go b/pkg/sql/importer/import_table_creation.go
@@ -298,13 +298,6 @@ func (so *importSequenceOperators) SchemaExists(
 	return false, errSequenceOperators
 }
 
-// IsTableVisible is part of the eval.DatabaseCatalog interface.
-func (so *importSequenceOperators) IsTableVisible(
-	ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid,
-) (bool, bool, error) {
-	return false, false, errors.WithStack(errSequenceOperators)
-}
-
 // IsTypeVisible is part of the eval.DatabaseCatalog interface.
 func (so *importSequenceOperators) IsTypeVisible(
 	ctx context.Context, curDB string, searchPath sessiondata.SearchPath, typeID oid.Oid,

diff --git a/pkg/sql/logictest/testdata/logic_test/pg_builtins b/pkg/sql/logictest/testdata/logic_test/pg_builtins
@@ -99,10 +99,12 @@ SET DATABASE = test;
 query TB rowsort
 SELECT c.relname, pg_table_is_visible(c.oid)
 FROM pg_class c
-WHERE c.relname IN ('is_visible', 'not_visible')
+WHERE c.relname IN ('is_visible', 'not_visible', 'is_visible_pkey', 'not_visible_pkey')
 ----
-is_visible   true
-not_visible  false
+is_visible        true
+is_visible_pkey   true
+not_visible       false
+not_visible_pkey  false
 
 # Looking up a table in a different database should return NULL.
 query B

diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go
@@ -135,54 +135,6 @@ func (p *planner) SchemaExists(ctx context.Context, dbName, scName string) (foun
 	return found, err
 }
 
-// IsTableVisible is part of the eval.DatabaseCatalog interface.
-func (p *planner) IsTableVisible(
-	ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid,
-) (isVisible, exists bool, err error) {
-	tableDesc, err := p.LookupTableByID(ctx, descpb.ID(tableID))
-	if err != nil {
-		// If a "not found" error happened here, we return "not exists" rather than
-		// the error.
-		if errors.Is(err, catalog.ErrDescriptorNotFound) ||
-			errors.Is(err, catalog.ErrDescriptorDropped) ||
-			pgerror.GetPGCode(err) == pgcode.UndefinedTable ||
-			pgerror.GetPGCode(err) == pgcode.UndefinedObject {
-			return false, false, nil //nolint:returnerrcheck
-		}
-		return false, false, err
-	}
-	schemaID := tableDesc.GetParentSchemaID()
-	schemaDesc, err := p.Descriptors().GetImmutableSchemaByID(ctx, p.Txn(), schemaID,
-		tree.SchemaLookupFlags{
-			Required:    true,
-			AvoidLeased: p.skipDescriptorCache})
-	if err != nil {
-		return false, false, err
-	}
-	if schemaDesc.SchemaKind() != catalog.SchemaVirtual {
-		dbID := tableDesc.GetParentID()
-		_, dbDesc, err := p.Descriptors().GetImmutableDatabaseByID(ctx, p.Txn(), dbID,
-			tree.DatabaseLookupFlags{
-				Required:    true,
-				AvoidLeased: p.skipDescriptorCache})
-		if err != nil {
-			return false, false, err
-		}
-		if dbDesc.GetName() != curDB {
-			// If the table is in a different database, then it's considered to be
-			// "not existing" instead of just "not visible"; this matches PostgreSQL.
-			return false, false, nil
-		}
-	}
-	iter := searchPath.Iter()
-	for scName, ok := iter.Next(); ok; scName, ok = iter.Next() {
-		if schemaDesc.GetName() == scName {
-			return true, true, nil
-		}
-	}
-	return false, true, nil
-}
-
 // IsTypeVisible is part of the eval.DatabaseCatalog interface.
 func (p *planner) IsTypeVisible(
 	ctx context.Context, curDB string, searchPath sessiondata.SearchPath, typeID oid.Oid,

diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go
@@ -1216,16 +1216,26 @@ SELECT description
 			ReturnType: tree.FixedReturnType(types.Bool),
 			Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) {
 				oidArg := tree.MustBeDOid(args[0])
-				isVisible, exists, err := evalCtx.Planner.IsTableVisible(
-					ctx, evalCtx.SessionData().Database, evalCtx.SessionData().SearchPath, oidArg.Oid,
+				row, err := evalCtx.Planner.QueryRowEx(
+					ctx, "pg_table_is_visible",
+					sessiondata.NoSessionDataOverride,
+					"SELECT n.nspname from pg_class c JOIN pg_namespace n ON c.relnamespace = n.oid WHERE c.oid=$1 LIMIT 1",
+					oidArg.Oid,
 				)
 				if err != nil {
 					return nil, err
 				}
-				if !exists {
+				if row == nil {
 					return tree.DNull, nil
 				}
-				return tree.MakeDBool(tree.DBool(isVisible)), nil
+				foundSchemaName := string(tree.MustBeDString(row[0]))
+				iter := evalCtx.SessionData().SearchPath.Iter()
+				for scName, ok := iter.Next(); ok; scName, ok = iter.Next() {
+					if foundSchemaName == scName {
+						return tree.DBoolTrue, nil
+					}
+				}
+				return tree.DBoolFalse, nil
 			},
 			Info:       "Returns whether the table with the given OID belongs to one of the schemas on the search path.",
 			Volatility: volatility.Stable,

diff --git a/pkg/sql/sem/eval/deps.go b/pkg/sql/sem/eval/deps.go
@@ -66,12 +66,6 @@ type DatabaseCatalog interface {
 	// whether it exists.
 	SchemaExists(ctx context.Context, dbName, scName string) (found bool, err error)
 
-	// IsTableVisible checks if the table with the given ID belongs to a schema
-	// on the given sessiondata.SearchPath.
-	IsTableVisible(
-		ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid,
-	) (isVisible bool, exists bool, err error)
-
 	// IsTypeVisible checks if the type with the given ID belongs to a schema
 	// on the given sessiondata.SearchPath.
 	IsTypeVisible(