On Wed, Apr 6, 2022 at 7:36 PM David Rowley <dgrowle...@gmail.com> wrote:
> On Wed, 6 Apr 2022 at 00:59, Andy Fan <zhihui.fan1...@gmail.com> wrote: > > > > On Tue, Apr 5, 2022 at 7:49 PM David Rowley <dgrowle...@gmail.com> > wrote: > >> Yeah, there is more performance to be had than even what you've done > >> there. There's no reason really for spool_tuples() to do > >> tuplestore_puttupleslot() when we're not in run mode. > > > > > > Yeah, this is a great idea. > > I've attached an updated patch that does most of what you mentioned. > To make this work I had to add another state to the WindowAggStatus. > This new state is what the top-level WindowAgg will move into when > there's a PARTITION BY clause and the run condition becomes false. > The new state is named WINDOWAGG_PASSTHROUGH_STRICT, which does all > that WINDOWAGG_PASSTHROUGH does plus skips tuplestoring tuples during > the spool. We must still spool those tuples when we're not the > top-level WindowAgg so that we can send those out to any calling > WindowAgg nodes. They'll need those so they return the correct result. > > This means that for intermediate WindowAgg nodes, when the > runcondition becomes false, we only skip evaluation of WindowFuncs. > WindowAgg nodes above us cannot reference these, so there's no need to > evaluate them, plus, if there's a run condition then these tuples will > be filtered out in the final WindowAgg node. > > For the top-level WindowAgg node, when the run condition becomes false > we can save quite a bit more work. If there's no PARTITION BY clause, > then we're done. Just return NULL. When there is a PARTITION BY > clause we move into WINDOWAGG_PASSTHROUGH_STRICT which allows us to > skip both the evaluation of WindowFuncs and also allows us to consume > tuples from our outer plan until we get a tuple belonging to another > partition. No need to tuplestore these tuples as they're being > filtered out. > > Since intermediate WindowAggs cannot filter tuples, all the filtering > must occur in the top-level WindowAgg. This cannot be done by way of > the run condition as the run condition is special as when it becomes > false, we don't check again to see if it's become true. A sort node > between the WindowAggs can change the tuple order (i.e previously > monotonic values may no longer be monotonic) so it's only valid to > evaluate the run condition that's meant for the WindowAgg node it was > intended for. To filter out the tuples that don't match the run > condition from intermediate WindowAggs in the top-level WindowAgg, > what I've done is introduced quals for WindowAgg nodes. This means > that we can now see Filter in EXPLAIN For WindowAgg and "Rows Removed > by Filter". > > Why didn't I just do the filtering in the outer query like was > happening before? The problem is that when we push the quals down > into the subquery, we don't yet have knowledge of which order that the > WindowAggs will be evaluated in. Only run conditions from > intermediate WindowAggs will ever make it into the Filter, and we > don't know which one the top-level WindowAgg will be until later in > planning. To do the filtering in the outer query we'd need to push > quals back out the subquery again. It seems to me to be easier and > better to filter them out lower down in the plan. > > Since the top-level WindowAgg node can now filter tuples, the executor > node had to be given a for(;;) loop so that it goes around again for > another tuple after it filters a tuple out. > > I've also updated the commit message which I think I've made quite > clear about what we optimise and how it's done. > > > And I would suggest the below fastpath for this feature. > > - if > (check_and_push_window_quals(subquery, rte, rti, clause)) > > + if (!subquery->hasWindowFuncs || > check_and_push_window_quals(subquery, rte, rti, clause)) > > Good idea. Thanks! > > David > Hi, + * We must keep the original qual in place if there is a + * PARTITION BY clause as the top-level WindowAgg remains in + * pass-through mode and does nothing to filter out unwanted + * tuples. + */ + *keep_original = false; The comment talks about keeping original qual but the assignment uses the value false. Maybe the comment can be rephrased so that it matches the assignment. Cheers
