From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Stephen Frost <sfrost(at)snowman(dot)net>, Vik Reykja <vikreykja(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: proposal: fix corner use case of variadic fuctions usage |
Date: | 2013-01-21 18:04:14 |
Message-ID: | CAFj8pRBz5tMzB59MNL9s6jNeF7C-mvn1zjPnq63L1EL8r+o7cg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-general pgsql-hackers |
Hello
2013/1/19 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2013/1/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> The approach is also inherently seriously inefficient. ...
>
>> What is important - for this use case - there is simple and perfect
>> possible optimization - in this case "non variadic manner call of
>> variadic "any" function" all variadic parameters will share same type.
>> Inside variadic function I have not information so this situation is
>> in this moment, but just I can remember last used type - and I can
>> reuse it, when parameter type is same like previous parameter.
>
>> So there no performance problem.
>
> Well, if we have to hack each variadic function to make it work well in
> this scenario, that greatly weakens the major argument for the proposed
> patch, namely that it provides a single-point fix for VARIADIC behavior.
>
> BTW, I experimented with lobotomizing array_in's caching of I/O function
> lookup behavior, by deleting the if-test at arrayfuncs.c line 184. That
> seemed to make it about 30% slower for a simple test involving
> converting two-element float8 arrays. So while failing to cache this
> stuff isn't the end of the world, arguing that it's not worth worrying
> about is simply wrong.
>
>>> But large arrays have a worse problem: the approach flat out does
>>> not work for arrays of more than FUNC_MAX_ARGS elements, because
>>> there's no place to put their values in the FunctionCallInfo struct.
>>> This last problem is, so far as I can see, unfixable within this
>>> approach; surely we are not going to accept a feature that only works
>>> for small arrays. So I am going to mark the CF item rejected not just
>>> RWF.
>
>> disagree - non variadic manner call should not be used for walk around
>> FUNC_MAX_ARGS limit. So there should not be passed big array.
>
> That's utter nonsense. Why wouldn't people expect concat(), for
> example, to work for large (or even just moderate-sized) arrays?
>
> This problem *is* a show stopper for this patch. I suggested a way you
> can fix it without having such a limitation. If you don't want to go
> that way, well, it's not going to happen.
>
> I agree the prospect that each variadic-ANY function would have to deal
> with this case for itself is a tad annoying. But there are only two of
> them in the existing system, and it's not like a variadic-ANY function
> isn't a pretty complicated beast anyway.
>
>> You propose now something, what you rejected four months ago.
>
> Well, at the time it wasn't apparent that this approach wouldn't work.
> It is now, though.
so here is rewritten patch
* there are no limit for array size that holds variadic arguments
* iteration over mentioned array is moved to variadic function implementation
* there is a new function get_fn_expr_arg_variadic, that returns
true, when last argument has label VARIADIC - via FuncExpr node
* fix all our variadic "any" functions - concat(), concat_ws() and format()
* there is a small optimization - remember last used typOutput
function and reuse it when possible
* it doesn't change any previous test or documented behave
I hope so almost all issues and questions are solved and there are
agreement on implemented behave.
Regards
Pavel
>
> regards, tom lane
Attachment | Content-Type | Size |
---|---|---|
variadic_any_fix_20130121.patch | application/octet-stream | 26.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Kevin Grittner | 2013-01-21 19:06:03 | Re: Running update in chunks? |
Previous Message | Pascal Tufenkji | 2013-01-21 17:46:18 | cache lookup failed |
From | Date | Subject | |
---|---|---|---|
Next Message | Stefan Kaltenbrunner | 2013-01-21 18:23:20 | Re: pg_dump transaction's read-only mode |
Previous Message | Marti Raudsepp | 2013-01-21 17:37:50 | Re: count(*) of zero rows returns 1 |