Re: Avoid extra Sort nodes between WindowAggs when sorting can be reused

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Avoid extra Sort nodes between WindowAggs when sorting can be reused
Date: 2018-07-02 08:25:19
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

> On 26 Jun 2018, at 17:11, Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> wrote:

> I took a look at the patch. It applies and compiles, the tests pass.

Thanks for reviewing, and apologies for the slow response.

> Some thoughts about the code:
> * Postgres lists cache their lengths, so you don't need uniqueLen.

Good point, fixed.

> * Use an array of WindowClauseSortNode's instead of a list. It's more suitable here because you are going to sort (qsort_list creates a temporary array).

I was thinking about that, but opted for code simplicity with a List approach.
The required size of the array isn’t known ahead of time, so it must either
potentially overallocate to the upper bound of root->parse->windowClause or use
heuristics and risk reallocating when growing, neither of which is terribly
appealing. Do you have any suggestions or preferences?

> * Reversing the sorted list looks more confusing to me than just sorting it in the proper order right away. A qsort() comparator returning negative means "left goes before right", but here it is effectively the other way around.


> * There is a function named make_pathkeys_for_window that makes a list of canonical pathkeys given a window clause. Using this function to give you the pathkeys, and then comparing them, would be more future-proof in case we ever start using hashing for windowing. Moreover, the canonical pathkeys can be compared with pointer comparison which is much faster than equal(). Still, I'm not sure whether it's going to be convenient to use in this case, or even whether it is a right thing to do. What do you think?

That’s an interesting thought, one that didn’t occur to me while hacking. I’m
not sure whether is would be wise/clean to overload with this functionality

Attached updated version also adds a testcase that was clearly missing from the
previous version and an updated window.out.

cheers ./daniel

Attachment Content-Type Size
window_prefix_sort-v3.patch application/octet-stream 9.9 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2018-07-02 08:38:36 Re: CREATE TABLE .. LIKE .. EXCLUDING documentation
Previous Message Amit Langote 2018-07-02 08:14:14 Re: Server crashed with dense_rank on partition table.