Re: review: plpgsql return a row-expression

Hello

2012/11/23 Asif Rehman <asifr(dot)rehman(at)gmail(dot)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(dot)rehman(at)gmail(dot)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(dot)stehule(at)gmail(dot)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
>>
>>
>