Thread: bugfix - VIP: variadic function ignore strict flag

Hello,

I am returning back to message
http://archives.postgresql.org/pgsql-general/2010-02/msg00119.php

Our implementation of variadic parameters missing correct handling
test on NULL, and test on non constant value.

This patch add correct tests for variadic parameters. Probably
Gen_fmgrtab.pl have needs some work - there are magic constants for
InvalidOid and ANYOID.

Regards

Pavel Stehule

Pavel Stehule <pavel.stehule@gmail.com> writes:
> +         /*
> +          * If function has variadic argument, skip calling
> +          * when variadic array contains NULL values.
> +          */

I don't think this is right at all.  The strict check ought to apply to
the array argument as a whole.
        regards, tom lane

2010/2/9 Tom Lane <tgl@sss.pgh.pa.us>:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> +             /*
>> +              * If function has variadic argument, skip calling
>> +              * when variadic array contains NULL values.
>> +              */
>
> I don't think this is right at all.  The strict check ought to apply to
> the array argument as a whole.

yes, this isn't clear. My arguments for change:

a) the behave depends on types - "any" is different than others.

b) optimization over fmgr doesn't work now.

b1. some possible const null and strict are ignored
b2. array is non const always - so pre eval doesn't work for variadic

c) it could be confusing, and it is partially confusing.

point c could be solved by notice in documentation. But a and b are
problem. The variadic funcall cannot be optimized :(

Regards
Pavel Stehule


>
>                        regards, tom lane
>

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2010/2/9 Tom Lane <tgl@sss.pgh.pa.us>:
>> I don't think this is right at all.

> yes, this isn't clear. My arguments for change:

> a) the behave depends on types - "any" is different than others.

So what?  "variadic any" is different in a lot of ways.

> b) optimization over fmgr doesn't work now.
> b1. some possible const null and strict are ignored

That's a matter of definition.

> b2. array is non const always - so pre eval doesn't work for variadic

You'd need to explain what you mean by that.  An ARRAY[] construct is
subject to const-folding AFAICS.
        regards, tom lane

2010/2/9 Tom Lane <tgl@sss.pgh.pa.us>:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> 2010/2/9 Tom Lane <tgl@sss.pgh.pa.us>:
>>> I don't think this is right at all.
>
>> yes, this isn't clear. My arguments for change:
>
>> a) the behave depends on types - "any" is different than others.
>
> So what?  "variadic any" is different in a lot of ways.
>

implementation is different, but from users perspective there can not
be differences. I am not sure. From my programmer's view is all ok.
But I believe so from customer view, there can be a surprise - because
NULL value doesn't skip function call.

>> b) optimization over fmgr doesn't work now.
>> b1. some possible const null and strict are ignored
>
> That's a matter of definition.
>
>> b2. array is non const always - so pre eval doesn't work for variadic
>
> You'd need to explain what you mean by that.  An ARRAY[] construct is
> subject to const-folding AFAICS.

I am sorry. I was confused. This optimization will work. Only NULL is problem.

Regards
Pavel


>
>                        regards, tom lane
>

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2010/2/9 Tom Lane <tgl@sss.pgh.pa.us>:
>> So what?  "variadic any" is different in a lot of ways.

> implementation is different, but from users perspective there can not
> be differences. I am not sure. From my programmer's view is all ok.
> But I believe so from customer view, there can be a surprise - because
> NULL value doesn't skip function call.

It's going to be a bit surprising in any case.  If I write
foo(1, VARIADIC ARRAY[2, NULL])

then what I'm passing is not a null, and so I'd be surprised if the
function wasn't executed.

I think we should just document this, not make a definitional change
that seems as likely to break applications as fix them.
        regards, tom lane

2010/2/9 Tom Lane <tgl@sss.pgh.pa.us>:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> 2010/2/9 Tom Lane <tgl@sss.pgh.pa.us>:
>>> So what?  "variadic any" is different in a lot of ways.
>
>> implementation is different, but from users perspective there can not
>> be differences. I am not sure. From my programmer's view is all ok.
>> But I believe so from customer view, there can be a surprise - because
>> NULL value doesn't skip function call.
>
> It's going to be a bit surprising in any case.  If I write
>
>        foo(1, VARIADIC ARRAY[2, NULL])
>
> then what I'm passing is not a null, and so I'd be surprised if the
> function wasn't executed.

>
> I think we should just document this, not make a definitional change
> that seems as likely to break applications as fix them.

really I am not sure, what is good solution. Maybe can speak some other.

Pavel

>
>                        regards, tom lane
>