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 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: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Timmer, Marius marius.tim...@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 -- 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
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.blackw...@rrd.com http://www.rrdonnelley.com http://www.rrdonnelley.com/ * mike.blackw...@rrd.com* On Mon, Jan 19, 2015 at 10:09 AM, Tom Lane t...@sss.pgh.pa.us wrote: Timmer, Marius marius.tim...@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 -- 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
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: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Timmer, Marius marius.tim...@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 -- 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 (fwd)
From: Timmer, Marius marius.tim...@uni-muenster.de Hi, attached is version 8, fixing remaining issues, adding docs and tests as requested/agreed. Marius Arne This looks good to me. Test coverage seems complete. Doc updates are included. Output format looks like it should be acceptable to Heikki. I'll mark this as ready for committer. Thanks for the patch! Mike
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 sche...@uni-muenster.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
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
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
--
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 hlinnakan...@vmware.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 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
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 -- 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
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 -- 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
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.blackw...@rrd.com http://www.rrdonnelley.com http://www.rrdonnelley.com/ * mike.blackw...@rrd.com* On Wed, Jan 7, 2015 at 10:17 AM, Timmer, Marius marius.tim...@uni-muenster.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...@uni-muenster.de
Re: [HACKERS] [PATCH] explain sortorder
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 -- 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
Looking forward to the new patch. I'll give it a more complete testing when you post it. Mike __ *Mike Blackwell | Technical Analyst, Distribution Services/Rollout Management | RR Donnelley* 1750 Wallace Ave | St Charles, IL 60174-3401 Office: 630.313.7818 mike.blackw...@rrd.com http://www.rrdonnelley.com http://www.rrdonnelley.com/ * mike.blackw...@rrd.com* On Tue, Dec 23, 2014 at 5:11 AM, Arne Scheffer arne.schef...@uni-muenster.de wrote: 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
Re: [HACKERS] [PATCH] explain sortorder
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 -- 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
Heikki Linnakangas hlinnakan...@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 -- 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
Re: [HACKERS] [PATCH] explain sortorder
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.blackw...@rrd.com
http://www.rrdonnelley.com
[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 mtimm...@uni-muenster.de, Arne Scheffer arne.schef...@uni-muenster.de, Lukas Kreft lukaskr...@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 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
