Re: review: plpgsql return a row-expression

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.

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
>>
>
>