Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nWoGR-0000QT-Jm for pgsql-hackers@arkaria.postgresql.org; Tue, 22 Mar 2022 23:50:11 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1nWoGQ-0001Wi-8j for pgsql-hackers@arkaria.postgresql.org; Tue, 22 Mar 2022 23:50:10 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nWoGP-0001VK-RS for pgsql-hackers@lists.postgresql.org; Tue, 22 Mar 2022 23:50:09 +0000 Received: from mail-lf1-x131.google.com ([2a00:1450:4864:20::131]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1nWoGN-0001gB-Cr for pgsql-hackers@lists.postgresql.org; Tue, 22 Mar 2022 23:50:09 +0000 Received: by mail-lf1-x131.google.com with SMTP id l20so32333124lfg.12 for ; Tue, 22 Mar 2022 16:50:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yugabyte.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=zsbjcz7plwjE1QGr1n3FgE6TAnhrvXQVgD6pqjdqKQw=; b=JszsmHtVdDH065Tx+sLg/aRc9tqY5gkPwJ2ObiVr4iJ9YTElcXEwhWrtEX9AMOPR2T BTAWBAd8c1i0pXN8kDm31j4qZ3AgwFPJyUXJFSq/lzAISHFAPFWL3rBXeMfv0kNqXjFb NKINyW7FdhHVl9FWXpp2PhphFIKMam98+/A2E= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zsbjcz7plwjE1QGr1n3FgE6TAnhrvXQVgD6pqjdqKQw=; b=mNeNcNLUWaOKbbqaWFnkxMzQgPQ3+8yUxcsqJm8p5lUg9snTa7uRx/WJjUCL/Xl9O7 9LA9cP+ErATVo4q/zeCHF/uz61wzTh7Yqp8BsCxLU6Ky9EEwYoKegTL5QXEh+cWe/+6D I610wPRHdn0ZoTQKLz0iaky6JO3ZQ2Ichlcy1lRQcT5yh9YLxCjTGJJX3pInuKacjs17 G35h0x7qWn664Rcz5eA4RoTPW0B9bKNG0FoEdxqzZBHcG9A9TbcR4FAKUf67wnWGLiK7 UfxP8XEl3SMJiyNHDEh4F6JLf65nTZnEHD2xYpUGLOTf9LgK9y3LtPUcP4GvDvkoQGOH LTTA== X-Gm-Message-State: AOAM532jFc+2aOCiGoPgS+NcrXF3ifTfNw2a8+bqLHdrgwgkdzhTV+LL Uupntt0UT1BkCEvqYVrRHjWhgAZ1s5Osod56BR0jvA18JeWWfQ== X-Google-Smtp-Source: ABdhPJyEpm6xhjaXlJq/mvQszbNGo8l9/lfETtq1XQNFcMqoY+2kc+WXGm9RILCNQxi2BkOKh4eHLvB9KR8TZXuAqiw= X-Received: by 2002:ac2:4203:0:b0:448:8053:d402 with SMTP id y3-20020ac24203000000b004488053d402mr19605363lfh.687.1647993006502; Tue, 22 Mar 2022 16:50:06 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Zhihong Yu Date: Tue, 22 Mar 2022 16:54:08 -0700 Message-ID: Subject: Re: Window Function "Run Conditions" To: David Rowley Cc: Andy Fan , PostgreSQL Developers Content-Type: multipart/alternative; boundary="000000000000beccf705dad745b4" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000beccf705dad745b4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Mar 22, 2022 at 3:24 PM David Rowley wrote: > On Thu, 26 Aug 2021 at 14:54, Andy Fan wrote: > > > > On Thu, Aug 19, 2021 at 2:35 PM David Rowley > wrote: > > > > > > On Thu, 19 Aug 2021 at 00:20, Andy Fan > wrote: > > > > In the current master, the result is: > > > > > > > > empno | salary | c | dr > > > > -------+--------+---+---- > > > > 8 | 6000 | 4 | 1 > > > > > > > In the patched version=EF=BC=8C 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 othe= r > > > 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 conflic= t > > 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 Rul= e > 4 of > > in SQL2008 - SQL2016.)" > > > > In summary, I am not sure if it is correct to change the execution Orde= r > > 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 =3D get_opfamily_member(opinfo->opfamily_id, + opinfo->oplefttype, + opinfo->oprighttype, + newstrategy); + + newopexpr =3D (OpExpr *) make_opclause(op, + opexpr->opresulttype, + opexpr->opretset, + otherexpr, + (Expr *) wfunc, + opexpr->opcollid, + opexpr->inputcollid); + newopexpr->opfuncid =3D get_opcode(op); + + *keep_original =3D true; + runopexpr =3D newopexpr; It would be nice if this code can be shared. + WindowClause *wclause =3D (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 --000000000000beccf705dad745b4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Mar 22, 2022 at 3:24 PM David= Rowley <dgrowleyml@gmail.com> wrote:
On = Thu, 26 Aug 2021 at 14:54, Andy Fan <zhihui.fan1213@gmail.com> wrote:
>
> On Thu, Aug 19, 2021 at 2:35 PM David Rowley <dgrowleyml@gmail.com> wrote: > >
> > On Thu, 19 Aug 2021 at 00:20, Andy Fan <zhihui.fan1213@gmail.com> wr= ote:
> > > In the current master, the result is:
> > >
> > >=C2=A0 empno | salary | c | dr
> > > -------+--------+---+----
> > >=C2=A0 =C2=A0 =C2=A0 8 |=C2=A0 =C2=A06000 | 4 |=C2=A0 1
> >
> > > In the patched version=EF=BC=8C the result is:
> > >
> > >=C2=A0 empno | salary | c | dr
> > > -------+--------+---+----
> > >=C2=A0 =C2=A0 =C2=A0 8 |=C2=A0 =C2=A06000 | 1 |=C2=A0 1
> >
> > Thanks for taking it for a spin.
> >
> > That's a bit unfortunate.=C2=A0 I don't immediately see h= ow 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 a= pply
> > 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 alread= y.
>
> However there are two troubles in this direction: a).=C2=A0 This may c= onflict
> 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 Ru= le 4 of
> <window clause> in SQL2008 - SQL2016.)"
>
> In summary, I am not sure if it is correct to change the execution Ord= er
> 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.<= br> 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.=C2=A0 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.=C2=A0 Plus, if we start messing with the evaluation<= br> 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.=C2=A0 =C2=A0The 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<= br> 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.=C2=A0 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.=C2=A0 I don't thi= nk
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_le= ft) of=C2=A0find_window_run_conditions().

+ =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 op =3D get_opfamily_member(opinfo-&g= t;opfamily_id,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0opinfo->oplefttype,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opinfo->oprighttype,
+ =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0newstrategy);
+
+= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 newopexpr =3D (OpExpr *) = make_opclause(op,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opexpr->opresultt= ype,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opexpr->opretset,
+ =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0otherexpr,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0(Expr *) wfunc,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opexpr->opcol= lid,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opexpr->inputcollid);
+ = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 newopexpr->opfuncid =3D= get_opcode(op);
+
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= *keep_original =3D true;
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 runopexpr =3D newopexpr;=C2=A0

It would= be nice if this code can be shared.

+ =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 WindowClause *wclause =3D (WindowClause *)
+ = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 list_nth(subquery->windowClause,
+= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wfunc= ->winref - 1);

The code would be more reada= ble if list_nth() is indented.

+ =C2=A0 /* Check t= he left side of the OpExpr */

It seems the cod= e for checking left / right is the same. It would be better to extract and = reuse the code.

Cheers
--000000000000beccf705dad745b4--