sql: handle and test routines with polymorphic parameters and return type

[DrewKimball]

sql: handle and test routines with polymorphic parameters and return type

This commit adds overload resolution and type-checking logic for UDFs
and stored procedures that have polymorphic-typed parameters and/or
a polymorphic return type. Currently, this only applies to ANYELEMENT
and ANYARRAY.

Fixes #123239

Release note (bug fix): Fixed a bug that could cause internal errors
when a routine had polymorphic parameters and/or a polymorphic return
type. The bug has existed since v22.2 when UDFs were introduced.

sql: add session var to gate polymorphic parameter fix

This commit adds a session variable, optimizer_use_polymorphic_parameter_fix,
as an escape hatch so that the polymorphic routine parameter fixes from
the previous commit can be disabled.

Informs #123239

Release note: None

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[DrewKimball]

The first commit is from #123457.

[yuzefovich]

I'm still reading through but thought I'd push what I have so far.

I ran pg_regress on this PR - the diff is at yuzefovich@24f25ed.

There are still two stack traces there worth checking out, e.g:

 create function f1(x anyarray) returns anyarray as $$
 begin
   return x;
 end$$ language plpgsql;
 select f1(array[2,4]) as int, f1(array[4.5, 7.7]) as num;

Another difference I noticed is:

  create function dfunc(anyelement = 'World'::text) returns text as $$
    select 'Hello, ' || $1::text;
  $$ language sql;
-+ERROR:  argument of DEFAULT must be type anyelement, not type string
++ERROR:  invalid cast: anyelement -> string

Reviewed 2 of 2 files at r1, 5 of 18 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

---

pkg/sql/opt/optbuilder/create_function.go line 337 at r2 (raw file):

					"A result of type %s requires at least one input of type "+
						"anyelement, anyarray, anynonarray, anyenum, anyrange, or anymultirange.",
					polyTyp.SQLStringForError(),

nit: should we do strings.ToLower(polyTyp.SQLStringForError()) to match postgres? Diff from the pg_regress:

--ERROR:  cannot determine result data type
+ ERROR:  cannot determine result data type
 -DETAIL:  A result of type anyelement requires at least one input of type anyelement, anyarray, anynonarray, anyenum, anyrange, or anymultirange.
++DETAIL:  A result of type ANYELEMENT requires at least one input of type anyelement, anyarray, anynonarray, anyenum, anyrange, or anymultirange.

---

