Re: Aggregate ORDER BY patch
От | Andrew Gierth |
---|---|
Тема | Re: Aggregate ORDER BY patch |
Дата | |
Msg-id | 87fx8fliel.fsf@news-spur.riddles.org.uk обсуждение исходный текст |
Ответ на | Re: Aggregate ORDER BY patch (Hitoshi Harada <umi.tanuki@gmail.com>) |
Ответы |
Re: Aggregate ORDER BY patch
Re: Aggregate ORDER BY patch |
Список | pgsql-hackers |
>>>>> "Hitoshi" == Hitoshi Harada <umi.tanuki@gmail.com> writes: Hitoshi> Questions here:Hitoshi> - agglevelsup? Hitoshi> We have aggregate capability that all arguments from upperHitoshi> level query in downer level aggregate makes aggregatecallHitoshi> itself to upper level call, as a constant value in downerHitoshi> level. What if ORDER BY clause hasdowner level Vars? For determining what query level the aggregate belongs to, the expressions in the ORDER BY clause are counted along with the actual argument expressions. Hitoshi> Is it sane? The result is consistent but surprised me aHitoshi> little. No need to raise an error? What case exactly would you consider an error? When an order by expression references a lower (more deeply nested) query level than any of the actual arguments? Hitoshi> - order by 1? Hitoshi> Normal ORDER BY clause accepts constant integer asHitoshi> TargetEntry's resno. The patch seems not to support it.Hitoshi>Shouldn't it be the same as normal ORDER BY? Specifically documented. The SQL spec doesn't allow ordinal positions in ORDER BY any more (those are a holdover from SQL92) and we don't support them in, for example, window ORDER BY clauses. Hitoshi> Performance doesn't seem slowing down, though I don't haveHitoshi> quantitative test result. The performance is intended to be no worse than DISTINCT already was, though it's also no better. Hitoshi> Coding, almost all sane. Since its syntax and semantics areHitoshi> similar to existing DISTINCT and ORDER BY features,parsingHitoshi> and transformation code are derived from those area. TheHitoshi> executor has few issues: Hitoshi> - #include in nodeAgg.cHitoshi> executor/tuptable.h is added in the patch but required really?Hitoshi> I removedthat line but still build without any warnings. The code is making explicit use of various Slot calls declared in tuptable.h. The only reason why it builds without error when you remove that is that utils/tuplesort.h happens to include tuptable.h indirectly. Hitoshi> - process_ordered_aggregate_(single|multi)Hitoshi> It seems that the patch left process_sorted_aggregate()Hitoshi>function as process_ordered_aggregate_single() and addedHitoshi> process_ordered_aggregate_multi()for more than one inputHitoshi> arguments (actually target entries) case. Why have thoseHitoshi>two? Could we combine them? Or I couldn't find convincingHitoshi> reason in comments. Performance. tuplesort_getdatum etc. seems to be substantially faster than tuplesort_gettupleslot especially for the case where you're sorting a pass-by-value datum such as an integer (since the datum is then stored only in the sort tuple header and doesn't require a separate space allocation for itself). Using a slot in all cases would have slowed down some common cases like count(distinct id) by a measurable amount. Cases like array_agg(x order by x) benefit from the faster code path too. The memory management between the two cases is sufficiently different that combining them into one function while still maintaining the slot vs. datum distinction would be ugly and probably error-prone. The relatively minor duplication of logic seemed much clearer to me. Hitoshi> And ParseFuncOrColumn() in parse_func.c now gets moreHitoshi> complicated. I thought very hard about breaking some of that out into a separate function, but it wasn't initially clear which parts might have needed access to the original raw parsetree. I'm open to opinions on this. Hitoshi> Since feature / semantics are similar, I bet we may shareHitoshi> some code to transform DISTINCT and ORDER BY withHitoshi>traditional code in parse_clause.c, though I'm not sure norHitoshi> don't have clear idea. Especially, codearound here Hitoshi> save_next_resno = pstate->p_next_resno;Hitoshi> pstate->p_next_resno = attno + 1; Hitoshi> cheats pstate to transform clauses and I felt a bit fear. The code that transforms RETURNING clauses does something similar with p_next_resno. Almost all the work of transforming the ORDER BY clause is actually done via the existing transformSortClause (which is the reason why p_next_resno needs to be saved and restored), the additional logic is only for the DISTINCT case, to validate the correspondance between DISTINCT args and ORDER BY args and to generate implicit ordering clauses (which provide comparison function info to the executor) when needed. Hitoshi> - SortGroupClause.implicitHitoshi> "implicit" member was added in SortGroupClause. I didn'tHitoshi> find clearreason to add this. Could you show a case toHitoshi> clarify this? Without that flag or something like it, when you do create view foo as select count(distinct x) from table; and then display the view definition, you would get back the query as "select count(distinct x order by x) from table" which would be confusing and unnecessarily backwards- and forwards-incompatible. So the code sets "implicit" for any SortGroupClause that is added for some internal reason rather than being present in the original query, and the deparse code in ruleutils skips such clauses. -- Andrew.
В списке pgsql-hackers по дате отправления: