Обсуждение: Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs

Atri Sharma wrote
> If order of result rows is not the same as required, an error is raised:
> 
> SELECT * FROM incorrect_order_nulls() ORDER BY e NULLS LAST;
> ERROR:  Order not same as specified

First reaction for the error was unfavorable but (see below) it likely is
the best option and does adequately cover the reason for failure -
programmer error.  It is not data specific (other than by accident) so any
runtime attempt to correct an error is going to be wasted effort and
overhead.


> In the inner joins thread, Tom mentioned having a new node which has
> multiple plans and executor can decide which plan to execute given runtime
> conditions. I played around with the idea, and am open to experiment
> having
> a new node which has a Sort based plan and is executed in case OrderCheck
> node sees that the inherent order of result tuples is not correct.
> Feedback
> here would be very welcome.

Could SQL functions be explored such that if the planner sees an order by it
omits the post-function sort node otherwise it adds an explicit one?

How expensive is sorting an already sorted result?

Runtime conditional sorting seems worse case, depending on implementation,
since a large result with an error in the last bit will end up nearly double
processing.  I'd rather deem unsorted output programmer error and raise the
error so it is likely to be discovered during testing and not have any
meaningful runtime overhead.

David J.




--
View this message in context:
http://postgresql.nabble.com/Patch-to-add-functionality-to-specify-ORDER-BY-in-CREATE-FUNCTION-for-SRFs-tp5832876p5832885.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

David G Johnston <david.g.johnston@gmail.com> writes:
> Atri Sharma wrote
>> If order of result rows is not the same as required, an error is raised:
>> 
>> SELECT * FROM incorrect_order_nulls() ORDER BY e NULLS LAST;
>> ERROR:  Order not same as specified

> First reaction for the error was unfavorable but (see below) it likely is
> the best option and does adequately cover the reason for failure -
> programmer error.

TBH, my first reaction to this entire patch is unfavorable: it's a
solution in search of a problem.  It adds substantial complication not
only for users but for PG developers in order to solve a rather narrow
performance issue.

What would make sense to me is to teach the planner about inlining
SQL functions that include ORDER BY clauses, so that the performance
issue of a double sort could be avoided entirely transparently to
the user.
        regards, tom lane

On 1/5/15, 12:20 PM, Atri Sharma wrote:
>     What would make sense to me is to teach the planner about inlining
>     SQL functions that include ORDER BY clauses, so that the performance
>     issue of a double sort could be avoided entirely transparently to
>     the user.
>
>
> It sounds good, but inlining in current way shall restrict the scope of optimization (which is not applicable for
currentdesign). For eg, you cannot inline RECORD returning SRFs...
 

Related... I'd like to see a way to inline a function that does something like:

CREATE FUNCTION foo(text) RETURNS int LANGUAGE sql AS $$
SELECT a FROM b WHERE lower(b.c) = lower($1)
$$

and have the performance be comparable to

SELECT ..., (SELECT a FROM b WHERE lower(b.c) = lower(something)) AS foo

I realize that there's a whole question about the function not being an SRF, but the thing is this works great when
manuallyinlined and is fast. The SQL function is significantly slower.
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby <Jim.Nasby@bluetreble.com> writes:
> Related... I'd like to see a way to inline a function that does something like:

> CREATE FUNCTION foo(text) RETURNS int LANGUAGE sql AS $$
> SELECT a FROM b WHERE lower(b.c) = lower($1)
> $$

The reason that's not inlined ATM is that the semantics wouldn't be the
same (ie, what happens if the SELECT returns more than one row).  It's
possible they would be the same if we attached a LIMIT 1 to the function's
query, but I'm not 100% sure about that offhand.  I'm also not real sure
that you'd still get good performance if there were an inserted LIMIT;
that would disable at least some optimizations.
        regards, tom lane

On 06-01-2015 PM 04:00, Ashutosh Bapat wrote:
> On Tue, Jan 6, 2015 at 12:23 PM, Atri Sharma <atri.jiit@gmail.com> wrote:
>>> We can eliminate the new node and put onus or having the right order on
>>> the user like we do with volatile setting of the function.
>>>
>>>
>>
>> That is exactly what the new node does, since we are not re sorting right
>> now in case the order is wrong. Please see my explanation upthread,
>> OrderCheck node's primary purpose is to check for a user error in the
>> result rows order. The onus right now to give correct order is on user.
>>
>>
> Even checking whether the output of the function is in the right order or
> not, has its cost. I am suggesting that we can eliminate this cost as well.
> For example, PostgreSQL does not check whether a function is really
> immutable or not.
> 

Sounds something like ORDERED BY x implying output is "known" ordered by
x perhaps enough hint for the planner to make necessary plan choices
though I may be wrong.

Amit

