sql: support output parameters in procedures #120851
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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. |
|
This change is |
yuzefovich
force-pushed
the
out-params
branch
4 times, most recently
from
22e9ba3
to
6ccbd99
Compare
yuzefovich
requested review from
michae2,
mgartner and
DrewKimball
and removed request for
a team and
michae2
|
This is RFAL. First and third commit are exactly as from one approved revision of #120029 (so they deserve little scrutiny) - I extracted all the new changes into the second commit (that I will squash into first one before merging). I also have some further changes to match the behavior of Postgres on DROP PROCEDURE, but it'll be in a separate PR. |
There was a problem hiding this comment.
Reviewed 21 of 21 files at r1, 24 of 24 files at r4, 19 of 19 files at r5, 8 of 8 files at r6, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)
pkg/sql/opt/optbuilder/routine.go line 146 at r5 (raw file):
pgerror.Newf( pgcode.WrongObjectType, "%s(%s) is not a procedure", def.Name, strings.Join(typeNames, ", "),
Nice catch!
pkg/sql/sem/tree/type_check.go line 1228 at r5 (raw file):
} else { s2.release() }
[nit] Since this is going to error anyway, could we simplify the release situation if we threw the _ is not a procedure error here? (or unknown signature if no UDF was found).
pkg/sql/sem/tree/type_check.go line 3475 at r5 (raw file):
) (QualifiedOverload, error) { ambiguousError := func() error { if ambiguousErrorOverride != nil {
[nit] can we just check expr.InCall here and throw the correct error?
There was a problem hiding this comment.
TFTR!
Will wait for green CI, then squash 2nd commit into 1st and will merge.
Reviewable status:
pkg/sql/sem/tree/type_check.go line 1228 at r5 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit] Since this is going to error anyway, could we simplify the
releasesituation if we threw the_ is not a procedureerror here? (orunknown signatureif no UDF was found).
Done.
pkg/sql/sem/tree/type_check.go line 3475 at r5 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit] can we just check
expr.InCallhere and throw the correct error?
Done.
There was a problem hiding this comment.
What a doozy! Thanks for being so thorough! And thanks to Drew for a thorough review of this.
Reviewed 21 of 21 files at r1, 24 of 24 files at r4, 14 of 19 files at r5, 10 of 10 files at r7, 8 of 8 files at r8, all commit messages.
Reviewable status:
This commit finalizes the overload resolution for procedures. The main difference between procedures and UDFs is that for procedures all parameters need to be specified in the CALL invocation, including OUT parameters. Thus, the original idea for implementing the overload resolution (which was already partially achieved in the code) was to include all parameters into the "signature" (unlike for UDFs where only input parameters are included). This turned out to be tricky to implement because in some contexts (e.g. in DROP) OUT parameters need to be ignored. This commit rips out this signature customization for procedures to only include the input parameters. However, in order to ignore the OUT parameters during the type checking (to match the correct overloads) this required additional plumbing of OUT parameters (their ordinals among all parameters as well as types). We need to include this information into the FunctionSignature proto which is done in backwards-compatible way, and the necessary type-checking code that looks at the types of the parameters has been adjusted accordingly. Furthermore, we include the OUT parameter information for UDFs too. Type-checking logic for CALL expressions has been extended so that if it doesn't find the procedure to invoke, it then attempts to treat UDFs as procedures so that a more useful error message is returned (i.e. instead of "procedure does not exist" we now will get "is not a procedure" - matching postgres). Expressions passed to CALL for OUT parameters are not evaluated but must be of coercible to the type of OUT parameters, so these expressions still go through the type-checking but are then not passed during the evaluation. An additional quirk of procedures when comparing to UDFs is that if any output parameters are specified, they define a RECORD return type (even with a single column). This required some adjustments to the existing logic. Note that this commit doesn't have any tests that actually call the procedures, because that logic is implemented in the following commit (previously, since we didn't support output parameters, procedures always returned VOID / NULL). Release note: None
This commit teaches the execution to return a single row from procedures with output parameters. Whenever a procedure has any output parameters, it's expected to return a single tuple elements from which form the result row of the procedure call. Additionally, this commit adjusts the logic to skip evaluation of OUT argument expressions (to match postgres) and ignore them from passing into the udf call. It also extends the logic test framework a bit to add a mode that asserts that the output of a query is empty (procedures with no output parameters don't return any rows). Release note (sql change): OUT and INOUT parameter classes are now supported in stored procedures.
|
TFTRs! bors r+ |
See each commit for details.
Addresses: #100405.
Epic: CRDB-30611