Thread: review: plpgsql return a row-expression

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,
sosafe to
 
do this: */                               td = DatumGetHeapTupleHeader(retval);                               /*
Extractrowtype 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

Hello

2012/11/23 Asif Rehman <asifr.rehman@gmail.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.rehman@gmail.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.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
>>
>>
>