Re: why can't plpgsql return a row-expression?

2012/12/4 Asif Rehman <asifr.rehman@gmail.com>:
> Hi,
>
> Here is the updated patch. I overlooked the loop, checking to free the
> conversions map. Here are the results now.
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral foo(i)
> as (a numeric, b numeric);
>    sum
> ----------
>  303000.0
> (1 row)
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral foo(i)
> as (a float, b numeric);
>        sum
> ------------------
>  303000.000000012
>
> Regards,
> --Asif

yes, it is fixed.

ok I have no objection

Regards

Pavel Stehule

>
>
> On Wed, Dec 5, 2012 at 12:40 AM, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>>
>> Hello
>>
>> I fully agree with Asif's arguments
>>
>> previous tupconvert implementation was really strict - so using
>> enhanced tupconver has very minimal impact for old user - and I
>> checked same speed for plpgsql function - patched and unpatched pg.
>>
>> tested
>>
>> CREATE OR REPLACE FUNCTION public.foo(i integer)
>>  RETURNS SETOF record
>>  LANGUAGE plpgsql
>> AS $function$
>> declare r record;
>> begin
>>   r := (10.1,20.1); for i in 1..10 loop return next r; end loop; return;
>> end;
>> $function$
>>
>> select sum(a) from generate_series(1,3000) g(i), lateral foo(i) as (a
>> numeric, b numeric);
>>
>> More - everywhere where we use tupconvert internally, then we use
>> cached conversion map - so I am sure, so speed of ANALYZE cannot be
>> impacted by this patch
>>
>> There are other two issue:
>>
>> it allow to write new differnt slow code - IO cast is about 5-10-20%
>> slower, and with this path anybody has new possibilities for new "bad"
>> code. But it is not a problem of this patch. It is related to plpgsql
>> design and probably we should to move some conversions to outside
>> plpgsql to be possible reuse conversion map and enhance plpgsql. I
>> have a simple half solutions - plpgsql_check_function can detect this
>> situation in almost typical use cases and can raises a performance
>> warning. So this issue is not stopper for me - because it is not new
>> issue in plpgsql.
>>
>> Second issue is more significant:
>>
>> there are bug:
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> foo(i) as (a numeric, b numeric);
>>    sum
>> ----------
>>  303000.0
>> (1 row)
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> foo(i) as (a float, b numeric);
>>           sum
>> -----------------------
>>  7.33675699577682e-232
>> (1 row)
>>
>> it produces wrong result
>>
>> And with minimal change it kill session
>>
>> create or replace function foo(i integer)
>> returns setof record as $$
>> declare r record;
>> begin
>>   r := (10,20); for i in 1..10 loop return next r; end loop; return;
>> end;
>> $$ language plpgsql;
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> foo(i) as (a numeric, b numeric);
>> The connection to the server was lost. Attempting reset: Failed.
>>
>> create or replace function fo(i integer)
>> returns record as $$
>> declare r record;
>> begin
>>   r := (10,20); return r;
>> end;
>> $$ language plpgsql;
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> fo(i) as (a int, b numeric);
>>   sum
>> -------
>>  30000
>> (1 row)
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> fo(i) as (a numeric, b numeric);
>> The connection to the server was lost. Attempting reset: Failed.
>>
>> Regards
>>
>> Pavel Stehule
>>
>>
>> 2012/12/3 Asif Rehman <asifr.rehman@gmail.com>:
>> > Hi,
>> >
>> > Thanks for the review. Please see the updated patch.
>> >
>> >> Hmm.  We're running an I/O cast during do_tup_convert() now, and look
>> >> up the required functions for each tuple.  I think if we're going to
>> >> support this operation at that level, we need to look up the necessary
>> >> functions at convert_tuples_by_foo level, and then apply
>> >> unconditionally
>> >> if they've been set up.
>> >
>> > Done. TupleConversionMap struct should keep the array of functions oid's
>> > that
>> > needs to be applied. Though only for those cases where both attribute
>> > type
>> > id's
>> > do not match. This way no unnecessary casting will happen.
>> >
>> >>
>> >> Also, what are the implicancies for existing users of tupconvert?  Do
>> >> we
>> >> want to apply casting during ANALYZE for example?  AFAICS the patch
>> >> shouldn't break any case that works today, but I don't see that there
>> >> has been any analysis of this.
>> >
>> > I believe this part of the code should not impact existing users of
>> > tupconvert.
>> > As this part of code is relaxing a restriction upon which an error would
>> > have been
>> > generated. Though it could have a little impact on performance but
>> > should be
>> > only for
>> > cases where attribute type id's are different and are implicitly cast
>> > able.
>> >
>> > Besides existing users involve tupconvert in case of inheritance. And in
>> > that case
>> > an exact match is expected.
>> >
>> > Regards,
>> > --Asif
>
>