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.steh...@gmail.com>wrote: > related to > http://archives.postgresql.org/message-id/caauglxwpedfwae6dajmf7sxewfusa0f68p07retbbpf_fsa...@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 >
return_rowtype.2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
