Re: review: plpgsql return a row-expression - Mailing list pgsql-hackers

Hello

2012/11/23 Asif Rehman <asifr.rehman@gmail.com>:
> Hi,
>
> I forgot to add documentation changes in the earlier patch. In the attached
> patch, I have added documentation as well as fixed other issues you raised.
>

ok

so

* applied and compiled cleanly
* All 133 tests passed.
* there are doc
* there are necessary regress tests
* automatic conversion is works like plpgsql user expects
* there are no performance issue now
* code respects our coding standards
+ code remove redundant lines

I have no any objection

this patch is ready to commit!

Regards

Pavel




> Regards,
> --Asif
>
>
> On Thu, Nov 22, 2012 at 10:47 PM, Asif Rehman <asifr.rehman@gmail.com>
> wrote:
>>
>> Thanks for the review Pavel. I have taken care of the points you raised.
>> Please see the updated patch.
>>
>> Regards,
>> --Asif
>>
>>
>> On Wed, Nov 21, 2012 at 1:47 AM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>>>
>>> related to
>>> http://archives.postgresql.org/message-id/CAAuGLxWpEDfwAE6DAJMF7SxEwFUsA0f68P07RetBbpf_FSaShA@mail.gmail.com
>>>
>>> * patched and compiled withou warnings
>>>
>>> * All 133 tests passed.
>>>
>>>
>>> but
>>>
>>> I don't like
>>>
>>> * call invalid function from anonymous block - it is messy (regress
>>> tests) - there is no reason why do it
>>>
>>> +create or replace function foo() returns footype as $$
>>> +declare
>>> +  v record;
>>> +  v2 record;
>>> +begin
>>> +  v := (1, 'hello');
>>> +  v2 := (1, 'hello');
>>> +  return (v || v2);
>>> +end;
>>> +$$ language plpgsql;
>>> +DO $$
>>> +declare
>>> +  v footype;
>>> +begin
>>> +  v := foo();
>>> +  raise info 'x = %', v.x;
>>> +  raise info 'y = %', v.y;
>>> +end; $$;
>>> +ERROR:  operator does not exist: record || record
>>> +LINE 1: SELECT (v || v2)
>>> +                  ^
>>>
>>> * there is some performance issue
>>>
>>> create or replace function fx2(a int)
>>> returns footype as $$
>>> declare x footype;
>>> begin
>>>   x = (10,20);
>>>   return x;
>>> end;
>>> $$ language plpgsql;
>>>
>>> postgres=# select sum(fx2.x) from generate_series(1,100000) g(i),
>>> lateral fx2(i);
>>>    sum
>>> ---------
>>>  1000000
>>> (1 row)
>>>
>>> Time: 497.129 ms
>>>
>>> returns footype as $$
>>> begin
>>>   return (10,20);
>>> end;
>>> $$ language plpgsql;
>>>
>>> postgres=# select sum(fx2.x) from generate_series(1,100000) g(i),
>>> lateral fx2(i);
>>>    sum
>>> ---------
>>>  1000000
>>> (1 row)
>>>
>>> Time: 941.192 ms
>>>
>>> following code has same functionality and it is faster
>>>
>>>         if (stmt->expr != NULL)
>>>         {
>>>                 if (estate->retistuple)
>>>                 {
>>>                         TupleDesc       tupdesc;
>>>                         Datum           retval;
>>>                         Oid             rettype;
>>>                         bool            isnull;
>>>                         int32           tupTypmod;
>>>                         Oid             tupType;
>>>                         HeapTupleHeader         td;
>>>                         HeapTupleData   tmptup;
>>>
>>>                         retval = exec_eval_expr(estate,
>>>
>>> stmt->expr,
>>>                                                                 &isnull,
>>>
>>> &rettype);
>>>
>>>                         /* Source must be of RECORD or composite type */
>>>                         if (!type_is_rowtype(rettype))
>>>                                 ereport(ERROR,
>>>
>>> (errcode(ERRCODE_DATATYPE_MISMATCH),
>>>                                                  errmsg("cannot return
>>> non-composite value from composite type returning function")));
>>>
>>>                         if (!isnull)
>>>                         {
>>>                                 /* Source is a tuple Datum, so safe to
>>> do this: */
>>>                                 td = DatumGetHeapTupleHeader(retval);
>>>                                 /* Extract rowtype info and find a
>>> tupdesc */
>>>                                 tupType = HeapTupleHeaderGetTypeId(td);
>>>                                 tupTypmod = HeapTupleHeaderGetTypMod(td);
>>>                                 tupdesc =
>>> lookup_rowtype_tupdesc(tupType, tupTypmod);
>>>
>>>                                 /* Build a temporary HeapTuple control
>>> structure */
>>>                                 tmptup.t_len =
>>> HeapTupleHeaderGetDatumLength(td);
>>>                                 ItemPointerSetInvalid(&(tmptup.t_self));
>>>                                 tmptup.t_tableOid = InvalidOid;
>>>                                 tmptup.t_data = td;
>>>
>>>                                 estate->retval =
>>> PointerGetDatum(heap_copytuple(&tmptup));
>>>                                 estate->rettupdesc =
>>> CreateTupleDescCopy(tupdesc);
>>>                                 ReleaseTupleDesc(tupdesc);
>>>                         }
>>>
>>>                         estate->retisnull = isnull;
>>>                 }
>>>
>>>
>>>
>>> * it is to restrictive (maybe) - almost all plpgsql' statements does
>>> automatic conversions (IO conversions when is necessary)
>>>
>>> create type footype2 as (a numeric, b varchar)
>>>
>>> postgres=# create or replace function fx3(a int)
>>> returns footype2 as
>>> $$
>>> begin
>>>   return (10000000.22234213412342143,'ewqerwqreeeee');
>>> end;
>>> $$ language plpgsql;
>>> CREATE FUNCTION
>>> postgres=# select fx3(10);
>>> ERROR:  returned record type does not match expected record type
>>> DETAIL:  Returned type unknown does not match expected type character
>>> varying in column 2.
>>> CONTEXT:  PL/pgSQL function fx3(integer) while casting return value to
>>> function's return type
>>> postgres=#
>>>
>>> * it doesn't support RETURN NEXT
>>>
>>> postgres=# create or replace function fx4()
>>> postgres-# returns setof footype as $$
>>> postgres$# begin
>>> postgres$#   for i in 1..10
>>> postgres$#   loop
>>> postgres$#     return next (10,20);
>>> postgres$#   end loop;
>>> postgres$#   return;
>>> postgres$# end;
>>> postgres$# $$ language plpgsql;
>>> ERROR:  RETURN NEXT must specify a record or row variable in function
>>> returning row
>>> LINE 6:     return next (10,20);
>>>
>>> * missing any documentation
>>>
>>> * repeated code - following code is used on more places in pl_exec.c
>>> and maybe it is candidate for subroutine
>>>
>>> +                                                       /* Source is a
>>> tuple Datum, so safe to do this: */
>>> +                                                       td =
>>> DatumGetHeapTupleHeader(value);
>>> +                                                       /* Extract
>>> rowtype info and find a tupdesc */
>>> +                                                       tupType =
>>> HeapTupleHeaderGetTypeId(td);
>>> +                                                       tupTypmod =
>>> HeapTupleHeaderGetTypMod(td);
>>> +                                                       tupdesc =
>>> lookup_rowtype_tupdesc_copy(tupType, tupTypmod);
>>> +                                                       /* Build a
>>> HeapTuple control structure */
>>> +                                                       htup.t_len =
>>> HeapTupleHeaderGetDatumLength(td);
>>> +
>>> ItemPointerSetInvalid(&(htup.t_self));
>>> +                                                       htup.t_tableOid =
>>> InvalidOid;
>>> +                                                       htup.t_data = td;
>>> +                                                       tuple =
>>> heap_copytuple(&htup);
>>>
>>> Regards
>>>
>>> Pavel Stehule
>>
>>
>