Re: [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(at)uni-muenster(dot)de>,
Arne Scheffer <arne(dot)scheffer(at)uni-muenster(dot)de>,
Lukas Kreft <lukaskreft(at)uni-muenster(dot)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

Initial review:

Patch applies cleanly to current head, although it appears to have
soft/hard tab and trailing space issues.

make check fails with the output below. The expected collation clause is
not present.

--
-- Test explain feature: sort order
--
CREATE TABLE sortordertest (n1 char(1), n2 int4);
-- Insert values by which should be ordered
INSERT INTO sortordertest(n1, n2) VALUES ('d', 5), ('b', 3), ('a', 1),
('e', 2), ('c', 4);
-- Display sort order when explain analyze and verbose are true.
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM sortordertest ORDER BY n1
COLLATE "C" DESC, n2;
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)

DROP TABLE sortordertest;

__________________________________________________________________________________
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
Mike(dot)Blackwell(at)rrd(dot)com
http://www.rrdonnelley.com

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

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

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_01(at)uni-muenster(dot)de

V6 of this patch applies, builds and checks against the current HEAD. The
areas below could use some attention.

In explain.c:

malloc() should not be called directly here. palloc() would be the
correct call, I believe, but the functions in stringinfo.h are probably
your best choice as they remove the necessity for dealing with buffer size
and overflow.

There is leftover commented out code from the previous patch version in
the T_Sort case.

In show_sort_group_keys(), the splitting of the existing declaration and
initialization of the keyresno and target seems unnecessary and against the
style of surrounding code.

Multi-line comments should follow the existing format.

There are no tests for the "... is LC_COLLATE" and "COLLATE..." cases.

Section 14.1 of the documentation may need to be updated.

Mike.

__________________________________________________________________________________
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
Mike(dot)Blackwell(at)rrd(dot)com
http://www.rrdonnelley.com

<http://www.rrdonnelley.com/>
* <Mike(dot)Blackwell(at)rrd(dot)com>*

On Wed, Jan 7, 2015 at 10:17 AM, Timmer, Marius <
marius(dot)timmer(at)uni-muenster(dot)de> wrote:

> 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_01(at)uni-muenster(dot)de
>

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_01(at)uni-muenster(dot)de

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

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_01(at)uni-muenster(dot)de

Am 13.01.2015 um 18:52 schrieb Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>:

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

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_01(at)uni-muenster(dot)de

Am 14.01.2015 um 17:42 schrieb Arne Scheffer <scheffa(at)uni-muenster(dot)de>:

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

"Timmer, Marius" <marius(dot)timmer(at)uni-muenster(dot)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(dot)timmer(at)uni-muenster(dot)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(at)uni-muenster(dot)de

Am 17.01.2015 um 00:45 schrieb Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> "Timmer, Marius" <marius(dot)timmer(at)uni-muenster(dot)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(dot)timmer(at)uni-muenster(dot)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

Tom,

Thanks for the comments on what you ended up changing. It helps point out
the kind of things I should be looking for. I'll try to let less slip
through in the future.

Mike

__________________________________________________________________________________
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
Mike(dot)Blackwell(at)rrd(dot)com
http://www.rrdonnelley.com

<http://www.rrdonnelley.com/>
* <Mike(dot)Blackwell(at)rrd(dot)com>*

On Mon, Jan 19, 2015 at 10:09 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "Timmer, Marius" <marius(dot)timmer(at)uni-muenster(dot)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
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>