Modernizing SQL functions' result type coercions

I got interested in $SUBJECT as a result of the complaint at [1]
about typmods not being checked/enforced in places where they
reasonably should be. The cause is that executor/functions.c's
check_sql_fn_retval() only worries about types not typmods.
Another thing not to like is that it only supports cases where
the target type is binary-compatible with the source. It's not
great that user-visible semantics depend on an implementation
detail like binary compatibility. (Amusingly, our user docs
ignore this altogether and claim that the types must be identical.)

Hence, the attached patch rearranges things so that we'll allow
any case where the parser's standard coercion logic can find an
assignment-level coercion, including typmod coercion if needed.
In a green field I might've argued for restricting this to
implicit coercions; but since some of the standard binary-compatible
casts are assignment-level, that would risk breaking applications
that work today. It's really safe enough though, just as assignment
coercions are fine in INSERT: there's no possible confusion about
which conversion is appropriate.

This required some adjustments of check_sql_fn_retval's API.
I found that pulling out the determination of the result tupdesc
and making the callers do that was advisable: in most cases, the
caller has more information and can produce a more accurate tupdesc
(eg by calling get_call_result_type not get_func_result_type).
I also pulled out creation of the JunkFilter that functions.c
wants (but none of the other callers do); having it in just one
place seems simpler. A nice side-effect of these changes is that
we can inline SQL functions in some cases where that wasn't
possible before.

This could use review/testing, so I'll add it to the next CF.

regards, tom lane

[1] https://www.postgresql.org/message-id/b2dbfbd0-8f12-4cca-a0b5-9ee3f00385b4%40phystech.edu

On 28/11/2019 00:57, Tom Lane wrote:
> Hence, the attached patch rearranges things so that we'll allow
> any case where the parser's standard coercion logic can find an
> assignment-level coercion, including typmod coercion if needed.
> In a green field I might've argued for restricting this to
> implicit coercions; but since some of the standard binary-compatible
> casts are assignment-level, that would risk breaking applications
> that work today. It's really safe enough though, just as assignment
> coercions are fine in INSERT: there's no possible confusion about
> which conversion is appropriate.

Makes sense. That's a nice usability improvement.

> This required some adjustments of check_sql_fn_retval's API.
> I found that pulling out the determination of the result tupdesc
> and making the callers do that was advisable: in most cases, the
> caller has more information and can produce a more accurate tupdesc
> (eg by calling get_call_result_type not get_func_result_type).
> I also pulled out creation of the JunkFilter that functions.c
> wants (but none of the other callers do); having it in just one
> place seems simpler. A nice side-effect of these changes is that
> we can inline SQL functions in some cases where that wasn't
> possible before.

In init_sql_fcache(), one comment says that the junkfilter is
responsible for injecting NULLs for dropped columns, and a later comment
says that the junk filter gets "rid of any dropped columns". That seems
contradictory; which is it? Or does "get rid of" mean "set to NULL"?

Other than that, looks good to me.

- Heikki

Heikki Linnakangas <hlinnaka(at)iki(dot)fi> writes:
> In init_sql_fcache(), one comment says that the junkfilter is
> responsible for injecting NULLs for dropped columns, and a later comment
> says that the junk filter gets "rid of any dropped columns". That seems
> contradictory; which is it? Or does "get rid of" mean "set to NULL"?

Yeah, the second comment is sloppily worded; the first one is more
accurate.

> Other than that, looks good to me.

Thanks for reviewing! I'll fix that comment and push.

regards, tom lane