On Tue, Mar 22, 2022 at 3:24 PM David Rowley <dgrowle...@gmail.com> wrote:
> On Thu, 26 Aug 2021 at 14:54, Andy Fan <zhihui.fan1...@gmail.com> wrote: > > > > On Thu, Aug 19, 2021 at 2:35 PM David Rowley <dgrowle...@gmail.com> > wrote: > > > > > > On Thu, 19 Aug 2021 at 00:20, Andy Fan <zhihui.fan1...@gmail.com> > wrote: > > > > In the current master, the result is: > > > > > > > > empno | salary | c | dr > > > > -------+--------+---+---- > > > > 8 | 6000 | 4 | 1 > > > > > > > In the patched version， the result is: > > > > > > > > empno | salary | c | dr > > > > -------+--------+---+---- > > > > 8 | 6000 | 1 | 1 > > > > > > Thanks for taking it for a spin. > > > > > > That's a bit unfortunate. I don't immediately see how to fix it other > > > than to restrict the optimisation to only apply when there's a single > > > WindowClause. It might be possible to relax it further and only apply > > > if it's the final window clause to be evaluated, but in those cases, > > > the savings are likely to be much less anyway as some previous > > > WindowAgg will have exhausted all rows from its subplan. > > > > I am trying to hack the select_active_windows function to make > > sure the WindowClause with Run Condition clause to be the last one > > to evaluate (we also need to consider more than 1 window func has > > run condition), at that time, the run condition clause is ready already. > > > > However there are two troubles in this direction: a). This may conflict > > with "the windows that need the same sorting are adjacent in the list." > > b). "when two or more windows are order-equivalent then all peer rows > > must be presented in the same order in all of them. .. (See General Rule > 4 of > > <window clause> in SQL2008 - SQL2016.)" > > > > In summary, I am not sure if it is correct to change the execution Order > > of WindowAgg freely. > > Thanks for looking at that. > > My current thoughts are that it just feels a little too risky to > adjust the comparison function that sorts the window clauses to pay > attention to the run-condition. > > We would need to ensure that there's just a single window function > with a run condition as it wouldn't be valid for there to be multiple. > It would be easy enough to ensure we only push quals into just 1 > window clause, but that and meddling with the evaluation order has > trade-offs. To do that properly, we'd likely want to consider the > costs when deciding which window clause would benefit from having > quals pushed the most. Plus, if we start messing with the evaluation > order then we'd likely really want some sort of costing to check if > pushing a qual and adjusting the evaluation order is actually cheaper > than not pushing the qual and keeping the clauses in the order that > requires the minimum number of sorts. The planner is not really > geared up for costing things like that properly, that's why we just > assume the order with the least sorts is best. In reality that's often > not going to be true as an index may exist and we might want to > evaluate a clause first if we could get rid of a sort and index scan > instead. Sorting the window clauses based on their SortGroupClause is > just the best we can do for now at that stage in planning. > > I think it's safer to just disable the optimisation when there are > multiple window clauses. Multiple matching clauses are merged > already, so it's perfectly valid to have multiple window functions, > it's just they must share the same window clause. I don't think > that's terrible as with the major use case that I have in mind for > this, the window function is only added to limit the number of rows. > In most cases I can imagine, there'd be no reason to have an > additional window function with different frame options. > > I've attached an updated patch. > Hi, The following code seems to be common between if / else blocks (w.r.t. wfunc_left) of find_window_run_conditions(). + op = get_opfamily_member(opinfo->opfamily_id, + opinfo->oplefttype, + opinfo->oprighttype, + newstrategy); + + newopexpr = (OpExpr *) make_opclause(op, + opexpr->opresulttype, + opexpr->opretset, + otherexpr, + (Expr *) wfunc, + opexpr->opcollid, + opexpr->inputcollid); + newopexpr->opfuncid = get_opcode(op); + + *keep_original = true; + runopexpr = newopexpr; It would be nice if this code can be shared. + WindowClause *wclause = (WindowClause *) + list_nth(subquery->windowClause, + wfunc->winref - 1); The code would be more readable if list_nth() is indented. + /* Check the left side of the OpExpr */ It seems the code for checking left / right is the same. It would be better to extract and reuse the code. Cheers
