Re: [HACKERS] why can't plpgsql return a row-expression?
Thanks. Regards, --Asif On Fri, Dec 7, 2012 at 9:11 AM, Tom Lane wrote: > Asif Rehman 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 >
Re: [HACKERS] why can't plpgsql return a row-expression?
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 wrote: > Robert Haas 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 > return_rowtype.v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] why can't plpgsql return a row-expression?
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.00012 Regards, --Asif On Wed, Dec 5, 2012 at 12:40 AM, Pavel Stehule 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 > --- > 3 > (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 : > > 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 > return_rowtype.v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] why can't plpgsql return a row-expression?
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 return_rowtype.v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: plpgsql return a row-expression
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 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 wrote:
>
>> related to
>> http://archives.postgresql.org/message-id/caauglxwpedfwae6dajmf7sxewfusa0f68p07retbbpf_fsa...@mail.gmail.com
>>
>> * patched and compiled withou warnings
>>
>> * All 133 tests passed.
>>
>>
>> but
>>
>> I don't like
>>
>> * call invalid function from anonymous block - it is messy (regress
>> tests) - there is no reason why do it
>>
>> +create or replace function foo() returns footype as $$
>> +declare
>> + v record;
>> + v2 record;
>> +begin
>> + v := (1, 'hello');
>> + v2 := (1, 'hello');
>> + return (v || v2);
>> +end;
>> +$$ language plpgsql;
>> +DO $$
>> +declare
>> + v footype;
>> +begin
>> + v := foo();
>> + raise info 'x = %', v.x;
>> + raise info 'y = %', v.y;
>> +end; $$;
>> +ERROR: operator does not exist: record || record
>> +LINE 1: SELECT (v || v2)
>> + ^
>>
>> * there is some performance issue
>>
>> create or replace function fx2(a int)
>> returns footype as $$
>> declare x footype;
>> begin
>> x = (10,20);
>> return x;
>> end;
>> $$ language plpgsql;
>>
>> postgres=# select sum(fx2.x) from generate_series(1,10) g(i),
>> lateral fx2(i);
>>sum
>> -
>> 100
>> (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,10) g(i),
>> lateral fx2(i);
>>sum
>> -
>> 100
>> (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;
>> boolisnull;
>> 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->
Re: [HACKERS] review: plpgsql return a row-expression
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 wrote:
> related to
> http://archives.postgresql.org/message-id/caauglxwpedfwae6dajmf7sxewfusa0f68p07retbbpf_fsa...@mail.gmail.com
>
> * patched and compiled withou warnings
>
> * All 133 tests passed.
>
>
> but
>
> I don't like
>
> * call invalid function from anonymous block - it is messy (regress
> tests) - there is no reason why do it
>
> +create or replace function foo() returns footype as $$
> +declare
> + v record;
> + v2 record;
> +begin
> + v := (1, 'hello');
> + v2 := (1, 'hello');
> + return (v || v2);
> +end;
> +$$ language plpgsql;
> +DO $$
> +declare
> + v footype;
> +begin
> + v := foo();
> + raise info 'x = %', v.x;
> + raise info 'y = %', v.y;
> +end; $$;
> +ERROR: operator does not exist: record || record
> +LINE 1: SELECT (v || v2)
> + ^
>
> * there is some performance issue
>
> create or replace function fx2(a int)
> returns footype as $$
> declare x footype;
> begin
> x = (10,20);
> return x;
> end;
> $$ language plpgsql;
>
> postgres=# select sum(fx2.x) from generate_series(1,10) g(i),
> lateral fx2(i);
>sum
> -
> 100
> (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,10) g(i),
> lateral fx2(i);
>sum
> -
> 100
> (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;
> boolisnull;
> 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 (1000.22234213412342143,'ewqerwqre');
> 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$#
Re: [HACKERS] why can't plpgsql return a row-expression?
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 wrote: > Pavel Stehule writes: > > 2012/10/8 Tom Lane : > >> 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 > return_rowtype.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow domains over composite types
Hi Yeb Havinga, I was digging archives to see anyone worked on supporting domain's over composite type and found your patch, but that was pulled back. According to commitfest comments it needs some more work... Are you going to submit the updated patch? Regards, --Asif On Wed, May 11, 2011 at 5:07 PM, Yeb Havinga wrote: > typecmds.c says: > "Domains over composite types might be made to work in the future, but not > today." > > Attached is a patch that allows domains over composite types, together > with test cases in domaincomp.sql. A domain over a composite type has > typtype TYPTYPE_DOMAIN, but typrelid and typrelkind are empty: that > information is only available in the pg_type record of the base type. The > remainder of the patch follows from that choice. While parsing a record > expression into a row type, an extra coercion node had to be inserted to > ensure that the domain checks are called. > > All regression tests are ok, comments are highly appreciated. > > -- > > Yeb Havinga > http://www.mgrid.net/ > Mastering Medical Data > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
