Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 55b4966

Browse files
committed
Fix misleading comment for get_cheapest_group_keys_order
The header comment for get_cheapest_group_keys_order() claimed that the output arguments were set to a newly allocated list which may be freed by the calling function, however, this was not always true as the function would simply leave these arguments untouched in some cases. This tripped me up when working on 1349d27 as I mistakenly assumed I could perform a list_concat with the output parameters. That turned out bad due to list_concat modifying the original input lists. In passing, make it more clear that the number of distinct values is important to reduce tiebreaks during sorts. Also, explain what the n_preordered parameter means. Backpatch-through: 15, where get_cheapest_group_keys_order was introduced.
1 parent 78a9af1 commit 55b4966

File tree

1 file changed

+16
-13
lines changed

1 file changed

+16
-13
lines changed

src/backend/optimizer/path/pathkeys.c

+16-13
Original file line numberDiff line numberDiff line change
@@ -587,22 +587,25 @@ pathkey_sort_cost_comparator(const void *_a, const void *_b)
587587

588588
/*
589589
* get_cheapest_group_keys_order
590-
* Reorders the group pathkeys/clauses to minimize the comparison cost.
590+
* Reorders the group pathkeys / clauses to minimize the comparison cost.
591591
*
592-
* Given a list of pathkeys, we try to reorder them in a way that minimizes
593-
* the CPU cost of sorting. This depends mainly on the cost of comparator
594-
* function (the pathkeys may use different data types) and the number of
595-
* distinct values in each column (which affects the number of comparator
596-
* calls for the following pathkeys).
592+
* Given the list of pathkeys in '*group_pathkeys', we try to arrange these
593+
* in an order that minimizes the sort costs that will be incurred by the
594+
* GROUP BY. The costs mainly depend on the cost of the sort comparator
595+
* function(s) and the number of distinct values in each column of the GROUP
596+
* BY clause (*group_clauses). Sorting on subsequent columns is only required
597+
* for tiebreak situations where two values sort equally.
597598
*
598599
* In case the input is partially sorted, only the remaining pathkeys are
599-
* considered.
600-
*
601-
* Returns true if the keys/clauses have been reordered (or might have been),
602-
* and a new list is returned through an argument. The list is a new copy
603-
* and may be freed using list_free.
604-
*
605-
* Returns false if no reordering was possible.
600+
* considered. 'n_preordered' denotes how many of the leading *group_pathkeys
601+
* the input is presorted by.
602+
*
603+
* Returns true and sets *group_pathkeys and *group_clauses to the newly
604+
* ordered versions of the lists that were passed in via these parameters.
605+
* If no reordering was deemed necessary then we return false, in which case
606+
* the *group_pathkeys and *group_clauses lists are left untouched. The
607+
* original *group_pathkeys and *group_clauses parameter values are never
608+
* destructively modified in place.
606609
*/
607610
static bool
608611
get_cheapest_group_keys_order(PlannerInfo *root, double nrows,

0 commit comments

Comments
 (0)