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.

There's an increase in number of round trips, but it's a constant
factor.

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.

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -53,9 +53,9 @@ exp,benchmark
 11,GrantRole/grant_1_role
 15,GrantRole/grant_2_roles
 3,ORMQueries/activerecord_type_introspection_query
-3,ORMQueries/django_table_introspection_1_table
-3,ORMQueries/django_table_introspection_4_tables
-3,ORMQueries/django_table_introspection_8_tables
+6,ORMQueries/django_table_introspection_1_table
+6,ORMQueries/django_table_introspection_4_tables
+6,ORMQueries/django_table_introspection_8_tables
 2,ORMQueries/has_column_privilege_using_attnum
 2,ORMQueries/has_column_privilege_using_column_name
 1,ORMQueries/has_schema_privilege_1

pkg/sql/faketreeeval/evalctx.go

@@ -68,13 +68,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,
@@ -372,13 +365,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,

pkg/sql/importer/import_table_creation.go

@@ -303,13 +303,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,

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

pkg/sql/resolver.go

@@ -138,56 +138,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) {
-	dbDesc, err := p.Descriptors().GetImmutableDatabaseByName(ctx, p.Txn(), curDB,
-		tree.DatabaseLookupFlags{
-			Required:    true,
-			AvoidLeased: p.skipDescriptorCache,
-		})
-	if err != nil {
-		return false, false, err
-	}
-	// It is critical that we set ParentID on the flags in order to ensure that
-	// we do not do a very expensive, and ultimately fruitless lookup for an
-	// OID which definitely does not exist. Only OIDs corresponding to relations
-	// in the current database are relevant for this function. If we have already
-	// fetched all the tables in the current database, then we can use that
-	// fact to avoid a KV lookup. The descs layer relies on our setting this
-	// field in the flags to avoid that lookup.
-	flags := p.ObjectLookupFlags(true /* required */, false /* requireMutable */)
-	flags.ParentID = dbDesc.GetID()
-	tableDesc, err := p.Descriptors().GetImmutableTableByID(ctx, p.Txn(), descpb.ID(tableID), flags)
-	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
-	}
-	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,

pkg/sql/sem/builtins/pg_builtins.go

@@ -1217,16 +1217,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 INNER LOOKUP 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,

pkg/sql/sem/eval/deps.go

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