Skip to content

Commit 1f8b7ae

Browse files
committed
sql: avoid internal error when type-checking routine in edge case
We just saw a sentry issue where `tree.Overload.Types` was `nil`. This is unexpected, and it resulted in a interface conversion panic during type-checking because we assumed that this field is castable to `tree.ParamTypes`. This usage was introduced in 92240f8 and is now fixed in this commit. I've audited the code and couldn't find a way for us to produce such a routine, so there is no reproduction test case added. I did add a defensive mechanism to ensure that all builtin overloads (i.e. non-user-defined) have non-nil `Types` set. This commit also removes an unused (but somewhat related) method from the catalog. Release note: None
1 parent c821431 commit 1f8b7ae

File tree

3 files changed

+19
-6
lines changed

3 files changed

+19
-6
lines changed

pkg/sql/catalog/funcdesc/func_desc.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -608,11 +608,6 @@ func (desc *Mutable) SetDeclarativeSchemaChangerState(state *scpb.DescriptorStat
608608
desc.DeclarativeSchemaChangerState = state
609609
}
610610

611-
// AddParams adds function parameters to the parameter list.
612-
func (desc *Mutable) AddParams(params ...descpb.FunctionDescriptor_Parameter) {
613-
desc.Params = append(desc.Params, params...)
614-
}
615-
616611
// SetVolatility sets the volatility attribute.
617612
func (desc *Mutable) SetVolatility(v catpb.Function_Volatility) {
618613
desc.Volatility = v

pkg/sql/sem/builtins/builtinsregistry/builtins_registry.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ func Register(name string, props *tree.FunctionProperties, overloads []tree.Over
2626
if _, exists := registry[name]; exists {
2727
panic("duplicate builtin: " + name)
2828
}
29+
for _, overload := range overloads {
30+
if overload.Types == nil || overload.Types == tree.TypeList(nil) {
31+
panic("nil Types field for builtin: " + name)
32+
}
33+
}
2934
registry[name] = definition{
3035
props: props,
3136
overloads: overloads,

pkg/sql/sem/tree/overload.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
1818
"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
1919
"github.com/cockroachdb/cockroach/pkg/sql/types"
20+
"github.com/cockroachdb/cockroach/pkg/util/buildutil"
2021
"github.com/cockroachdb/cockroach/pkg/util/intsets"
2122
"github.com/cockroachdb/errors"
2223
"github.com/cockroachdb/redact"
@@ -1108,7 +1109,19 @@ func (s *overloadTypeChecker) typeCheckOverloadedExprs(
11081109
// Don't filter builtin routines.
11091110
return true
11101111
}
1111-
params := ol.Types.(ParamTypes)
1112+
params, ok := ol.Types.(ParamTypes)
1113+
if !ok {
1114+
if buildutil.CrdbTestBuild {
1115+
// All user-defined routines are expected to have non-nil
1116+
// ParamTypes set.
1117+
panic(errors.AssertionFailedf(
1118+
"found user-defined routine with non-ParamTypes Types field, %v", ol,
1119+
))
1120+
}
1121+
// Don't filter overloads that have nil ParamTypes or have variadic
1122+
// or homogeneous types.
1123+
return true
1124+
}
11121125
var outParams ParamTypes
11131126
if ol.Type == ProcedureRoutine && foundOutParams {
11141127
outParams = ol.OutParamTypes.(ParamTypes)

0 commit comments

Comments
 (0)