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

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.

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.

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.)

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.

regards, tom lane