Re: Aggregate ORDER BY patch

>>>>> "Hitoshi" == Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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.