On 06-01-2015 PM 04:08, Atri Sharma wrote:
> On Tue, Jan 6, 2015 at 12:29 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp
>> wrote:
>>> Even checking whether the output of the function is in the right order or
>>> not, has its cost. I am suggesting that we can eliminate this cost as
>> well.
>>> For example, PostgreSQL does not check whether a function is really
>>> immutable or not.
>>>
>>
>> Sounds something like ORDERED BY x implying output is "known" ordered by
>> x perhaps enough hint for the planner to make necessary plan choices
>> though I may be wrong.
>>
>>
>>
> I may be missing something, but isnt what you mentioned the exact
> functionality this patch adds?
> 

I read what Ashutosh says as that a clause like IMMUTABLE does not
entail a node execution. Reading manual for CREATE FUNCTION:

<manual>
IMMUTABLE
STABLE
VOLATILE

These attributes inform the query optimizer about the behavior of the
function.
</manual>

They declare the shape of the kind of output the function produces and
planner simply trusts the declaration meaning it does not add a node to
check if, say, an IMMUTABLE function did not actually modify the
database or that it is returning the same output for a given input.

Though, I have no strong opinion on whether one thing is good or the
other or whether they cover some particular use case all the same.
Perhaps you can say that better.

Thanks,
Amit

Best Wishes,
Ashutosh Bapat

On 06-01-2015 PM 04:26, Atri Sharma wrote:
> On Tue, Jan 6, 2015 at 12:43 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp
>> wrote:
>> Though, I have no strong opinion on whether one thing is good or the
>> other or whether they cover some particular use case all the same.
>> Perhaps you can say that better.
>>
>>
> Personally, I think returning non ordered rows when ORDER BY clause is
> specifically specified by user is a gross violation of security and could
> lead to major user application breakdowns, since the application will trust
> that postgres will return the rows in order since ORDER BY was specified.
> Of course, what Ashutosh suggested makes the patch much simpler, but I
> would rather not go down that road.
> 

I think the same thing applies to IMMUTABLE declarations for example.
Planner trusts (or take as a hint) such declarations during, say,
constraint exclusion where quals involving non-immutable functions are
kept out of the exclusion proof. If a miscreant declares a non-immutable
function IMMUTABLE, then constraint violations may ensue simply because
planner trusted the miscreant. That is, such unsafe restrict clauses
would wrongly prove a partition as being unnecessary to scan. I am sure
there are other sites where such bets are made. In that light, I might
as well call them hints than anything.

<manual>
The volatility category is a *promise* to the optimizer about the
behavior of the function
</manual>

Though, as I said ordering behavior *may not be* a good candidate to
make such promises.

On the other hand, what such a thing might help with, are the situations
where a developer is frustrated because planner would ignore (or is
uninformed about) the order that the developer *knows* his function
produces.

But, if the node you propose to enforce the order is good enough, then
it may be worthwhile to go that route, :)

Thanks,
Amit

On 06-01-2015 PM 04:26, Atri Sharma wrote:
> On Tue, Jan 6, 2015 at 12:43 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp
>> wrote:
>> Though, I have no strong opinion on whether one thing is good or the
>> other or whether they cover some particular use case all the same.
>> Perhaps you can say that better.
>>
>>
> Personally, I think returning non ordered rows when ORDER BY clause is
> specifically specified by user is a gross violation of security and could
> lead to major user application breakdowns, since the application will trust
> that postgres will return the rows in order since ORDER BY was specified.
> Of course, what Ashutosh suggested makes the patch much simpler, but I
> would rather not go down that road.
>

I think the same thing applies to IMMUTABLE declarations for example.
Planner trusts (or take as a hint) such declarations during, say,
constraint exclusion where quals involving non-immutable functions are
kept out of the exclusion proof. If a miscreant declares a non-immutable
function IMMUTABLE, then constraint violations may ensue simply because
planner trusted the miscreant. That is, such unsafe restrict clauses
would wrongly prove a partition as being unnecessary to scan. I am sure
there are other sites where such bets are made. In that light, I might
as well call them hints than anything.

<manual>
The volatility category is a *promise* to the optimizer about the
behavior of the function
</manual>

Though, as I said ordering behavior *may not be* a good candidate to
make such promises.

On the other hand, what such a thing might help with, are the situations
where a developer is frustrated because planner would ignore (or is
uninformed about) the order that the developer *knows* his function
produces.

But, if the node you propose to enforce the order is good enough, then
it may be worthwhile to go that route, :)

Tom Lane wrote:

> What would make sense to me is to teach the planner about inlining
> SQL functions that include ORDER BY clauses, so that the performance
> issue of a double sort could be avoided entirely transparently to
> the user.

Wouldn't this be applicable to functions in other languages too, not
only SQL?

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

