On Wed, Jul 11, 2018 at 4:21 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Masahiko Sawada <sawada.m...@gmail.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
update_window_syntax_doc.patch
Description: Binary data
