>>>>> "Hitoshi" == Hitoshi Harada <umi.tan...@gmail.com> writes:
Hitoshi> Questions here: Hitoshi> - agglevelsup? Hitoshi> We have aggregate capability that all arguments from upper Hitoshi> level query in downer level aggregate makes aggregate call Hitoshi> itself to upper level call, as a constant value in downer Hitoshi> level. What if ORDER BY clause has downer 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 a Hitoshi> 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 as Hitoshi> 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 have Hitoshi> 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 are Hitoshi> similar to existing DISTINCT and ORDER BY features, parsing Hitoshi> and transformation code are derived from those area. The Hitoshi> executor has few issues: Hitoshi> - #include in nodeAgg.c Hitoshi> executor/tuptable.h is added in the patch but required really? Hitoshi> I removed that 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 added Hitoshi> process_ordered_aggregate_multi() for more than one input Hitoshi> arguments (actually target entries) case. Why have those Hitoshi> two? Could we combine them? Or I couldn't find convincing Hitoshi> 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 more Hitoshi> 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 share Hitoshi> some code to transform DISTINCT and ORDER BY with Hitoshi> traditional code in parse_clause.c, though I'm not sure nor Hitoshi> don't have clear idea. Especially, code around 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.implicit Hitoshi> "implicit" member was added in SortGroupClause. I didn't Hitoshi> find clear reason to add this. Could you show a case to Hitoshi> 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
