Re: review: plpgsql return a row-expression

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
>