Re: [PATCH] explain sortorder

Hi Tom,

we are very happy seeing this committed.
Thank you for committing and Mike for the review!!

Your changes make sense to us, except one question:

We think, you wanted to switch to DESC behavior
(print out NULLS FIRST) in cases, where
„USING“ uses an operator which is considered to be
a DESC operator.
Great, we missed that.

But get_equality_op_for_ordering_op is called in
cases, where reverse is false, but
the part

if (reverse)
*reverse = (strategy == BTGreaterStrategyNumber);

never changes this to true?

VlG

Marius & Arne

---
Marius Timmer
Arne Scheffer
Zentrum für Informationsverarbeitung
Westfälische Wilhelms-Universität Münster
Einsteinstraße 60

mtimm_01(at)uni-muenster(dot)de

Am 17.01.2015 um 00:45 schrieb Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> "Timmer, Marius" <marius(dot)timmer(at)uni-muenster(dot)de> writes:
>> attached is version 8, fixing remaining issues, adding docs and tests as requested/agreed.
>
> I've committed this with some rather substantial alterations, notably:
>
> * Having get_sortorder_by_keyno dig into the plan state node by itself
> seemed like a bad idea; it's certainly at variance with the existing
> division of knowledge in this code, and I think it might outright do
> the wrong thing if there's a Sort node underneath an Agg or Group node
> (since in those cases the child Sort node, not the parent node, would
> get passed in). I fixed it so that show_sort_keys and siblings are
> responsible for extracting and passing in the correct data arrays.
>
> * There were some minor bugs in the rules for when to print NULLS
> FIRST/LAST too. I think the way I've got it now is a precise inverse of
> what addTargetToSortList() will do.
>
> * The proposed new regression test cases were not portable ("en_US"
> isn't guaranteed to exist), and I thought adding a new regression
> script file for just one test was a bit excessive. The DESC and
> USING behaviors were already covered by existing test cases so I just
> added something to exercise COLLATE and NULLS FIRST in collate.sql.
>
> * I took out the change in perform.sgml. The change as proposed was
> seriously confusing because it injected off-topic discussion into an
> example that was meant to be just about the additional information printed
> by EXPLAIN ANALYZE. I'm not really sure that this feature needs any
> specific documentation (other than its eventual mention in the release
> notes); but if it does, we should probably add a brand new example
> someplace before the EXPLAIN ANALYZE subsection.
>
> * Assorted cleanups such as removal of irrelevant whitespace changes.
> That sort of thing just makes a reviewer's job harder, so it's best
> to avoid it if you can.
>
> regards, tom lane