Re: [HACKERS] [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...@uni-muenster.de
Am 17.01.2015 um 00:45 schrieb Tom Lane :
> "Timmer, Marius" 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:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Hi,
attached is version 8, fixing remaining issues, adding docs and tests as
requested/agreed.
Marius & Arne
---
Marius Timmer
Zentrum für Informationsverarbeitung
Westfälische Wilhelms-Universität Münster
Einsteinstraße 60
mtimm...@uni-muenster.de
Am 14.01.2015 um 17:42 schrieb Arne Scheffer :
> Hi,
>
> we will also remove the following is lc_collate hint in the next version,
> showing only mandatory info as suggested.
>
>/* for those who use COLLATE although their default is already
> the wanted */
>if (strcmp(collname, localeptr) == 0)
>{
>appendStringInfo(sortorderInformation, " (%s is
> LC_COLLATE)", collname);
>}
>
> Anybody insisting on that?
>
> Arne
>
> Note: I see, at the moment we use the wrong default for DESC. We'll fix that.
>
> On Wed, 14 Jan 2015, Heikki Linnakangas wrote:
>
>> On 01/14/2015 05:26 PM, Timmer, Marius wrote:
>>> Hello Heikki,
>>> abbreviated version:
>>> Sorry, the problem is only the unhandy patch text format, not different
>>> opinions how to proceed.
>>> Long version:
>>> The v7 patch file already addressed your suggestions,
>>> but the file contained serveral (old) local commits,
>>> the new ones at the end of the patch text/file.
>>
>> Ah, missed that. I stopped reading when I saw the old stuff there :-).
>>
>>> v7.1 is attached and addresses this issue providing a clean patch file.
>>
>> Ok, thanks, will take a look.
>>
>>> V8 will - as mentioned - add missing docs and regression tests,
>>
>> Great!
>>
>> - Heikki
>>
>>
explain_sortorder-v8.patch
Description: explain_sortorder-v8.patch
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Hello Heikki, abbreviated version: Sorry, the problem is only the unhandy patch text format, not different opinions how to proceed. Long version: The v7 patch file already addressed your suggestions, but the file contained serveral (old) local commits, the new ones at the end of the patch text/file. v7.1 is attached and addresses this issue providing a clean patch file. V8 will - as mentioned - add missing docs and regression tests, Mike suggested. VlG-Arne & Marius --- Marius Timmer Zentrum für Informationsverarbeitung Westfälische Wilhelms-Universität Münster Einsteinstraße 60 mtimm...@uni-muenster.de Am 13.01.2015 um 18:52 schrieb Heikki Linnakangas : > On 01/13/2015 06:04 PM, Timmer, Marius wrote: >> -malloc() (StringInfo is used as suggested now). > > There really shouldn't be any snprintf() calls in the patch, when StringInfo > is used correctly... > >> @@ -1187,6 +1187,7 @@ explain (verbose, costs off) select * from matest0 >> order by 1-id; >> Sort >>Output: matest0.id, matest0.name, ((1 - matest0.id)) >>Sort Key: ((1 - matest0.id)) >> + Sort Order: ASC NULLS LAST >>-> Result >> Output: matest0.id, matest0.name, (1 - matest0.id) >> -> Append > > This patch isn't going to be committed with this output format. Please change > per my suggestion earlier: > >> I don't like this output. If there are a lot of sort keys, it gets >> difficult to match the right ASC/DESC element to the sort key it applies >> to. (Also, there seems to be double-spaces in the list) >> >> I would suggest just adding the information to the Sort Key line. As >> long as you don't print the modifiers when they are defaults (ASC and >> NULLS LAST), we could print the information even in non-VERBOSE mode. So >> it would look something like: >> >> Sort Key: sortordertest.n1 NULLS FIRST, sortordertest.n2 DESC > > Or if you don't agree with that, explain why. > > - Heikki > explain_sortorder-v7_1.patch Description: explain_sortorder-v7_1.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Hi, we removed -malloc() (StringInfo is used as suggested now). -leftover commented out code -the splitting of existing declaration and initialization in show_group_keys(). Missing tests and documentation are WIP and will follow with the next patch version. Best regards Marius --- Marius Timmer Zentrum für Informationsverarbeitung Westfälische Wilhelms-Universität Münster Einsteinstraße 60 mtimm...@uni-muenster.de explain_sortorder-v7.patch Description: explain_sortorder-v7.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Hi, we have spent the last days to realize your suggestions in the patch. It affects the result of a EXPLAIN-Statement (even in non-verbose-mode). Now you will get the order-information for every single sort-key which is not ordered by the defaults. best regards, Marius --- Marius Timmer Zentrum für Informationsverarbeitung Westfälische Wilhelms-Universität Münster Einsteinstraße 60 mtimm...@uni-muenster.de explain_sortorder-v6.patch Description: explain_sortorder-v6.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Hi Mike,
Now I've replaced the spaces at the beginning of the lines with tabs.
Quotes(") in the expected/explain_sortorder.out-File caused failing „make
check“. So I changed them to single quotes(').
I’ve added the corrected patch as attachment.
Kind regards,
Marius
explain_sortorder-v3.patch
Description: Binary data
---
Marius Timmer
Zentrum für Informationsverarbeitung
Westfälische Wilhelms-Universität Münster
Einsteinstraße 60
smime.p7s
Description: S/MIME cryptographic signature
[HACKERS] [PATCH] explain sortorder
Hello everyone, For creating indexes on more than one column, it is useful to know the sort order of each sort key. So now, if you run EXPLAIN in VERBOSE mode, you get the sort order information in the order the sort keys are displayed - Lukas - This patch is meant for discussion - It’s against the master branch - The patch compiles successfully and one test (inherit) is affected - There are no platform-specific items in this patch - The patch, as described, enhances EXPLAIN VERBOSE. For an example, see the regression test - There is no TODO item referring to this patch @patchname: explain_sortorder v2 @version: 2.01 @author: Marius Timmer , Arne Scheffer , Lukas Kreft @description: Display sort order options in VERBOSE mode of EXPLAIN - The situation Currently I am able to run a EXPLAIN-Statement (verbose) for getting more Information about a Query. But it is not enough to check in which order the results will be sorted, what could be interesting to modify some Statements so they can become more performant. - What this patch does This patch will add one more information to the result of an EXPLAIN- Statement in verbose-mode. You will find the new property "Sort order" which tells you the order of the used keys while sorting. You can use it in all available Formats. Greetings, Marius Timmer explain_sortorder-v2.patch Description: Binary data --- Marius Timmer Zentrum für Informationsverarbeitung Westfälische Wilhelms-Universität Münster Einsteinstraße 60 mtimm...@uni-muenster.de smime.p7s Description: S/MIME cryptographic signature
