Thread: why can't plpgsql return a row-expression?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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