Skip to content

Commit

Permalink
sql: make builtin function oids deterministic
Browse files Browse the repository at this point in the history
Previously, oids were assigned to functions in the order
they were registered, so adding an alphabetically-earlier
function, overload, or file would change the oid. This was
mainly inconvenient for a few tests but also made it unsafe
to do things like serialize a function oid on a database
descriptor.

This commit hardcodes all builtin function oids to what
they were as of its immediate ancestor commit, and enforces
that newly-added overloads also get hardcoded.

The only reason this isn't a generator is that right now
running a generator is about equally onerous as adding one
line to fixed_oids.go. If that changes, this should be a generator.

Release justification: Low-impact refactor.
Release note: None
  • Loading branch information
HonoreDB committed Sep 7, 2022
1 parent 1ac25dc commit 5ee5d85
Show file tree
Hide file tree
Showing 6 changed files with 2,049 additions and 27 deletions.
4 changes: 2 additions & 2 deletions pkg/sql/logictest/testdata/logic_test/pg_catalog
Original file line number Diff line number Diff line change
Expand Up @@ -4672,7 +4672,7 @@ FROM pg_proc p
JOIN pg_type t ON t.typinput = p.oid
WHERE t.typname = '_int4'
----
2018 array_in array_in
2013 array_in array_in

## #16285
## int2vectors should be 0-indexed
Expand Down Expand Up @@ -4710,7 +4710,7 @@ SELECT cur_max_builtin_oid FROM [SELECT max(oid) as cur_max_builtin_oid FROM pg_
query TT
SELECT proname, oid FROM pg_catalog.pg_proc WHERE oid = $cur_max_builtin_oid
----
to_regtype 2038
pg_get_functiondef 2035

## Ensure that unnest works with oid wrapper arrays

Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/logictest/testdata/logic_test/pgoidtype
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,15 @@ WHERE relname = 'pg_constraint'
query OOOO
SELECT 'upper'::REGPROC, 'upper'::REGPROCEDURE, 'pg_catalog.upper'::REGPROCEDURE, 'upper'::REGPROC::OID
----
upper upper upper 833
upper upper upper 829

query error invalid function name
SELECT 'invalid.more.pg_catalog.upper'::REGPROCEDURE

query OOO
SELECT 'upper(int)'::REGPROC, 'upper(int)'::REGPROCEDURE, 'upper(int)'::REGPROC::OID
----
upper upper 833
upper upper 829

query error unknown function: blah\(\)
SELECT 'blah(ignored, ignored)'::REGPROC, 'blah(ignored, ignored)'::REGPROCEDURE
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/sem/builtins/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ go_library(
"aggregate_builtins.go",
"all_builtins.go",
"builtins.go",
"fixed_oids.go",
"generator_builtins.go",
"generator_probe_ranges.go",
"geo_builtins.go",
Expand Down
4 changes: 4 additions & 0 deletions pkg/sql/sem/builtins/all_builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ func init() {
tree.FunDefs = make(map[string]*tree.FunctionDefinition)
tree.ResolvedBuiltinFuncDefs = make(map[string]*tree.ResolvedFunctionDefinition)
builtinsregistry.Iterate(func(name string, props *tree.FunctionProperties, overloads []tree.Overload) {
for i, fn := range overloads {
signature := name + fn.Signature(true)
overloads[i].Oid = signatureMustHaveHardcodedOID(signature)
}
fDef := tree.NewFunctionDefinition(name, props, overloads)
addResolvedFuncDef(tree.ResolvedBuiltinFuncDefs, fDef)
tree.FunDefs[name] = fDef
Expand Down
23 changes: 0 additions & 23 deletions pkg/sql/sem/builtins/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/colexecerror"
"github.com/cockroachdb/cockroach/pkg/sql/lex"
"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
"github.com/cockroachdb/cockroach/pkg/sql/oidext"
"github.com/cockroachdb/cockroach/pkg/sql/parser"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
Expand Down Expand Up @@ -96,7 +95,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/uuid"
"github.com/cockroachdb/errors"
"github.com/knz/strtime"
"github.com/lib/pq/oid"
"golang.org/x/text/cases"
"golang.org/x/text/language"
)
Expand Down Expand Up @@ -161,28 +159,7 @@ func arrayProps() tree.FunctionProperties {
return tree.FunctionProperties{Category: builtinconstants.CategoryArray}
}

// builtinFuncOIDCnt is used as a counter to assign OIDs to builtin function
// overloads. OIDs are assigned in the order of overloads being initialized by
// go. That is, OIDs are given by alphabetical order of the file name a function
// is declared and the order of tree.Overload is declared in that file. So
// changing file names or function declaration position will change OIDs.
var builtinFuncOIDCnt = 0

// Please always use this function to creat builtinDefinition instead of
// construct it directly. Otherwise, the new builtin function will not have an
// OID.
func makeBuiltin(props tree.FunctionProperties, overloads ...tree.Overload) builtinDefinition {
for i := range overloads {
builtinFuncOIDCnt++
if builtinFuncOIDCnt > oidext.CockroachPredefinedOIDMax {
panic(
errors.AssertionFailedf(
"builtin function oid exceeds maximum predefined oid %d", oidext.CockroachPredefinedOIDMax,
),
)
}
overloads[i].Oid = oid.Oid(builtinFuncOIDCnt)
}
return builtinDefinition{
props: props,
overloads: overloads,
Expand Down
Loading

0 comments on commit 5ee5d85

Please sign in to comment.