Skip to content

Commit

Permalink
sql: make the lookup of built-in functions smarted wrt pg_catalog.
Browse files Browse the repository at this point in the history 
Prior to this patch, there were two sorts of functions:

- functions defined with a "`pg_catalog.`" prefix. These functions
  could be found either when used with their prefix, or without their
  prefix as long as `pg_catalog` was in the search path (and the
  session was defined to include `pg_catalog` in the search path by
  default).

- functions defined without a "`pg_catalog.`" prefix. These functions
  could only be found when used without a prefix.

This created a lurking bug with regards to compatibility: in
PostgreSQL, *all* built-in functions reside in namespace `pg_catalog`,
not only those fews that have motivated the "`pg_catalog.`" prefix in
CockroachDB so far. For example, `SELECT pg_catalog.count(*) FROM foo`
is valid, and so is `SELECT pg_catalog.length(name) FROM users`. It
was just a matter of time before a user would demand compatibility
with a tool producing pg queries automatically using this form.

To address this, three solutions were considered:

- move all built-in functions supported by CockroachDB to the
  namespace `pg_catalog`. Since `pg_catalog` is always searched (it's
  always part of the search path), unqualified names would still be
  found. This would achieve compatibility but make the name
  "`pg_catalog`" much more prominent in error messages, which use the
  fully qualified name of the function in all cases.

- move all built-in functions supported by CockroachDB to a new
  namespace with a more generic name, say "`sql`", then tweak the
  lookup rules so that whenever the prefix "`pg_catalog`" is used, it
  would be substituted implicitly by "`sql`". This would achieve
  compatibility and also make the error messages less pg-flavored. The
  drawback of this approach is that it introduces a mandatory prefix
  "`sql.`" in many places in the code and error messages where it was
  not present before.

- move all built-in functions supported by both CockroachDB and
  PostgreSQL to the global namespace (the one with no prefix), then
  tweak the lookup rules so that whenever the prefix "`pg_catalog`" is
  used, it would be removed to look up the remainder of the name in
  the global namespace. This also achieves compatibility and has the
  least impact on the code and error messages. The drawback of this
  approach is that those functions that are really pg-specific and
  merely present to please ORMs looking at `pg_proc` now also appear
  in the auto-generated docs for the global namespace.

The last solution was deemed to offer the best trade-off and was thus
adopted for the time being. Remaining issues with documentation, if
any, can be addressed in a subsequent patch by adding a boolean to
mark the builtin definition of those pg compatibility functions we do
not want to promote.
  • Loading branch information
@knz
knz committed Feb 4, 2017
1 parent 110cb63 commit 7cd4dff
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 32 deletions.
41 changes: 20 additions & 21 deletions pkg/sql/parser/builtins.go
View file
Expand Up @@ -1476,17 +1476,16 @@ var Builtins = map[string][]Builtin{
},
},

