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@uni-muenster.de
Am 17.01.2015 um 00:45 schrieb Tom Lane <tgl@sss.pgh.pa.us>:
> "Timmer, Marius" <marius.timmer@uni-muenster.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