Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: only re-use resolved routine type when flag is set #143170

Merged
merged 1 commit into from
Mar 25, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/udf
Original file line number Diff line number Diff line change
@@ -981,6 +981,8 @@ LIMIT
subtest end

# Regression test for #104009.
subtest regression_104009

statement ok
CREATE TABLE ab104009(a INT PRIMARY KEY, b INT)

@@ -1005,6 +1007,10 @@ PREPARE p AS SELECT f($1::REGCLASS::INT)
statement ok
EXECUTE p(10)

subtest end

subtest regression_142886

# Regression test for #142886 - we should not be able to create an overload that
# differs only in the type width of the input type.
statement ok
@@ -1015,3 +1021,28 @@ CREATE FUNCTION f142886(p VARCHAR(100)) RETURNS INT LANGUAGE SQL AS $$ SELECT 0;

statement ok
DROP FUNCTION f142886;

subtest end

# Regression test for #142615.
subtest regression_142615

statement ok
create function app_to_db_id(app_id INT8) RETURNS INT8 LANGUAGE plpgsql AS $$ BEGIN RETURN app_id * 2; END; $$;

statement ok
create sequence seq1;

statement ok
create table test (id int8 not null default app_to_db_id(nextval('seq1'::REGCLASS)));

query TTITT rowsort
select * from pg_catalog.pg_attrdef;
----
1508958170 132 2 unique_rowid() unique_rowid()
1202826234 151 1 gen_random_uuid() gen_random_uuid()
1202826232 151 3 now() now()
3466299042 175 1 public.app_to_db_id(nextval('public.seq1'::REGCLASS)) public.app_to_db_id(nextval('public.seq1'::REGCLASS))
3466299041 175 2 unique_rowid() unique_rowid()

subtest end
17 changes: 9 additions & 8 deletions pkg/sql/opt/optbuilder/values.go
Original file line number Diff line number Diff line change
@@ -77,14 +77,15 @@ func (b *Builder) buildValuesClause(
elemPos += numCols
if texpr.ResolvedType().IsWildcardType() {
// Type-check the expression once again in order to update expressions
// that wrap a routine to reflect the modified type. Make sure to use
// the previously resolved type as the desired type, since the AST may
// have been modified to remove type annotations. This can happen when
// the routine's return type is unknown until its body is built.
texpr, err = tree.TypeCheck(b.ctx, texpr, b.semaCtx, texpr.ResolvedType())
if err != nil {
panic(err)
}
// that wrap a routine to reflect the modified type.
func() {
defer b.semaCtx.Properties.Restore(b.semaCtx.Properties)
b.semaCtx.Properties.RoutineUseResolvedType = true
texpr, err = tree.TypeCheck(b.ctx, texpr, b.semaCtx, desired)
if err != nil {
panic(err)
}
}()
}
if typ := texpr.ResolvedType(); typ.Family() != types.UnknownFamily {
if colTypes[colIdx].Family() == types.UnknownFamily {
13 changes: 10 additions & 3 deletions pkg/sql/sem/tree/type_check.go
Original file line number Diff line number Diff line change
@@ -96,6 +96,12 @@ type SemaProperties struct {
// IgnoreUnpreferredOverloads is set to true when "unpreferred" overloads
// should not be used during type-checking and overload resolution.
IgnoreUnpreferredOverloads bool

// RoutineUseResolvedType is set to true when type-checking for a routine
// should reuse the already resolved type, if any. This is used to handle
// "re-type-checking" that occurs for a RECORD-returning routine, for which
// the return type is not known until the routine body is built.
RoutineUseResolvedType bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to use ScalarAncestors here? The special case is a bit confusing to me - it only applies to routines within a VALUES expression?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can use ScalarAncestors, or at least it would potentially be more confusing. We apply the special case to expressions within a VALUES operator, but only on the second type-checking pass which happens when the VALUES column type depends on a record-returning routine. Using ScalarAncestors here would cause us to to also use this special behavior on the first type-checking pass.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

type semaRequirements struct {
@@ -1179,9 +1185,10 @@ func (expr *FuncExpr) typeCheckWithFuncAncestor(semaCtx *SemaContext, fn func()
func (expr *FuncExpr) TypeCheck(
ctx context.Context, semaCtx *SemaContext, desired *types.T,
) (TypedExpr, error) {
if expr.fn != nil && expr.fn.Type != BuiltinRoutine && expr.typ != nil {
// Don't overwrite the resolved properties for a user-defined routine if the
// routine has already been resolved.
if semaCtx != nil && semaCtx.Properties.RoutineUseResolvedType &&
expr.typ != nil && !expr.typ.IsWildcardType() {
// Don't overwrite the resolved properties for a routine if the routine has
// already been resolved (and we are in a context that needs this behavior).
return expr, nil
}