Merge pull request #2223 from dolthub/zachmu/regclass

put pg_catalog implicitly on the search_path when it isn't there already

Author: zachmu

core/context.go

@@ -199,7 +199,7 @@ func GetSqlTableFromContext(ctx *sql.Context, databaseName string, tableName dol
 	var searchPath []string
 	if len(tableName.Schema) == 0 {
 		// If a schema was not provided, then we'll use the search path
-		searchPath, err = resolve.SearchPath(ctx)
+		searchPath, err = SearchPath(ctx)
 		if err != nil {
 			return nil, err
 		}
@@ -228,6 +228,30 @@ func GetSqlTableFromContext(ctx *sql.Context, databaseName string, tableName dol
 	return nil, nil
 }
 
+// SearchPath returns the effective schema search path for the current session
+func SearchPath(ctx *sql.Context) ([]string, error) {
+	path, err := resolve.SearchPath(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	// pg_catalog is *always* implicitly part of the search path as the first element, unless it's specifically
+	// included later. This allows users to override built-in names with user-defined names, but they have to
+	// opt in to that behavior.
+	hasPgCatalog := false
+	for _, schema := range path {
+		if schema == "pg_catalog" {
+			hasPgCatalog = true
+			break
+		}
+	}
+
+	if !hasPgCatalog {
+		path = append([]string{"pg_catalog"}, path...)
+	}
+	return path, nil
+}
+
 // GetExtensionsCollectionFromContext returns the extensions collection from the given context. Will always return a
 // collection if no error is returned.
 func GetExtensionsCollectionFromContext(ctx *sql.Context, database string) (*extensions.Collection, error) {

server/functions/pg_table_is_visible.go

@@ -15,11 +15,10 @@
 package functions
 
 import (
-	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/resolve"
 	"github.com/dolthub/go-mysql-server/sql"
 
+	"github.com/dolthub/doltgresql/core"
 	"github.com/dolthub/doltgresql/core/id"
-
 	"github.com/dolthub/doltgresql/server/functions/framework"
 	pgtypes "github.com/dolthub/doltgresql/server/types"
 )
@@ -38,7 +37,7 @@ var pg_table_is_visible_oid = framework.Function1{
 	Strict:             true,
 	Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
 		oidVal := val.(id.Id)
-		paths, err := resolve.SearchPath(ctx)
+		paths, err := core.SearchPath(ctx)
 		if err != nil {
 			return false, err
 		}

server/functions/pg_type_is_visible.go

@@ -15,9 +15,10 @@
 package functions
 
 import (
-	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/resolve"
 	"github.com/dolthub/go-mysql-server/sql"
 
+	"github.com/dolthub/doltgresql/core"
+
 	"github.com/dolthub/doltgresql/core/id"
 	"github.com/dolthub/doltgresql/server/functions/framework"
 	pgtypes "github.com/dolthub/doltgresql/server/types"
@@ -48,7 +49,7 @@ var pg_type_is_visible = framework.Function1{
 		schemaName := oidVal.Segment(0)
 
 		// Get the current search path
-		searchPath, err := resolve.SearchPath(ctx)
+		searchPath, err := core.SearchPath(ctx)
 		if err != nil {
 			return false, err
 		}

server/functions/regclass.go

@@ -19,9 +19,9 @@ import (
 	"strconv"
 
 	"github.com/cockroachdb/errors"
-	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/resolve"
 	"github.com/dolthub/go-mysql-server/sql"
 
+	"github.com/dolthub/doltgresql/core"
 	"github.com/dolthub/doltgresql/core/id"
 	"github.com/dolthub/doltgresql/server/functions/framework"
 	"github.com/dolthub/doltgresql/server/settings"
@@ -65,7 +65,7 @@ var regclassin = framework.Function1{
 		switch len(sections) {
 		case 1:
 			database = ctx.GetCurrentDatabase()
-			searchSchemas, err = resolve.SearchPath(ctx)
+			searchSchemas, err = core.SearchPath(ctx)
 			if err != nil {
 				return id.Null, err
 			}
@@ -79,7 +79,7 @@ var regclassin = framework.Function1{
 			searchSchemas = []string{sections[2]}
 			relationName = sections[4]
 		default:
-			return id.Null, errors.Errorf("regclass failed validation")
+			return id.Null, errors.Errorf("unexpected input for regclass: %s", input)
 		}
 
 		// Iterate over all of the items to find which relation matches.

server/node/create_trigger.go

@@ -16,7 +16,6 @@ package node
 
 import (
 	"github.com/cockroachdb/errors"
-	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/resolve"
 	"github.com/dolthub/go-mysql-server/sql"
 	"github.com/dolthub/go-mysql-server/sql/plan"
 	vitess "github.com/dolthub/vitess/go/vt/sqlparser"
@@ -180,11 +179,10 @@ func loadFunction(ctx *sql.Context, funcCollection *functions.Collection, funcID
 			return functions.Function{}, err
 		}
 	} else {
-		searchPaths, err := resolve.SearchPath(ctx)
+		searchPaths, err := core.SearchPath(ctx)
 		if err != nil {
 			return functions.Function{}, err
 		}
-		searchPaths = append(searchPaths, "pg_catalog") // This isn't included in the search path but functions use it
 		for _, searchPath := range searchPaths {
 			function, err = funcCollection.GetFunction(ctx, id.NewFunction(searchPath, funcID.FunctionName()))
 			if err != nil {

testing/go/prepared_statement_test.go

@@ -1341,8 +1341,29 @@ var pgCatalogTests = []ScriptTest{
 		},
 		Assertions: []ScriptTestAssertion{
 			{
-				// https://github.com/dolthub/doltgresql/issues/2217
-				Skip: true,
+				Query:    "select distinct relnamespace from pg_catalog.pg_class c INNER JOIN pg_catalog.pg_namespace n ON c.relnamespace = n.oid WHERE n.nspname=$1;",
+				BindVars: []any{"testschema"},
+				Expected: []sql.Row{{2638679668}},
+			},
+			// TODO: when this test is run in isolation without the query above, the below query returns no rows. This is
+			//  because the process of converting an OID to its internal ID can only proceed in one direction: from internal
+			//  to OID. When the above query runs, it causes the internal ID for the testschema namespace to be cached,
+			//  allowing the reverse lookup to succeed in subsequent queries. For OIDs that have not yet been cached in this
+			//  manner, lookups by their OID will fail. This doesn't impact all queries since many of them get an index
+			//  lookup on OID, which has the side effect of converting everything to numeric IDs anyway. But for queries
+			//  that use a normal comparison function for an OID literal value, the conversion to an internal ID of the
+			//  appropriate type (e.g. id.Namespace) cannot happen in the |oidin| function in some cases because the internal
+			//  to OID mapping hasn't yet been established for that schema element, so the comparison fails, yielding
+			//  incorrect results.
+			//  To fix this, we need to correctly seed the internal ID cache with all schema elements in the database.
+			{
+				Query: `SELECT c.oid,pg_catalog.pg_get_expr(c.relpartbound, c.oid) as partition_expr,  pg_catalog.pg_get_partkeydef(c.oid) as partition_key 
+FROM pg_catalog.pg_class c
+WHERE c.relnamespace=$1 AND c.relkind not in ('i','I','c') and c.oid not in (select oid from pg_catalog.pg_class where left(relname, 5) = 'dolt_');`,
+				BindVars: []any{2638679668},
+				Expected: []sql.Row{{1712283605, nil, ""}},
+			},
+			{
 				Query: `SELECT c.oid,d.description,pg_catalog.pg_get_expr(c.relpartbound, c.oid) as partition_expr,  pg_catalog.pg_get_partkeydef(c.oid) as partition_key 
 FROM pg_catalog.pg_class c
 LEFT OUTER JOIN pg_catalog.pg_description d ON d.objoid=c.oid AND d.objsubid=0 AND d.classoid='pg_class'::regclass
@@ -1351,10 +1372,8 @@ WHERE c.relnamespace=$1 AND c.relkind not in ('i','I','c') and c.oid not in (sel
 				Expected: []sql.Row{{1712283605, nil, nil, ""}},
 			},
 			{
-				// https://github.com/dolthub/doltgresql/issues/2217
-				Skip:  true,
 				Query: `SELECT d.description from pg_catalog.pg_description d WHERE d.classoid='pg_class'::regclass`,
-				// TODO: add expected values
+				// TODO: add expected values (pg_description not yet implemented)
 			},
 			{
 				Query:    `select c.oid,pg_catalog.pg_total_relation_size(c.oid) as total_rel_size,pg_catalog.pg_relation_size(c.oid) as rel_size FROM pg_class c WHERE c.relnamespace=$1 and c.oid not in (select oid from pg_catalog.pg_class where left(relname, 5) = 'dolt_');`,