Re: proposal: fix corner use case of variadic fuctions usage

Hello

I try to recapitulate our design of variadic functions (sorry for my
English - really I have plan to learn this language better - missing
time).

We have two kinds of VARG functions: VARIADIC "any" and VARIADIC
anyarray. These kinds are really different although it is transparent
for final user.

Our low level design supports variadic functions - it based on
FunctionCallInfoData structure. But SQL functions are designed as
nonvariadic overloaded functions - described by pg_proc tuple and
later FmgrInfo structure. This structure contains fn_nargs and fn_expr
(pointer to related parser node) and others, but fn_nargs and fn_epxr
are important for this moment. When we would to get parameter of any
function parameter, we have to use a get_fn_expr_argtype, that is
based on some magic over parser nodes. This expect static number of
parameters of any function during query execution - FmgrInfo is
significantly reused.

So variadic SQL function break some old expectation - but only
partially - with using some trick we are able use variadic function
inside SQL space without significant changes.

1) CALL of variadic functions are transformed to CALL of function with
fixed number of parameters - variadic parameters are transformed to
array
2) if you call variadic function in non variadic manner, then its
calling function with fixed number of parameters and then there
transformation are not necessary.

Points @1 and @2 are valid for VARIADIC anyarray functions.

We introduced VARIADIC "any" function. Motivation for this kind of
function was bypassing postgresql's coerce rules - and own rules
implementation for requested functionality. Some builtins function
does it internally - but it is not possible for custom functions or it
is not possible without some change in parser. Same motivation is
reason why "format" function is VARIADIC "any" function.

Internal implementation of this function is very simple - just enhance
internal checks for mutable number of parameters - and doesn't do
anything more - parameters are passed with FunctionCallInfoData,
necessary expression nodes are created natively. There is important
fact - number of parameters are immutable per one usage - so we can
reuse FmgrInfo. Current limit of VARIADIC "any" function is support
for call in non variadic manner - originally I didn't propose call in
non variadic manner and later we missed support for this use case.
What is worse, we don't handle this use case corectly now - call in
non variadic manner is quietly and buggy forwarded to variadic call
(keyword VARIADIC is ignored)

so we can

SELECT myleast(10,20,40);
SELECT myleast(VARIADIC array[10,20,40]);

SELECT format("%d %d", 10, 20);
but SELECT format("%d %d", VARIADIC array[10, 20]) returns wrong
result because it is not implemented

I proposed more solutions

a) disabling nob variadic call for VARIADIC "any" function and using
overloading as solution for this use case. It was rejected - it has
some issues due possible signature ambiguity.

b) enhancing FunctionCallInfoData about manner of call - variadic, or
non variadic. It was rejected - it needs fixing of all current
VARIADIC "any" functions and probably duplicate some work that can be
handled generic routine.

I don't defend these proposals too much - a issues are clear.

c) enhancing executor, so it can transform non variadic call to
variadic call - just quietly unpack array with parameters and stores
values to FunctionCallInfoData structure. There is new issue - we
cannot reuse parser's nodes and fn_expr and FmgrInfo structure -
because with this transformation these data are mutable.

some example

CREATE TABLE test(format_str text, params text[]);
INSERT INTO test VALUES('%s', ARRAY['Hello']);
INSERT INTO test VALUES('%s %s', ARRAY['Hello','World']);

SELECT format(format_str, VARIADIC params) FROM test;

now I have to support two instances of function - format('%s',
'Hello') and format('%s %s', 'Hello','World') with two different
FmgrInfo - mainly with two different fake parser nodes.

any call needs different FmgrInfo - and I am creating it in short
context due safe design - any forgotten pointer to this mutable
FmgrInfo or more precisely flinfo->fn_expr returns cleaned memory
(with enabled asserts) .

2013/1/18 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> Now I fixed these issues and I hope so it will work on all platforms
>
> As mentioned on the commitfest application, this needs documentation.
> That is not the responsibility of the committer; if you need help, then
> please ask for it.
>
> I've also done a quick review of it.
>
> The massive if() block being added to execQual.c:ExecMakeFunctionResult
> really looks like it might be better pulled out into a function of its
> own.

good idea

>
> What does "use element_type depends for dimensions" mean and why is it
> a ToDo? When will it be done?

I had to thinking some time :( - forgotten note - should be removed

Actually it should to support multidimensional array as parameter's
array - and it does one dimensional slicing - so passing arrays to
variadic "any" function in non variadic manner is possible.

-- multidimensional array is supported
select format('%s, %s', variadic array[['Nazdar','Svete'],['Hello','World']]);
format
-------------------------------
{Nazdar,Svete}, {Hello,World}
(1 row)

>
> Overall, the comments really need to be better. Things like this:
>
> + /* create short lived copies of fmgr data */
> + fmgr_info_copy(&sfinfo, fcinfo->flinfo, fcinfo->flinfo->fn_mcxt);
> + memcpy(scinfo, fcinfo, sizeof(FunctionCallInfoData));
> + scinfo->flinfo = &sfinfo;
>
>
> Really don't cut it, imv. *Why* are we creating a copy of the fmgr
> data? Why does it need to be short lived? And what is going to happen
> later when you do this?:
>

pls, see my introduction - in this case FmgrInfo is not immutable (in
this use case), so it is reason for short living copy and then I using
this copied structure instead repeated modification original
structure.

> fcinfo = scinfo;
> MemoryContextSwitchTo(oldContext);
>
> Is there any reason to worry about the fact that fcache->fcinfo_data no
> longer matches the fcinfo that we're working on? What if there are
> other references made to it in the future? These memcpy's and pointer
> foolishness really don't strike me as being a well thought out approach.
>

fcinfo_data is used for some purposes now - and it can be accessed
from function. Changes that I do are transparent for variadic function
- so I cannot use this.

> There are other similar comments throughout which really need to be
> rewritten to address why we're doing something, not what is being done-
> you can read the code for that.

Thanks for review

Regards

Pavel

>
> Marking this Waiting on Author.
>
> Thanks,
>
> Stephen