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

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