// pg_catalog functions.
// See https://www.postgresql.org/docs/9.6/static/functions-info.html.
"pg_catalog.pg_backend_pid": {
"pg_backend_pid": {
Builtin{
Types: ArgTypes{},
ReturnType: TypeInt,
fn: func(_ *EvalContext, _ DTuple) (Datum, error) {
return NewDInt(-1), nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
},

Expand All @@ -1496,7 +1495,7 @@ var Builtins = map[string][]Builtin{
// corresponding expression as a string, which means that this function can simply
// return the first argument directly. It also means we can ignore the second and
// optional third argument.
"pg_catalog.pg_get_expr": {
"pg_get_expr": {
Builtin{
Types: ArgTypes{
{"pg_node_tree", TypeString},
Expand All @@ -1507,7 +1506,7 @@ var Builtins = map[string][]Builtin{
return args[0], nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
Builtin{
Types: ArgTypes{
Expand All @@ -1520,13 +1519,13 @@ var Builtins = map[string][]Builtin{
return args[0], nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
},

// pg_get_indexdef functions like SHOW CREATE INDEX would if we supported that
// statement.
"pg_catalog.pg_get_indexdef": {
"pg_get_indexdef": {
Builtin{
Types: ArgTypes{
{"index_oid", TypeInt},
Expand All @@ -1545,14 +1544,14 @@ var Builtins = map[string][]Builtin{
return r[0], nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
// The other overload for this function, pg_get_indexdef(index_oid,
// column_no, pretty_bool), is unimplemented, because it isn't used by
// supported ORMs.
},

"pg_catalog.pg_typeof": {
"pg_typeof": {
// TODO(knz): This is a proof-of-concept until TypeAny works
// properly.
Builtin{
Expand All @@ -1562,10 +1561,10 @@ var Builtins = map[string][]Builtin{
return NewDString(args[0].ResolvedType().String()), nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
},
"pg_catalog.pg_get_userbyid": {
"pg_get_userbyid": {
Builtin{
Types: ArgTypes{
{"role_oid", TypeInt},
Expand All @@ -1583,10 +1582,10 @@ var Builtins = map[string][]Builtin{
return t[0], nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
},
"pg_catalog.format_type": {
"format_type": {
Builtin{
// TODO(jordan) typemod should be a Nullable TypeInt when supported.
Types: ArgTypes{{"type_oid", TypeInt}, {"typemod", TypeInt}},
Expand All @@ -1604,48 +1603,48 @@ var Builtins = map[string][]Builtin{
"Currently, the type modifier is ignored.",
},
},
"pg_catalog.col_description": {
"col_description": {
Builtin{
Types: ArgTypes{{"table_oid", TypeInt}, {"column_number", TypeInt}},
ReturnType: TypeString,
fn: func(_ *EvalContext, _ DTuple) (Datum, error) {
return DNull, nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
},
"pg_catalog.obj_description": {
"obj_description": {
Builtin{
Types: ArgTypes{{"object_oid", TypeInt}},
ReturnType: TypeString,
fn: func(_ *EvalContext, _ DTuple) (Datum, error) {
return DNull, nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
},
"pg_catalog.shobj_description": {
"shobj_description": {
Builtin{
Types: ArgTypes{{"object_oid", TypeInt}, {"catalog_name", TypeString}},
ReturnType: TypeString,
fn: func(_ *EvalContext, _ DTuple) (Datum, error) {
return DNull, nil
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
},
"pg_catalog.array_in": {
"array_in": {
Builtin{
Types: ArgTypes{{"string", TypeString}, {"element_oid", TypeInt}, {"element_typmod", TypeInt}},
ReturnType: TypeString,
fn: func(_ *EvalContext, _ DTuple) (Datum, error) {
return nil, errors.New("unimplemented")
},
category: categoryCompatibility,
Info: "Not usable; exposed only for ORM compatibility.",
Info: "Not usable; exposed only for ORM compatibility with PostgreSQL.",
},
},
}
Expand Down
10 changes: 10 additions & 0 deletions pkg/sql/parser/function_definition.go
View file
Expand Up @@ -86,9 +86,19 @@ func (n UnresolvedName) ResolveFunction(searchPath SearchPath) (*FunctionDefinit
// Name.Normalize(), functions are special in that they are
// guaranteed to not contain special Unicode characters. So we can
// use ToLower directly.
// TODO(knz) this will need to be revisited once we allow
// function names to exist in custom namespaces, whose names
// may contain special characters.
prefix := strings.ToLower(fn.prefix())
smallName := strings.ToLower(fn.function())
fullName := smallName

if prefix == "pg_catalog" {
// If the user specified e.g. `pg_catalog.max()` we want to find
// it in the global namespace.
prefix = ""
}

if prefix != "" {
fullName = prefix + "." + smallName
}
Expand Down
3 changes: 1 addition & 2 deletions pkg/sql/parser/function_name_test.go
View file
Expand Up @@ -28,8 +28,7 @@ func TestResolveFunction(t *testing.T) {
err string
}{
{`count`, `count`, ``},
{`pg_catalog.pg_typeof`, `pg_catalog.pg_typeof`, ``},
{`pg_typeof`, `pg_catalog.pg_typeof`, ``},
{`pg_catalog.pg_typeof`, `pg_typeof`, ``},

{`foo`, ``, `unknown function: foo`},
{`""`, ``, `invalid function name: ""`},
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/pg_catalog.go
View file
Expand Up @@ -1363,7 +1363,7 @@ var (

typDelim = parser.NewDString(",")

arrayInProcName = "pg_catalog.array_in"
arrayInProcName = "array_in"
arrayInProcOid = makeOidHasher().BuiltinOid(arrayInProcName, &parser.Builtins[arrayInProcName][0])
)

Expand Down
10 changes: 8 additions & 2 deletions pkg/sql/testdata/builtin_function
View file
Expand Up @@ -1436,7 +1436,7 @@ SELECT pg_catalog.pg_get_indexdef((SELECT oid from pg_class WHERE relname='pg_in
----
CREATE INDEX pg_indexdef_idx ON test.pg_indexdef_test (a ASC)

query error pq: unknown signature: pg_catalog.pg_get_indexdef\(int, int, bool\)
query error pq: unknown signature: pg_get_indexdef\(int, int, bool\)
SELECT pg_catalog.pg_get_indexdef(0, 0, true)

# This function always returns NULL since we don't support column comments.
Expand All @@ -1445,5 +1445,11 @@ SELECT col_description('pg_class'::regclass::oid, 2)
----
NULL

query error pq: pg_catalog.array_in\(\): unimplemented
query error pq: array_in\(\): unimplemented
SELECT array_in('foo', 3, -1)

# Check that base function names are also visible in namespace pg_catalog.
query I
SELECT pg_catalog.length('hello')
----
5
8 changes: 4 additions & 4 deletions pkg/sql/testdata/pg_catalog
View file
Expand Up @@ -718,10 +718,10 @@ oid typname typinput typoutput typreceive typsend typmodin typmodo
26 oid 0 0 0 0 0 0 0
700 float4 0 0 0 0 0 0 0
701 float8 0 0 0 0 0 0 0
1005 int2[] 2892088402 0 0 0 0 0 0
1007 int4[] 2892088402 0 0 0 0 0 0
1009 string[] 2892088402 0 0 0 0 0 0
1016 int8[] 2892088402 0 0 0 0 0 0
1005 int2[] 4288767817 0 0 0 0 0 0
1007 int4[] 4288767817 0 0 0 0 0 0
1009 string[] 4288767817 0 0 0 0 0 0
1016 int8[] 4288767817 0 0 0 0 0 0
1043 string 0 0 0 0 0 0 0
1082 date 0 0 0 0 0 0 0
1114 timestamp 0 0 0 0 0 0 0
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/testdata/pgoidtype
View file
Expand Up @@ -39,12 +39,12 @@ SELECT 'max'::REGPROC
query OOOO
SELECT 'array_in'::REGPROC, 'array_in(a,b,c)'::REGPROC, 'pg_catalog.array_in'::REGPROC, 'pg_catalog.array_in( a ,b, c )'::REGPROC
----
2892088402 2892088402 2892088402 2892088402
4288767817 4288767817 4288767817 4288767817

query OOOO
SELECT 'array_in'::REGPROCEDURE, 'array_in(a,b,c)'::REGPROCEDURE, 'pg_catalog.array_in'::REGPROCEDURE, 'pg_catalog.array_in( a ,b, c )'::REGPROCEDURE
----
2892088402 2892088402 2892088402 2892088402
4288767817 4288767817 4288767817 4288767817

query O
SELECT 'public'::REGNAMESPACE
Expand Down

0 comments on commit 7cd4dff

Please sign in to comment.