>>>>> "Surafel" == Surafel Temesgen <surafel3000@gmail.com> writes:
Surafel> Unlike most other executor node limit node has implementation
Surafel> for handling backward scan that support cursor operation but
Surafel> your approach didn't do this inherently because it outsource
Surafel> limitNode functionality to window function and window function
Surafel> didn't do this
Correct. But this is a non-issue: if you want to be able to do backward
scan you are supposed to declare the cursor as SCROLL; if it happens to
work without it, that is pure coincidence. (Cursors declared with neither
SCROLL nor NO SCROLL support backwards scan only if the underlying plan
supports backward scan with no additional overhead, which is something
you can't predict from the query.)
The Limit node declares that it supports backwards scan if, and only if,
its immediate child node supports it. It happens that WindowAgg does
not, so in this implementation, LIMIT ... WITH TIES will not support
backward scan without a tuplestore. I don't consider this an especially
big deal; backward scans are extremely rare (as shown by the fact that
bugs in backward scan have tended to go unnoticed for decades, e.g. bug
#15336), and therefore we should not optimize for them.
Surafel> If am not mistaken the patch also reevaluate limit every time
The (offset+limit) expression is, yes. I noted in the original post that
this needs work - probably it should be pushed out to an InitPlan if it
doesn't fold to a constant. i.e. using the expression
rank() over (...) > (select offset+limit)
where it currently has
rank() over (...) > (offset+limit)
(Generating the limit expression so late in planning is the main thing
that needs changing to get this from a hack POC to usable code)
The main point here is that the same rather minimal executor changes
allow support for not only WITH TIES but also PERCENT and possibly
arbitrary stop conditions as well. (I know I've often wanted LIMIT WHEN
to stop a query at a data-dependent point without having to resort to
recursion - this patch doesn't quite get there, because of the scope
issues involved in analyzing the WHEN condition, but it at least sets up
the concept.)
--
Andrew (irc:RhodiumToad)
