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...@uni-muenster.de 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 (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers