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?


Marius & Arne

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


Am 17.01.2015 um 00:45 schrieb Tom Lane <t...@sss.pgh.pa.us>:

> "Timmer, Marius" <marius.tim...@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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to