Re: Failure assertion in GROUPS mode of window function in current HEAD

On Wed, Jul 11, 2018 at 4:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
>> BTW, I found an another but related issue. We can got an assertion
>> failure also by the following query.
>
>> =# select sum(c) over (partition by c order by c groups between 1
>> preceding and current row) from test;
>
> Oh, good point, that's certainly legal per spec, so we'd need to do
> something about it.
>
> The proximate cause of the problem, I think, is that the planner throws
> away the "order by c" as being redundant because it already sorted by c
> for the partitioning requirement. So we end up with ordNumCols = 0
> even though the query had ORDER BY.

Yeah, I think so too.

>
> This breaks not only GROUPS cases, but also RANGE OFFSET cases, because
> the executor expects to have an ordering column. I thought for a bit
> about fixing that by forcing the planner to emit the ordering column for
> RANGE OFFSET even if it's redundant. But it's hard to make the existing
> logic in get_column_info_for_window do that --- it can't tell which
> partitioning column the ordering column was redundant with, and even if it
> could, that column might've been of a different data type. So eventually
> I just threw away that redundant-key-elimination logic altogether.
> I believe that we never thought it was really useful as an optimization,
> but way back when window functions were put in, we didn't have (or at
> least didn't think about) a way to identify the partitioning/ordering
> columns without reference to the input pathkeys.
>

Agreed.

> With this patch, WindowAggPath.winpathkeys is not used for anything
> anymore. I'm inclined to get rid of it, though I didn't do so here.
> (If we keep it, we at least need to adjust the comment in relation.h
> that claims createplan.c needs it.)

+1 for removing.

>
> The other issue here is that nodeWindowAgg's setup logic is not quite
> consistent with update_frameheadpos and update_frametailpos about when
> to create tuplestore read pointers and slots. We might've prevented
> all the inconsistent cases with the parser and planner fixes, but it
> seems best to make them really consistent anyway, so I changed that.
>
> Draft patch attached.
>

Thank you for committing!
I think we also can update the doc about that GROUPS offset mode
requires ORDER BY clause. Thoughts? Attached patch updates it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center