Thread: [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 <mtimm_01@uni-muenster.de>,
                  Arne Scheffer <arne.scheffer@uni-muenster.de>,
                  Lukas Kreft <lukaskreft@uni-muenster.de>
    @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


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

mtimm_01@uni-muenster.de

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




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

On 12/15/2014 06:49 PM, Mike Blackwell wrote:
>                     QUERY PLAN
> ------------------------------------------------
>   Sort
>     Output: n1, n2, ((n1)::character(1))
>     Sort Key: sortordertest.n1, sortordertest.n2
>     Sort Order:  ASC NULLS LAST,  ASC NULLS LAST
>     ->  Seq Scan on public.sortordertest
>           Output: n1, n2, n1
> (6 rows)

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

- Heikki

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> 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.

+1.  I had assumed without looking that that was what it did already,
else I'd have complained too.
        regards, tom lane

>Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> 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.

>+1.  I had assumed without looking that that was what it did already,
>else I'd have complained too.

>                       regards, tom lane

We will change the patch according to Heikkis suggestions.

A nice Christmas & all the best in the New Year

Arne Scheffer

http://www.uni-muenster.de/ZIV/Mitarbeiter/ArneScheffer.shtml

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

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

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
isLC_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
>
>

"Timmer, Marius" <marius.timmer@uni-muenster.de> writes:
> attached is version 8, fixing remaining issues, adding docs and tests as requested/agreed.

I'll pick this up --- I've been a bit lax about helping with this
commitfest.
        regards, tom lane

"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

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

"Timmer, Marius" <marius.timmer@uni-muenster.de> writes:
> 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.

Right, because that's how addTargetToSortList() would parse it.

> 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?

Sorry, not following?  It's true that what I added to explain.c doesn't
worry too much about the possibility of get_ordering_op_properties()
failing --- that really shouldn't happen for something that was previously
accepted as a sorting operator.  But if it does, "reverse" will just be
left as false, so the behavior will anyway be unsurprising I think.
We could alternatively make it throw a "cache lookup failed" error but
I'm not sure how that makes anyone's life better.
        regards, tom lane