Horton
Menu

Thread

  1. why can't plpgsql return a row-expression?

    Robert Haas <robertmhaas@gmail.com> — 2012-10-08T14:42:27Z

    Hi,

PL/pgsql seems to have a strange restriction regarding "RETURN".
Normally, you can write "RETURN <expression>".  But if the function
returns a row type, then you can only write "RETURN variable" or
"RETURN NULL".

rhaas=# create type xyz as (a int, b int);
CREATE TYPE
rhaas=# select row(1,2)::xyz;
  row
-------
 (1,2)
(1 row)

rhaas=# create or replace function return_xyz() returns xyz as $$
rhaas$# begin
rhaas$#     return row(1,2)::xyz;
rhaas$# end$$ language plpgsql;
ERROR:  RETURN must specify a record or row variable in function returning row
LINE 3:     return row(1,2)::xyz;
                   ^

Off the top of my head, I can't think of any reason for this
restriction, nor can I find any code comments or anything in the
commit log which explains the reason for it.  Does anyone know why we
don't allow this?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  2. Re: why can't plpgsql return a row-expression?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-08T15:09:22Z

    Robert Haas <robertmhaas@gmail.com> writes:
> ERROR:  RETURN must specify a record or row variable in function returning row

> Off the top of my head, I can't think of any reason for this
> restriction, nor can I find any code comments or anything in the
> commit log which explains the reason for it.  Does anyone know why we
> don't allow this?

Laziness, probably.  Feel free to have at it.

			regards, tom lane

  3. Re: why can't plpgsql return a row-expression?

    Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-08T16:21:48Z

    2012/10/8 Tom Lane <tgl@sss.pgh.pa.us>:
> Robert Haas <robertmhaas@gmail.com> writes:
>> ERROR:  RETURN must specify a record or row variable in function returning row
>
>> Off the top of my head, I can't think of any reason for this
>> restriction, nor can I find any code comments or anything in the
>> commit log which explains the reason for it.  Does anyone know why we
>> don't allow this?
>
> Laziness, probably.  Feel free to have at it.

I wrote patch some years ago. It was rejected from performance reasons
- because every row had to be casted to resulted type.

Pavel



>
>                         regards, tom lane
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

  4. Re: why can't plpgsql return a row-expression?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-08T16:53:10Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2012/10/8 Tom Lane <tgl@sss.pgh.pa.us>:
>> Laziness, probably.  Feel free to have at it.

> I wrote patch some years ago. It was rejected from performance reasons
> - because every row had to be casted to resulted type.

I don't recall that patch in any detail, but it's not apparent to me
that an extra cast step *must* be required to implement this.  In the
cases that are supported now, surely we could notice that no additional
work is required.

It's also worth commenting that if we were to switch the storage of
composite-type plpgsql variables to HeapTuple, as has been suggested
here for other reasons, the performance tradeoffs in this area would
likely change completely anyway.

			regards, tom lane

  5. Re: why can't plpgsql return a row-expression?

    Asif Rehman <asifr.rehman@gmail.com> — 2012-11-12T13:50:49Z

    Hi,

I have tried to solve this issue. Please see the attached patch.

With this patch, any expression is allowed in the return statement. For any
invalid expression an error is generated without doing any special handling.
When a row expression is used in the return statement then the resultant
tuple will have rowtype in a single column that needed to be extracted.
Hence I have handled that case in exec_stmt_return().

any comments/suggestions?

Regards,
--Asif

On Mon, Oct 8, 2012 at 9:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > 2012/10/8 Tom Lane <tgl@sss.pgh.pa.us>:
> >> Laziness, probably.  Feel free to have at it.
>
> > I wrote patch some years ago. It was rejected from performance reasons
> > - because every row had to be casted to resulted type.
>
> I don't recall that patch in any detail, but it's not apparent to me
> that an extra cast step *must* be required to implement this.  In the
> cases that are supported now, surely we could notice that no additional
> work is required.
>
> It's also worth commenting that if we were to switch the storage of
> composite-type plpgsql variables to HeapTuple, as has been suggested
> here for other reasons, the performance tradeoffs in this area would
> likely change completely anyway.
>
>                         regards, tom lane
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

  6. review: plpgsql return a row-expression

    Pavel Stehule <pavel.stehule@gmail.com> — 2012-11-20T20:47:56Z

    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

  7. Re: review: plpgsql return a row-expression

    Asif Rehman <asifr.rehman@gmail.com> — 2012-11-22T17:47:19Z

    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