pkg/sql/opt/optbuilder/create_function.go line 343 at r2 (raw file):

				makeErr(funcReturnType)
			} else {
				for i := range funcReturnType.TupleContents() {

nit: could simplify this a bit with

for _, tc := range funcReturnType.TupleContents() {
...

by using tc later.

---

pkg/sql/opt/optbuilder/routine.go line 675 at r2 (raw file):

	// Check whether some arguments were omitted. We need to use the
	// corresponding DEFAULT expressions if so.
	if numDefaultsToUse := o.Types.Length() - len(args); numDefaultsToUse > 0 {

nit: now that this block is inside a function, it'd be cleaner to check if numDefaultToUse is non-positive and short-circuit the function if so.

---

pkg/sql/sem/tree/overload.go line 1066 at r2 (raw file):

			}
		}
		if hasPolymorphicTyp {

nit: maybe negate and short-circuit?

---

pkg/sql/sem/tree/type_check.go line 3632 at r2 (raw file):

//     be at least one non-NULL argument in order to resolve a concrete type.
//
// enforceConsistency, if true, indicates that resolvePolymorphicArgTypes should

nit: do you think it would be cleaner to return (anyElemTyp *types.T, err error), and the caller than decide whether to panic with the error or not? We'll probably need an extra boolean to indicate whether any polymorphic types were found. The current approach seems reasonable too.

nit: s/resolve/Resolve/.

---

pkg/sql/logictest/testdata/logic_test/procedure_polymorphic line 137 at r2 (raw file):

CALL p('hi'::greetings, 'hello'::greetings);

## The supplied arguments for ANYENUM parameters must have the same type, and

nit: probably missing a TODO above this comment.

[DrewKimball]

Thanks for taking a look, and for running pg_regress!

There are still two stack traces there worth checking out

This is now fixed - we were missing the logic to replace polymorphic parameter types when they are passed to PL/pgSQL. This also prompted me to add a commit to throw the correct error when the user attempts to define a polymorphic-typed variable.

Another difference I noticed

Opened an issue here: #123536

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/opt/optbuilder/create_function.go line 337 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we do strings.ToLower(polyTyp.SQLStringForError()) to match postgres? Diff from the pg_regress:

--ERROR:  cannot determine result data type
+ ERROR:  cannot determine result data type
 -DETAIL:  A result of type anyelement requires at least one input of type anyelement, anyarray, anynonarray, anyenum, anyrange, or anymultirange.
++DETAIL:  A result of type ANYELEMENT requires at least one input of type anyelement, anyarray, anynonarray, anyenum, anyrange, or anymultirange.

Good point; I'll use T.Name() like in other places. I also added anyarray to the implementation. I wonder though, if we should be using SQLStandardName instead?

---

pkg/sql/opt/optbuilder/create_function.go line 343 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: could simplify this a bit with

for _, tc := range funcReturnType.TupleContents() {
...

by using tc later.

Done.

---

pkg/sql/opt/optbuilder/routine.go line 675 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: now that this block is inside a function, it'd be cleaner to check if numDefaultToUse is non-positive and short-circuit the function if so.

Done.

---

pkg/sql/sem/tree/overload.go line 1066 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: maybe negate and short-circuit?

Done.

---

pkg/sql/sem/tree/type_check.go line 3632 at r2 (raw file):
My thinking was that we should try and avoid the overhead, since this function could be called often if there's a candidate overload that usually gets filtered out. But that's probably not that important... what do you think?

nit: s/resolve/Resolve/.
Done.

---

pkg/sql/logictest/testdata/logic_test/procedure_polymorphic line 137 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: probably missing a TODO above this comment.

Done.

[yuzefovich]

Very nice work! I have some minor comments.

Reviewed 9 of 18 files at r2, 20 of 20 files at r4, 13 of 13 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

---

-- commits line 16 at r6:
What's your thinking behind introducing this gate? Is it because we'll be backporting it to 24.1.1 version so that we have an escape hatch?

---

pkg/sql/opt/optbuilder/create_function.go line 337 at r2 (raw file):

I wonder though, if we should be using SQLStandardName instead?

Seems like we should.

---

pkg/sql/opt/optbuilder/create_function.go line 321 at r4 (raw file):

		for i := range cf.Params {
			if cf.Params[i].IsInParam() {
				typ, err := tree.ResolveType(b.ctx, cf.Params[i].Type, b.semaCtx.TypeResolver)

nit: we resolve types for all parameters in a loop above. Should we additionally check all input parameters for having polymorphic type there too, unconditionally?

---

pkg/sql/opt/optbuilder/plpgsql.go line 352 at r6 (raw file):

				panic(recordVarErr)
			} else if typ.IsPolymorphicType() {
        // NOTE: Postgres also returns an "unsupported" error.

nit: indentation is off.

---

pkg/sql/opt/optbuilder/plpgsql.go line 354 at r6 (raw file):

        // NOTE: Postgres also returns an "unsupported" error.
				panic(pgerror.Newf(pgcode.FeatureNotSupported,
					"variable \"%s\" has pseudo-type %s", dec.Var, typ.SQLStringForError(),

nit: should we be using typ.SQLStandardName() here?

---

pkg/sql/opt/optbuilder/routine.go line 738 at r4 (raw file):

func (b *Builder) routineHasPolymorphicOutParam(params tree.RoutineParams) bool {
	for i := range params {
		if params[i].IsOutParam() {

nit: this seems like another boolean we can compute when resolving types in the first loop over parameters. I don't feel strongly whether this helper function should be removed though - up to you.

---

pkg/sql/sem/tree/overload.go line 1074 at r4 (raw file):

				_, isOutParam := toParamOrdinal(i, ol.OutParamOrdinals)
				if isOutParam {
					// A CALL statement must specify an argument for each OUT parameter

Related to something you spotted earlier in one of my PRs: IIRC the expression for the OUT parameter must of coercible to the type, is it already handled elsewhere when that type is polymorphic? Do we have a test for a mismatch?

---

pkg/sql/sem/tree/type_check.go line 3632 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

My thinking was that we should try and avoid the overhead, since this function could be called often if there's a candidate overload that usually gets filtered out. But that's probably not that important... what do you think?

nit: s/resolve/Resolve/.
Done.

What overhead are you referring to? The overhead of having to create an error object when ok=false would suffice? Yeah, that's a good point, current approach seems good then. Maybe extend the comment a bit for why we have this enforceConsistency=false option.

---

pkg/sql/sem/tree/type_check.go line 3707 at r4 (raw file):

		// All supplied arguments were NULL.
		//
		// Note: we do not return ok=false when enforceConsistency is false, because

nit: I think we either need to adjust the contract of the method (i.e. extend the comment to spell out what happens in this case) or return ok=false when enforceConsistency=false in order to make misuse of this exported method less error-prone.

---

pkg/sql/logictest/testdata/logic_test/procedure_polymorphic line 404 at r4 (raw file):

# Note: When the proc is called, Postgres returns an error with the message:
# "cannot display a value of type anyelement". This seems like an oversight, so
# the difference is probably ok. See #123454.

Interesting find! Do you wanna file a bug upstream? 😃

[DrewKimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

-- commits line 16 at r6:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

What's your thinking behind introducing this gate? Is it because we'll be backporting it to 24.1.1 version so that we have an escape hatch?

Yes, that's right. It wasn't too hard to do and it decreases risk, so why not?

---

pkg/sql/opt/optbuilder/create_function.go line 337 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I wonder though, if we should be using SQLStandardName instead?

Seems like we should.

Tracking issue: #124087

---

pkg/sql/opt/optbuilder/create_function.go line 321 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: we resolve types for all parameters in a loop above. Should we additionally check all input parameters for having polymorphic type there too, unconditionally?

I've moved this check to the previous loop. Is that what you meant?

---

pkg/sql/opt/optbuilder/plpgsql.go line 352 at r6 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: indentation is off.

Done.

---

pkg/sql/opt/optbuilder/plpgsql.go line 354 at r6 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we be using typ.SQLStandardName() here?

🤦 Yes, good catch. I'll do Name() for now for consistency with other errors; we can track changing to SqlStandardName with #124087.

---

pkg/sql/opt/optbuilder/routine.go line 738 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this seems like another boolean we can compute when resolving types in the first loop over parameters. I don't feel strongly whether this helper function should be removed though - up to you.

Done.

---

pkg/sql/sem/tree/overload.go line 1074 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Related to something you spotted earlier in one of my PRs: IIRC the expression for the OUT parameter must of coercible to the type, is it already handled elsewhere when that type is polymorphic? Do we have a test for a mismatch?

Very good point - we should also be calling ResolvePolymorphicArgTypes with the OUT parameters here. I've now fixed it, and added some tests.

---

pkg/sql/sem/tree/type_check.go line 3632 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

What overhead are you referring to? The overhead of having to create an error object when ok=false would suffice? Yeah, that's a good point, current approach seems good then. Maybe extend the comment a bit for why we have this enforceConsistency=false option.

Yes, exactly. I've added some detail to the comment.

---

pkg/sql/sem/tree/type_check.go line 3707 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think we either need to adjust the contract of the method (i.e. extend the comment to spell out what happens in this case) or return ok=false when enforceConsistency=false in order to make misuse of this exported method less error-prone.

We can't return ok=false, since that case needs to pass the overload resolution step. I'll move this check outside of this function so it's no longer an issue. This also required returning the count of polymorphic parameters, so that we can distinguish between no polymorphic parameters, and the case when only NULL is supplied as the arguments.

---

pkg/sql/logictest/testdata/logic_test/procedure_polymorphic line 404 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Interesting find! Do you wanna file a bug upstream? 😃

Filed one here: https://postgrespro.com/list/id/18463-f8cd77e12564d8a2@postgresql.org#head

[yuzefovich]

Reviewed 16 of 16 files at r7, 11 of 11 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball)

---

pkg/sql/opt/optbuilder/create_function.go line 321 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I've moved this check to the previous loop. Is that what you meant?

Yeah, thanks.

---

pkg/sql/sem/tree/type_check.go line 3638 at r7 (raw file):

// throw a suitable error in the case of invalid arguments, rather than
// returning with ok=false. Note: we do this instead of always returning the
// error because error construction can be expensive.a

nit: "expensive.a".

[DrewKimball]

TFYR!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball)

---

pkg/sql/sem/tree/type_check.go line 3638 at r7 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: "expensive.a".

Done.