On 1/5/15, 3:14 PM, Tom Lane wrote:
> Jim Nasby <Jim.Nasby@bluetreble.com> writes:
>> Related... I'd like to see a way to inline a function that does something like:
>
>> CREATE FUNCTION foo(text) RETURNS int LANGUAGE sql AS $$
>> SELECT a FROM b WHERE lower(b.c) = lower($1)
>> $$
>
> The reason that's not inlined ATM is that the semantics wouldn't be the
> same (ie, what happens if the SELECT returns more than one row).  It's
> possible they would be the same if we attached a LIMIT 1 to the function's
> query, but I'm not 100% sure about that offhand.  I'm also not real sure
> that you'd still get good performance if there were an inserted LIMIT;
> that would disable at least some optimizations.

In this case there's actually a unique index on lower(b.c). I don't know if the planner is smart enough to recognize
thattoday though.
 

Perhaps a good interim solution would be a flag/option you could set on SQL functions to force (or disallow) inlining?
Thatmeans if the option is set it's on the callers head if it doesn't do what's desired. We should throw an error if
there'ssomething about the function that would prevent inlining though.
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 1/6/15, 10:32 AM, Alvaro Herrera wrote:
> Tom Lane wrote:
>
>> What would make sense to me is to teach the planner about inlining
>> SQL functions that include ORDER BY clauses, so that the performance
>> issue of a double sort could be avoided entirely transparently to
>> the user.
>
> Wouldn't this be applicable to functions in other languages too, not
> only SQL?

Dumb question... we can inline functions from other languages? What chunk of code handles that?
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 1/6/15, 1:00 AM, Ashutosh Bapat wrote:
>
> Even checking whether the output of the function is in the right order or not, has its cost. I am suggesting that we
caneliminate this cost as well. For example, PostgreSQL does not check whether a function is really immutable or not.
 

Actually, it does:

select test();
ERROR:  UPDATE is not allowed in a non-volatile function
CONTEXT:  SQL statement "UPDATE i SET i=i+1"PL/pgSQL function test() line 3 at SQL statement
STATEMENT:  select test();
ERROR:  UPDATE is not allowed in a non-volatile function
CONTEXT:  SQL statement "UPDATE i SET i=i+1"
PL/pgSQL function test() line 3 at SQL statement

-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 07-01-2015 AM 08:33, Jim Nasby wrote:
> On 1/6/15, 1:00 AM, Ashutosh Bapat wrote:
>>
>> Even checking whether the output of the function is in the right order
>> or not, has its cost. I am suggesting that we can eliminate this cost
>> as well. For example, PostgreSQL does not check whether a function is
>> really immutable or not.
> 
> Actually, it does:
> 
> select test();
> ERROR:  UPDATE is not allowed in a non-volatile function
> CONTEXT:  SQL statement "UPDATE i SET i=i+1"
>     PL/pgSQL function test() line 3 at SQL statement
> STATEMENT:  select test();
> ERROR:  UPDATE is not allowed in a non-volatile function
> CONTEXT:  SQL statement "UPDATE i SET i=i+1"
> PL/pgSQL function test() line 3 at SQL statement
> 

I think Ashutosh's point is that there is no dedicated executor node to
perform this check. ISTM, the above error is raised during planning
itself as part of the initialization of state for a function.

Thanks,
Amit

On Tue, Jan 6, 2015 at 12:12 AM, Atri Sharma <atri.jiit@gmail.com> wrote:
> I will add the patch to current commitfest.
It has been indeed added to the commit fest 2014-12. That's a bit
late, moving it to upcoming one 2015-02.
Thanks,
-- 
Michael

2015-01-05 Tom Lane <tgl@sss.pgh.pa.us>:

> What would make sense to me is to teach the planner about inlining
> SQL functions that include ORDER BY clauses, so that the performance
> issue of a double sort could be avoided entirely transparently to
> the user.

Another way of getting to the point where the extra check-node is not
needed in obvious cases, would be:

* Apply the current patch in some form.
* Later, add code that analyzes the query inside the function. If it
turns out that the result of the analysis implies the declared order,
don't add the check-node.

The analysis can in principle also be performed for other languages,
but that would most likely be way more complex for the typical "Turing
complete" languages.

Nicolas

-- 
A. Because it breaks the logical sequence of discussion.
Q. Why is top posting bad?

On Mon, Jan 5, 2015 at 12:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> TBH, my first reaction to this entire patch is unfavorable: it's a
> solution in search of a problem.  It adds substantial complication not
> only for users but for PG developers in order to solve a rather narrow
> performance issue.
>
> What would make sense to me is to teach the planner about inlining
> SQL functions that include ORDER BY clauses, so that the performance
> issue of a double sort could be avoided entirely transparently to
> the user.

That's not a bad idea, but it only helps for SQL functions.  Actually,
the problem I have run into in the past was not that the planner
didn't know the ordering of the SRF's return value, but that it had no
statistics for it, and therefore made bad optimization decisions.

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