>

  8. Re: review: plpgsql return a row-expression

    Asif Rehman <asifr.rehman@gmail.com> — 2012-11-23T08:04:39Z

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

  9. Re: review: plpgsql return a row-expression

    Pavel Stehule <pavel.stehule@gmail.com> — 2012-11-23T10:55:02Z

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

  10. Re: why can't plpgsql return a row-expression?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-27T20:55:41Z

    Asif Rehman escribió:
> Hi,
> 
> I have tried to solve this issue. Please see the attached patch.
> 
> With this patch, any expression is allowed in the return statement. For any
> invalid expression an error is generated without doing any special handling.
> When a row expression is used in the return statement then the resultant
> tuple will have rowtype in a single column that needed to be extracted.
> Hence I have handled that case in exec_stmt_return().
> 
> any comments/suggestions?

Hmm.  We're running an I/O cast during do_tup_convert() now, and look
up the required functions for each tuple.  I think if we're going to
support this operation at that level, we need to look up the necessary
functions at convert_tuples_by_foo level, and then apply unconditionally
if they've been set up.

Also, what are the implicancies for existing users of tupconvert?  Do we
want to apply casting during ANALYZE for example?  AFAICS the patch
shouldn't break any case that works today, but I don't see that there
has been any analysis of this.

(I looked at the patch posted in the thread started by Pavel elsewhere.
I'm replying to both emails in the interest of keeping things properly
linked.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

  11. Re: why can't plpgsql return a row-expression?

    Asif Rehman <asifr.rehman@gmail.com> — 2012-12-03T19:15:27Z

    Hi,

Thanks for the review. Please see the updated patch.

Hmm.  We're running an I/O cast during do_tup_convert() now, and look
> up the required functions for each tuple.  I think if we're going to
> support this operation at that level, we need to look up the necessary
> functions at convert_tuples_by_foo level, and then apply unconditionally
> if they've been set up.
>
Done. TupleConversionMap struct should keep the array of functions oid's
that
needs to be applied. Though only for those cases where both attribute type
id's
do not match. This way no unnecessary casting will happen.


> Also, what are the implicancies for existing users of tupconvert?  Do we
> want to apply casting during ANALYZE for example?  AFAICS the patch
> shouldn't break any case that works today, but I don't see that there
> has been any analysis of this.
>
I believe this part of the code should not impact existing users of
tupconvert.
As this part of code is relaxing a restriction upon which an error would
have been
generated. Though it could have a little impact on performance but should
be only for
cases where attribute type id's are different and are implicitly cast able.

Besides existing users involve tupconvert in case of inheritance. And in
that case
an exact match is expected.

Regards,
--Asif

  12. Re: why can't plpgsql return a row-expression?

    Pavel Stehule <pavel.stehule@gmail.com> — 2012-12-04T19:40:49Z

    Hello

I fully agree with Asif's arguments

previous tupconvert implementation was really strict - so using
enhanced tupconver has very minimal impact for old user - and I
checked same speed for plpgsql function - patched and unpatched pg.

tested

CREATE OR REPLACE FUNCTION public.foo(i integer)
 RETURNS SETOF record
 LANGUAGE plpgsql
AS $function$
declare r record;
begin
  r := (10.1,20.1); for i in 1..10 loop return next r; end loop; return;
end;
$function$

select sum(a) from generate_series(1,3000) g(i), lateral foo(i) as (a
numeric, b numeric);

More - everywhere where we use tupconvert internally, then we use
cached conversion map - so I am sure, so speed of ANALYZE cannot be
impacted by this patch

There are other two issue:

it allow to write new differnt slow code - IO cast is about 5-10-20%
slower, and with this path anybody has new possibilities for new "bad"
code. But it is not a problem of this patch. It is related to plpgsql
design and probably we should to move some conversions to outside
plpgsql to be possible reuse conversion map and enhance plpgsql. I
have a simple half solutions - plpgsql_check_function can detect this
situation in almost typical use cases and can raises a performance
warning. So this issue is not stopper for me - because it is not new
issue in plpgsql.

Second issue is more significant:

there are bug:

postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
foo(i) as (a numeric, b numeric);
   sum
----------
 303000.0
(1 row)

postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
foo(i) as (a float, b numeric);
          sum
-----------------------
 7.33675699577682e-232
(1 row)

it produces wrong result

And with minimal change it kill session

create or replace function foo(i integer)
returns setof record as $$
declare r record;
begin
  r := (10,20); for i in 1..10 loop return next r; end loop; return;
end;
$$ language plpgsql;

postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
foo(i) as (a numeric, b numeric);
The connection to the server was lost. Attempting reset: Failed.

create or replace function fo(i integer)
returns record as $$
declare r record;
begin
  r := (10,20); return r;
end;
$$ language plpgsql;

postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
fo(i) as (a int, b numeric);
  sum
-------
 30000
(1 row)

postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
fo(i) as (a numeric, b numeric);
The connection to the server was lost. Attempting reset: Failed.

Regards

Pavel Stehule


2012/12/3 Asif Rehman <asifr.rehman@gmail.com>:
> Hi,
>
> Thanks for the review. Please see the updated patch.
>
>> Hmm.  We're running an I/O cast during do_tup_convert() now, and look
>> up the required functions for each tuple.  I think if we're going to
>> support this operation at that level, we need to look up the necessary
>> functions at convert_tuples_by_foo level, and then apply unconditionally
>> if they've been set up.
>
> Done. TupleConversionMap struct should keep the array of functions oid's
> that
> needs to be applied. Though only for those cases where both attribute type
> id's
> do not match. This way no unnecessary casting will happen.
>
>>
>> Also, what are the implicancies for existing users of tupconvert?  Do we
>> want to apply casting during ANALYZE for example?  AFAICS the patch
>> shouldn't break any case that works today, but I don't see that there
>> has been any analysis of this.
>
> I believe this part of the code should not impact existing users of
> tupconvert.
> As this part of code is relaxing a restriction upon which an error would
> have been
> generated. Though it could have a little impact on performance but should be
> only for
> cases where attribute type id's are different and are implicitly cast able.
>
> Besides existing users involve tupconvert in case of inheritance. And in
> that case
> an exact match is expected.
>
> Regards,
> --Asif

  13. Re: why can't plpgsql return a row-expression?

    Asif Rehman <asifr.rehman@gmail.com> — 2012-12-04T21:04:11Z

    Hi,

Here is the updated patch. I overlooked the loop, checking to free the
conversions map. Here are the results now.

postgres=# select sum(a) from generate_series(1,3000) g(i), lateral foo(i)
as (a numeric, b numeric);
   sum
----------
 303000.0
(1 row)

postgres=# select sum(a) from generate_series(1,3000) g(i), lateral foo(i)
as (a float, b numeric);
       sum
------------------
 303000.000000012

Regards,
--Asif


On Wed, Dec 5, 2012 at 12:40 AM, Pavel Stehule <pavel.stehule@gmail.com>wrote:

> Hello
>
> I fully agree with Asif's arguments
>
> previous tupconvert implementation was really strict - so using
> enhanced tupconver has very minimal impact for old user - and I
> checked same speed for plpgsql function - patched and unpatched pg.
>
> tested
>
> CREATE OR REPLACE FUNCTION public.foo(i integer)
>  RETURNS SETOF record
>  LANGUAGE plpgsql
> AS $function$
> declare r record;
> begin
>   r := (10.1,20.1); for i in 1..10 loop return next r; end loop; return;
> end;
> $function$
>
> select sum(a) from generate_series(1,3000) g(i), lateral foo(i) as (a
> numeric, b numeric);
>
> More - everywhere where we use tupconvert internally, then we use
> cached conversion map - so I am sure, so speed of ANALYZE cannot be
> impacted by this patch
>
> There are other two issue:
>
> it allow to write new differnt slow code - IO cast is about 5-10-20%
> slower, and with this path anybody has new possibilities for new "bad"
> code. But it is not a problem of this patch. It is related to plpgsql
> design and probably we should to move some conversions to outside
> plpgsql to be possible reuse conversion map and enhance plpgsql. I
> have a simple half solutions - plpgsql_check_function can detect this
> situation in almost typical use cases and can raises a performance
> warning. So this issue is not stopper for me - because it is not new
> issue in plpgsql.
>
> Second issue is more significant:
>
> there are bug:
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
> foo(i) as (a numeric, b numeric);
>    sum
> ----------
>  303000.0
> (1 row)
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
> foo(i) as (a float, b numeric);
>           sum
> -----------------------
>  7.33675699577682e-232
> (1 row)
>
> it produces wrong result
>
> And with minimal change it kill session
>
> create or replace function foo(i integer)
> returns setof record as $$
> declare r record;
> begin
>   r := (10,20); for i in 1..10 loop return next r; end loop; return;
> end;
> $$ language plpgsql;
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
> foo(i) as (a numeric, b numeric);
> The connection to the server was lost. Attempting reset: Failed.
>
> create or replace function fo(i integer)
> returns record as $$
> declare r record;
> begin
>   r := (10,20); return r;
> end;
> $$ language plpgsql;
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
> fo(i) as (a int, b numeric);
>   sum
> -------
>  30000
> (1 row)
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
> fo(i) as (a numeric, b numeric);
> The connection to the server was lost. Attempting reset: Failed.
>
> Regards
>
> Pavel Stehule
>
>
> 2012/12/3 Asif Rehman <asifr.rehman@gmail.com>:
> > Hi,
> >
> > Thanks for the review. Please see the updated patch.
> >
> >> Hmm.  We're running an I/O cast during do_tup_convert() now, and look
> >> up the required functions for each tuple.  I think if we're going to
> >> support this operation at that level, we need to look up the necessary
> >> functions at convert_tuples_by_foo level, and then apply unconditionally
> >> if they've been set up.
> >
> > Done. TupleConversionMap struct should keep the array of functions oid's
> > that
> > needs to be applied. Though only for those cases where both attribute
> type
> > id's
> > do not match. This way no unnecessary casting will happen.
> >
> >>
> >> Also, what are the implicancies for existing users of tupconvert?  Do we
> >> want to apply casting during ANALYZE for example?  AFAICS the patch
> >> shouldn't break any case that works today, but I don't see that there
> >> has been any analysis of this.
> >
> > I believe this part of the code should not impact existing users of
> > tupconvert.
> > As this part of code is relaxing a restriction upon which an error would
> > have been
> > generated. Though it could have a little impact on performance but
> should be
> > only for
> > cases where attribute type id's are different and are implicitly cast
> able.
> >
> > Besides existing users involve tupconvert in case of inheritance. And in
> > that case
> > an exact match is expected.
> >
> > Regards,
> > --Asif
>

  14. Re: why can't plpgsql return a row-expression?

    Pavel Stehule <pavel.stehule@gmail.com> — 2012-12-05T08:06:22Z

    2012/12/4 Asif Rehman <asifr.rehman@gmail.com>:
> Hi,
>
> Here is the updated patch. I overlooked the loop, checking to free the
> conversions map. Here are the results now.
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral foo(i)
> as (a numeric, b numeric);
>    sum
> ----------
>  303000.0
> (1 row)
>
> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral foo(i)
> as (a float, b numeric);
>        sum
> ------------------
>  303000.000000012
>
> Regards,
> --Asif

yes, it is fixed.

ok I have no objection

Regards

Pavel Stehule

>
>
> On Wed, Dec 5, 2012 at 12:40 AM, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>>
>> Hello
>>
>> I fully agree with Asif's arguments
>>
>> previous tupconvert implementation was really strict - so using
>> enhanced tupconver has very minimal impact for old user - and I
>> checked same speed for plpgsql function - patched and unpatched pg.
>>
>> tested
>>
>> CREATE OR REPLACE FUNCTION public.foo(i integer)
>>  RETURNS SETOF record
>>  LANGUAGE plpgsql
>> AS $function$
>> declare r record;
>> begin
>>   r := (10.1,20.1); for i in 1..10 loop return next r; end loop; return;
>> end;
>> $function$
>>
>> select sum(a) from generate_series(1,3000) g(i), lateral foo(i) as (a
>> numeric, b numeric);
>>
>> More - everywhere where we use tupconvert internally, then we use
>> cached conversion map - so I am sure, so speed of ANALYZE cannot be
>> impacted by this patch
>>
>> There are other two issue:
>>
>> it allow to write new differnt slow code - IO cast is about 5-10-20%
>> slower, and with this path anybody has new possibilities for new "bad"
>> code. But it is not a problem of this patch. It is related to plpgsql
>> design and probably we should to move some conversions to outside
>> plpgsql to be possible reuse conversion map and enhance plpgsql. I
>> have a simple half solutions - plpgsql_check_function can detect this
>> situation in almost typical use cases and can raises a performance
>> warning. So this issue is not stopper for me - because it is not new
>> issue in plpgsql.
>>
>> Second issue is more significant:
>>
>> there are bug:
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> foo(i) as (a numeric, b numeric);
>>    sum
>> ----------
>>  303000.0
>> (1 row)
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> foo(i) as (a float, b numeric);
>>           sum
>> -----------------------
>>  7.33675699577682e-232
>> (1 row)
>>
>> it produces wrong result
>>
>> And with minimal change it kill session
>>
>> create or replace function foo(i integer)
>> returns setof record as $$
>> declare r record;
>> begin
>>   r := (10,20); for i in 1..10 loop return next r; end loop; return;
>> end;
>> $$ language plpgsql;
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> foo(i) as (a numeric, b numeric);
>> The connection to the server was lost. Attempting reset: Failed.
>>
>> create or replace function fo(i integer)
>> returns record as $$
>> declare r record;
>> begin
>>   r := (10,20); return r;
>> end;
>> $$ language plpgsql;
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> fo(i) as (a int, b numeric);
>>   sum
>> -------
>>  30000
>> (1 row)
>>
>> postgres=# select sum(a) from generate_series(1,3000) g(i), lateral
>> fo(i) as (a numeric, b numeric);
>> The connection to the server was lost. Attempting reset: Failed.
>>
>> Regards
>>
>> Pavel Stehule
>>
>>
>> 2012/12/3 Asif Rehman <asifr.rehman@gmail.com>:
>> > Hi,
>> >
>> > Thanks for the review. Please see the updated patch.
>> >
>> >> Hmm.  We're running an I/O cast during do_tup_convert() now, and look
>> >> up the required functions for each tuple.  I think if we're going to
>> >> support this operation at that level, we need to look up the necessary
>> >> functions at convert_tuples_by_foo level, and then apply
>> >> unconditionally
>> >> if they've been set up.
>> >
>> > Done. TupleConversionMap struct should keep the array of functions oid's
>> > that
>> > needs to be applied. Though only for those cases where both attribute
>> > type
>> > id's
>> > do not match. This way no unnecessary casting will happen.
>> >
>> >>
>> >> Also, what are the implicancies for existing users of tupconvert?  Do
>> >> we
>> >> want to apply casting during ANALYZE for example?  AFAICS the patch
>> >> shouldn't break any case that works today, but I don't see that there
>> >> has been any analysis of this.
>> >
>> > I believe this part of the code should not impact existing users of
>> > tupconvert.
>> > As this part of code is relaxing a restriction upon which an error would
>> > have been
>> > generated. Though it could have a little impact on performance but
>> > should be
>> > only for
>> > cases where attribute type id's are different and are implicitly cast
>> > able.
>> >
>> > Besides existing users involve tupconvert in case of inheritance. And in
>> > that case
>> > an exact match is expected.
>> >
>> > Regards,
>> > --Asif
>
>

  15. Re: why can't plpgsql return a row-expression?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-05T20:46:04Z

    Asif Rehman <asifr.rehman@gmail.com> writes:
> Here is the updated patch. I overlooked the loop, checking to free the
> conversions map. Here are the results now.

I looked at this patch briefly.  It seems to me to be completely
schizophrenic about the coercion rules.  This bit:

-			if (atttypid != att->atttypid ||
-				(atttypmod != att->atttypmod && atttypmod >= 0))
+			if ((atttypmod != att->atttypmod && atttypmod >= 0) ||
+				!can_coerce_type(1, &atttypid, &(att->atttypid), COERCE_IMPLICIT_CAST))

says that we'll allow column types to differ if there is an implicit
cast from the source to the target (or at least I think that's what's
intended, although it's got the source and target backwards).  Fine, but
then why don't we use the cast machinery to do the conversion?  This
is taking plpgsql's cowboy let's-coerce-via-IO-no-matter-whether-that's-
right-or-not approach and sticking it into a core part of the system.
There's no guarantee at all that applying typoutput then typinput
will match the conversion semantics you get from an actual cast, and
in many practical cases such as int4 to int8 it'll be drastically less
efficient anyway.  It would make more sense to do something similar to
coerce_record_to_complex(), that is modify the expression tree to
coerce the columns using the regular cast machinery.

Also, the typmod part of the test seems completely broken.  For one
thing, comparing typmods isn't sane if the types themselves aren't
the same.  And it's quite unclear to me why we'd want to have an
anything-goes policy for type coercion, or even an implicit-casts-only
policy, but then insist that the typmods match exactly.  This coding
will allow varchar(42) to text, but not varchar(42) to varchar(43)
... where's the sense in that?

The patch also seems to go a great deal further than what was asked for
originally, or indeed is mentioned in the documentation patch, namely
fixing the restriction on RETURN to allow composite-typed expressions.
Specifically it's changing the code that accepts composite input
arguments.  Do we actually want that?  If so, shouldn't it be
documented?

I'm inclined to suggest that we drop all the coercion stuff and just
do what Robert actually asked for originally, which was the mere
ability to return a composite value as long as it matched the function's
result type.  I'm not convinced that we want automatic implicit type
coercions here.  In any case I'm very much against sticking such a thing
into general-purpose support code like tupconvert.c.  That will create a
strong likelihood that plpgsql's poorly-designed coercion semantics will
leak into other aspects of the system.

			regards, tom lane

  16. Re: why can't plpgsql return a row-expression?

    Pavel Stehule <pavel.stehule@gmail.com> — 2012-12-06T05:31:16Z

    2012/12/5 Tom Lane <tgl@sss.pgh.pa.us>:
> Asif Rehman <asifr.rehman@gmail.com> writes:
>> Here is the updated patch. I overlooked the loop, checking to free the
>> conversions map. Here are the results now.
>
> I looked at this patch briefly.  It seems to me to be completely
> schizophrenic about the coercion rules.  This bit:
>
> -                       if (atttypid != att->atttypid ||
> -                               (atttypmod != att->atttypmod && atttypmod >= 0))
> +                       if ((atttypmod != att->atttypmod && atttypmod >= 0) ||
> +                               !can_coerce_type(1, &atttypid, &(att->atttypid), COERCE_IMPLICIT_CAST))
>
> says that we'll allow column types to differ if there is an implicit
> cast from the source to the target (or at least I think that's what's
> intended, although it's got the source and target backwards).  Fine, but
> then why don't we use the cast machinery to do the conversion?  This
> is taking plpgsql's cowboy let's-coerce-via-IO-no-matter-whether-that's-
> right-or-not approach and sticking it into a core part of the system.
> There's no guarantee at all that applying typoutput then typinput
> will match the conversion semantics you get from an actual cast, and
> in many practical cases such as int4 to int8 it'll be drastically less
> efficient anyway.  It would make more sense to do something similar to
> coerce_record_to_complex(), that is modify the expression tree to
> coerce the columns using the regular cast machinery.
>
> Also, the typmod part of the test seems completely broken.  For one
> thing, comparing typmods isn't sane if the types themselves aren't
> the same.  And it's quite unclear to me why we'd want to have an
> anything-goes policy for type coercion, or even an implicit-casts-only
> policy, but then insist that the typmods match exactly.  This coding
> will allow varchar(42) to text, but not varchar(42) to varchar(43)
> ... where's the sense in that?
>
> The patch also seems to go a great deal further than what was asked for
> originally, or indeed is mentioned in the documentation patch, namely
> fixing the restriction on RETURN to allow composite-typed expressions.
> Specifically it's changing the code that accepts composite input
> arguments.  Do we actually want that?  If so, shouldn't it be
> documented?
>
> I'm inclined to suggest that we drop all the coercion stuff and just
> do what Robert actually asked for originally, which was the mere
> ability to return a composite value as long as it matched the function's
> result type.  I'm not convinced that we want automatic implicit type
> coercions here.  In any case I'm very much against sticking such a thing
> into general-purpose support code like tupconvert.c.  That will create a
> strong likelihood that plpgsql's poorly-designed coercion semantics will
> leak into other aspects of the system.

I think so without some change of coercion this patch is not too
useful because very simply test fail

create type foo(a int, b text);

create or replace function foo_func()
returns foo as $$
begin
  ...
  return (10, 'hello');

end;

but we can limit a implicit coercion in tupconvert via new parameter -
because we would to forward plpgsql behave just from this direction.
Then when this parameter - maybe "allowIOCoercion" will be false, then
tupconvert will have same behave like before.

Regards

Pavel

>
>                         regards, tom lane

  17. Re: why can't plpgsql return a row-expression?

    Robert Haas <robertmhaas@gmail.com> — 2012-12-06T17:52:41Z

    On Thu, Dec 6, 2012 at 12:31 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2012/12/5 Tom Lane <tgl@sss.pgh.pa.us>:
>> Asif Rehman <asifr.rehman@gmail.com> writes:
>>> Here is the updated patch. I overlooked the loop, checking to free the
>>> conversions map. Here are the results now.
>>
>> I looked at this patch briefly.  It seems to me to be completely
>> schizophrenic about the coercion rules.  This bit:
>>
>> -                       if (atttypid != att->atttypid ||
>> -                               (atttypmod != att->atttypmod && atttypmod >= 0))
>> +                       if ((atttypmod != att->atttypmod && atttypmod >= 0) ||
>> +                               !can_coerce_type(1, &atttypid, &(att->atttypid), COERCE_IMPLICIT_CAST))
>>
>> says that we'll allow column types to differ if there is an implicit
>> cast from the source to the target (or at least I think that's what's
>> intended, although it's got the source and target backwards).  Fine, but
>> then why don't we use the cast machinery to do the conversion?  This
>> is taking plpgsql's cowboy let's-coerce-via-IO-no-matter-whether-that's-
>> right-or-not approach and sticking it into a core part of the system.
>> There's no guarantee at all that applying typoutput then typinput
>> will match the conversion semantics you get from an actual cast, and
>> in many practical cases such as int4 to int8 it'll be drastically less
>> efficient anyway.  It would make more sense to do something similar to
>> coerce_record_to_complex(), that is modify the expression tree to
>> coerce the columns using the regular cast machinery.
>>
>> Also, the typmod part of the test seems completely broken.  For one
>> thing, comparing typmods isn't sane if the types themselves aren't
>> the same.  And it's quite unclear to me why we'd want to have an
>> anything-goes policy for type coercion, or even an implicit-casts-only
>> policy, but then insist that the typmods match exactly.  This coding
>> will allow varchar(42) to text, but not varchar(42) to varchar(43)
>> ... where's the sense in that?
>>
>> The patch also seems to go a great deal further than what was asked for
>> originally, or indeed is mentioned in the documentation patch, namely
>> fixing the restriction on RETURN to allow composite-typed expressions.
>> Specifically it's changing the code that accepts composite input
>> arguments.  Do we actually want that?  If so, shouldn't it be
>> documented?
>>
>> I'm inclined to suggest that we drop all the coercion stuff and just
>> do what Robert actually asked for originally, which was the mere
>> ability to return a composite value as long as it matched the function's
>> result type.  I'm not convinced that we want automatic implicit type
>> coercions here.  In any case I'm very much against sticking such a thing
>> into general-purpose support code like tupconvert.c.  That will create a
>> strong likelihood that plpgsql's poorly-designed coercion semantics will
>> leak into other aspects of the system.
>
> I think so without some change of coercion this patch is not too
> useful because very simply test fail
>
> create type foo(a int, b text);
>
> create or replace function foo_func()
> returns foo as $$
> begin
>   ...
>   return (10, 'hello');
>
> end;
>
> but we can limit a implicit coercion in tupconvert via new parameter -
> because we would to forward plpgsql behave just from this direction.
> Then when this parameter - maybe "allowIOCoercion" will be false, then
> tupconvert will have same behave like before.

It would be nice to make that work, but it could be left for a
separate patch, I suppose.  I'm OK with Tom's proposal to go ahead and
commit the core mechanic first, if doing more than that is
controversial.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  18. Re: why can't plpgsql return a row-expression?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-06T18:36:47Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Dec 6, 2012 at 12:31 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> but we can limit a implicit coercion in tupconvert via new parameter -
>> because we would to forward plpgsql behave just from this direction.
>> Then when this parameter - maybe "allowIOCoercion" will be false, then
>> tupconvert will have same behave like before.

> It would be nice to make that work, but it could be left for a
> separate patch, I suppose.  I'm OK with Tom's proposal to go ahead and
> commit the core mechanic first, if doing more than that is
> controversial.

I'm against putting I/O coercion semantics into tupconvert, period.  Ever.
If plpgsql wants that behavior rather than something more consistent
with the rest of the system, it needs to implement it for itself.

If the per-column conversions were cast-based, it wouldn't be such a
flagrantly bad idea to implement it in tupconvert.  I'm still not
convinced that that's the best place for it though.  tupconvert is about
rearranging columns, not about changing their contents.

It might be more appropriate to invent an expression evaluation
structure that could handle such nontrivial composite-type coercions.
I'm imagining that somehow we disassemble a composite value (produced by
some initial expression node), pass its fields through coercion nodes as
required, and then reassemble them in a toplevel RowExpr.

			regards, tom lane

  19. Re: why can't plpgsql return a row-expression?

    Robert Haas <robertmhaas@gmail.com> — 2012-12-06T19:04:44Z

    On Thu, Dec 6, 2012 at 1:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm against putting I/O coercion semantics into tupconvert, period.  Ever.
> If plpgsql wants that behavior rather than something more consistent
> with the rest of the system, it needs to implement it for itself.

I'm sure that can be done.  I don't think anyone is objecting to that,
just trying to get useful behavior out of the system.

Are you going to commit a stripped-down version of the patch?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  20. Re: why can't plpgsql return a row-expression?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-06T20:14:01Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Are you going to commit a stripped-down version of the patch?

I set it back to "waiting on author" --- don't know if he wants to
produce a stripped-down version with no type coercions, or try to use
cast-based coercions.

			regards, tom lane

  21. Re: why can't plpgsql return a row-expression?

    Asif Rehman <asifr.rehman@gmail.com> — 2012-12-06T21:13:49Z

    Hi,

I have attached the stripped-down version. I will leave the type coercions
support for a separate patch.

Regards,
--Asif



On Fri, Dec 7, 2012 at 1:14 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Robert Haas <robertmhaas@gmail.com> writes:
> > Are you going to commit a stripped-down version of the patch?
>
> I set it back to "waiting on author" --- don't know if he wants to
> produce a stripped-down version with no type coercions, or try to use
> cast-based coercions.
>
>                         regards, tom lane
>

  22. Re: why can't plpgsql return a row-expression?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-06T22:01:16Z

    Asif Rehman <asifr.rehman@gmail.com> writes:
> I have attached the stripped-down version. I will leave the type coercions
> support for a separate patch.

OK, I'll take a look at this one.

			regards, tom lane

  23. Re: why can't plpgsql return a row-expression?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-07T04:11:21Z

    Asif Rehman <asifr.rehman@gmail.com> writes:
> I have attached the stripped-down version. I will leave the type coercions
> support for a separate patch.

Applied with assorted corrections.

			regards, tom lane

  24. Re: why can't plpgsql return a row-expression?

    Asif Rehman <asifr.rehman@gmail.com> — 2012-12-07T04:29:21Z

    Thanks.

Regards,
--Asif


On Fri, Dec 7, 2012 at 9:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Asif Rehman <asifr.rehman@gmail.com> writes:
> > I have attached the stripped-down version. I will leave the type
> coercions
> > support for a separate patch.
>
> Applied with assorted corrections.
>
>                         regards, tom lane
>