Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitff4f889

Browse files
committed
Fix bugs with degenerate window ORDER BY clauses in GROUPS/RANGE mode.
nodeWindowAgg.c failed to cope with the possibility that no orderingcolumns are defined in the window frame for GROUPS mode or RANGE OFFSETmode, leading to assertion failures or odd errors, as reported by MasahikoSawada and Lukas Eder. In RANGE OFFSET mode, an ordering column is reallyrequired, so add an Assert about that. In GROUPS mode, the code wouldwork, except that the node initialization code wasn't in sync with theexecution code about when to set up tuplestore read pointers and spareslots. Fix the latter for consistency's sake (even though I think thechanges described below make the out-of-sync cases unreachable for now).Per SQL spec, a single ordering column is required for RANGE OFFSET mode,and at least one ordering column is required for GROUPS mode. The parserenforced the former but not the latter; add a check for that.We were able to reach the no-ordering-column cases even with fully speccompliant queries, though, because the planner would drop partitioningand ordering columns from the generated plan if they were redundant withearlier columns according to the redundant-pathkey logic, for instance"PARTITION BY x ORDER BY y" in the presence of a "WHERE x=y" qual.While in principle that's an optimization that could save some pointlesscomparisons at runtime, it seems unlikely to be meaningful in the realworld. I think this behavior was not so much an intentional optimizationas a side-effect of an ancient decision to construct the plan node'sordering-column info by reverse-engineering the PathKeys of the inputpath. If we give up redundant-column removal then it takes very littlecode to generate the plan node info directly from the WindowClause,ensuring that we have the expected number of ordering columns in allcases. (If anyone does complain about this, the planner could perhapsbe taught to remove redundant columns only when it's safe to do so,ie *not* in RANGE OFFSET mode. But I doubt anyone ever will.)With these changes, the WindowAggPath.winpathkeys field is not used foranything anymore, so remove it.The test cases added here are not actually very interesting given theremoval of the redundant-column-removal logic, but they would representimportant corner cases if anyone ever tries to put that back.Tom Lane and Masahiko Sawada. Back-patch to v11 where RANGE OFFSETand GROUPS modes were added.Discussion:https://postgr.es/m/CAD21AoDrWqycq-w_+Bx1cjc+YUhZ11XTj9rfxNiNDojjBx8Fjw@mail.gmail.comDiscussion:https://postgr.es/m/153086788677.17476.8002640580496698831@wrigleys.postgresql.org
1 parentedf59c4 commitff4f889

File tree

10 files changed

+217
-178
lines changed

10 files changed

+217
-178
lines changed

‎src/backend/executor/nodeWindowAgg.c

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1079,6 +1079,7 @@ begin_partition(WindowAggState *winstate)
10791079
{
10801080
WindowAgg*node= (WindowAgg*)winstate->ss.ps.plan;
10811081
PlanState*outerPlan=outerPlanState(winstate);
1082+
intframeOptions=winstate->frameOptions;
10821083
intnumfuncs=winstate->numfuncs;
10831084
inti;
10841085

@@ -1143,8 +1144,8 @@ begin_partition(WindowAggState *winstate)
11431144
* If the frame head is potentially movable, or we have an EXCLUSION
11441145
* clause, we might need to restart aggregation ...
11451146
*/
1146-
if (!(winstate->frameOptions&FRAMEOPTION_START_UNBOUNDED_PRECEDING)||
1147-
(winstate->frameOptions&FRAMEOPTION_EXCLUSION))
1147+
if (!(frameOptions&FRAMEOPTION_START_UNBOUNDED_PRECEDING)||
1148+
(frameOptions&FRAMEOPTION_EXCLUSION))
11481149
{
11491150
/* ... so create a mark pointer to track the frame head */
11501151
agg_winobj->markptr=tuplestore_alloc_read_pointer(winstate->buffer,0);
@@ -1182,21 +1183,24 @@ begin_partition(WindowAggState *winstate)
11821183

11831184
/*
11841185
* If we are in RANGE or GROUPS mode, then determining frame boundaries
1185-
* requires physical access to the frame endpoint rows, except in
1186+
* requires physical access to the frame endpoint rows, except in certain
11861187
* degenerate cases. We create read pointers to point to those rows, to
11871188
* simplify access and ensure that the tuplestore doesn't discard the
1188-
* endpoint rows prematurely. (Mustmatch logic inupdate_frameheadpos
1189-
* and update_frametailpos.)
1189+
* endpoint rows prematurely. (Mustcreate pointers inexactly the same
1190+
*cases that update_frameheadposand update_frametailpos need them.)
11901191
*/
11911192
winstate->framehead_ptr=winstate->frametail_ptr=-1;/* if not used */
11921193

1193-
if ((winstate->frameOptions& (FRAMEOPTION_RANGE |FRAMEOPTION_GROUPS))&&
1194-
node->ordNumCols!=0)
1194+
if (frameOptions& (FRAMEOPTION_RANGE |FRAMEOPTION_GROUPS))
11951195
{
1196-
if (!(winstate->frameOptions&FRAMEOPTION_START_UNBOUNDED_PRECEDING))
1196+
if (((frameOptions&FRAMEOPTION_START_CURRENT_ROW)&&
1197+
node->ordNumCols!=0)||
1198+
(frameOptions&FRAMEOPTION_START_OFFSET))
11971199
winstate->framehead_ptr=
11981200
tuplestore_alloc_read_pointer(winstate->buffer,0);
1199-
if (!(winstate->frameOptions&FRAMEOPTION_END_UNBOUNDED_FOLLOWING))
1201+
if (((frameOptions&FRAMEOPTION_END_CURRENT_ROW)&&
1202+
node->ordNumCols!=0)||
1203+
(frameOptions&FRAMEOPTION_END_OFFSET))
12001204
winstate->frametail_ptr=
12011205
tuplestore_alloc_read_pointer(winstate->buffer,0);
12021206
}
@@ -1210,8 +1214,8 @@ begin_partition(WindowAggState *winstate)
12101214
*/
12111215
winstate->grouptail_ptr=-1;
12121216

1213-
if ((winstate->frameOptions& (FRAMEOPTION_EXCLUDE_GROUP |
1214-
FRAMEOPTION_EXCLUDE_TIES))&&
1217+
if ((frameOptions& (FRAMEOPTION_EXCLUDE_GROUP |
1218+
FRAMEOPTION_EXCLUDE_TIES))&&
12151219
node->ordNumCols!=0)
12161220
{
12171221
winstate->grouptail_ptr=
@@ -1563,6 +1567,9 @@ update_frameheadpos(WindowAggState *winstate)
15631567
boolsub,
15641568
less;
15651569

1570+
/* We must have an ordering column */
1571+
Assert(node->ordNumCols==1);
1572+
15661573
/* Precompute flags for in_range checks */
15671574
if (frameOptions&FRAMEOPTION_START_OFFSET_PRECEDING)
15681575
sub= true;/* subtract startOffset from current row */
@@ -1814,6 +1821,9 @@ update_frametailpos(WindowAggState *winstate)
18141821
boolsub,
18151822
less;
18161823

1824+
/* We must have an ordering column */
1825+
Assert(node->ordNumCols==1);
1826+
18171827
/* Precompute flags for in_range checks */
18181828
if (frameOptions&FRAMEOPTION_END_OFFSET_PRECEDING)
18191829
sub= true;/* subtract endOffset from current row */
@@ -2318,16 +2328,21 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
23182328
winstate->temp_slot_2=ExecInitExtraTupleSlot(estate,scanDesc);
23192329

23202330
/*
2321-
* create frame head and tail slots only if needed (must match logic in
2322-
* update_frameheadpos and update_frametailpos)
2331+
* create frame head and tail slots only if needed (must create slots in
2332+
* exactly the same cases that update_frameheadpos and update_frametailpos
2333+
* need them)
23232334
*/
23242335
winstate->framehead_slot=winstate->frametail_slot=NULL;
23252336

23262337
if (frameOptions& (FRAMEOPTION_RANGE |FRAMEOPTION_GROUPS))
23272338
{
2328-
if (!(frameOptions&FRAMEOPTION_START_UNBOUNDED_PRECEDING))
2339+
if (((frameOptions&FRAMEOPTION_START_CURRENT_ROW)&&
2340+
node->ordNumCols!=0)||
2341+
(frameOptions&FRAMEOPTION_START_OFFSET))
23292342
winstate->framehead_slot=ExecInitExtraTupleSlot(estate,scanDesc);
2330-
if (!(frameOptions&FRAMEOPTION_END_UNBOUNDED_FOLLOWING))
2343+
if (((frameOptions&FRAMEOPTION_END_CURRENT_ROW)&&
2344+
node->ordNumCols!=0)||
2345+
(frameOptions&FRAMEOPTION_END_OFFSET))
23312346
winstate->frametail_slot=ExecInitExtraTupleSlot(estate,scanDesc);
23322347
}
23332348

‎src/backend/nodes/outfuncs.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2110,7 +2110,6 @@ _outWindowAggPath(StringInfo str, const WindowAggPath *node)
21102110

21112111
WRITE_NODE_FIELD(subpath);
21122112
WRITE_NODE_FIELD(winclause);
2113-
WRITE_NODE_FIELD(winpathkeys);
21142113
}
21152114

21162115
staticvoid

‎src/backend/optimizer/plan/createplan.c

Lines changed: 39 additions & 145 deletions
Original file line numberDiff line numberDiff line change
@@ -107,15 +107,6 @@ static WindowAgg *create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_p
107107
staticSetOp*create_setop_plan(PlannerInfo*root,SetOpPath*best_path,
108108
intflags);
109109
staticRecursiveUnion*create_recursiveunion_plan(PlannerInfo*root,RecursiveUnionPath*best_path);
110-
staticvoidget_column_info_for_window(PlannerInfo*root,WindowClause*wc,
111-
List*tlist,
112-
intnumSortCols,AttrNumber*sortColIdx,
113-
int*partNumCols,
114-
AttrNumber**partColIdx,
115-
Oid**partOperators,
116-
int*ordNumCols,
117-
AttrNumber**ordColIdx,
118-
Oid**ordOperators);
119110
staticLockRows*create_lockrows_plan(PlannerInfo*root,LockRowsPath*best_path,
120111
intflags);
121112
staticModifyTable*create_modifytable_plan(PlannerInfo*root,ModifyTablePath*best_path);
@@ -2160,19 +2151,17 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path)
21602151
{
21612152
WindowAgg*plan;
21622153
WindowClause*wc=best_path->winclause;
2154+
intnumPart=list_length(wc->partitionClause);
2155+
intnumOrder=list_length(wc->orderClause);
21632156
Plan*subplan;
21642157
List*tlist;
2165-
intnumsortkeys;
2166-
AttrNumber*sortColIdx;
2167-
Oid*sortOperators;
2168-
Oid*collations;
2169-
bool*nullsFirst;
21702158
intpartNumCols;
21712159
AttrNumber*partColIdx;
21722160
Oid*partOperators;
21732161
intordNumCols;
21742162
AttrNumber*ordColIdx;
21752163
Oid*ordOperators;
2164+
ListCell*lc;
21762165

21772166
/*
21782167
* WindowAgg can project, so no need to be terribly picky about child
@@ -2183,32 +2172,43 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path)
21832172
tlist=build_path_tlist(root,&best_path->path);
21842173

21852174
/*
2186-
* We shouldn't need to actually sort, but it's convenient to use
2187-
* prepare_sort_from_pathkeys to identify the input's sort columns.
2175+
* Convert SortGroupClause lists into arrays of attr indexes and equality
2176+
* operators, as wanted by executor. (Note: in principle, it's possible
2177+
* to drop some of the sort columns, if they were proved redundant by
2178+
* pathkey logic. However, it doesn't seem worth going out of our way to
2179+
* optimize such cases. In any case, we must *not* remove the ordering
2180+
* column for RANGE OFFSET cases, as the executor needs that for in_range
2181+
* tests even if it's known to be equal to some partitioning column.)
21882182
*/
2189-
subplan=prepare_sort_from_pathkeys(subplan,
2190-
best_path->winpathkeys,
2191-
NULL,
2192-
NULL,
2193-
false,
2194-
&numsortkeys,
2195-
&sortColIdx,
2196-
&sortOperators,
2197-
&collations,
2198-
&nullsFirst);
2199-
2200-
/* Now deconstruct that into partition and ordering portions */
2201-
get_column_info_for_window(root,
2202-
wc,
2203-
subplan->targetlist,
2204-
numsortkeys,
2205-
sortColIdx,
2206-
&partNumCols,
2207-
&partColIdx,
2208-
&partOperators,
2209-
&ordNumCols,
2210-
&ordColIdx,
2211-
&ordOperators);
2183+
partColIdx= (AttrNumber*)palloc(sizeof(AttrNumber)*numPart);
2184+
partOperators= (Oid*)palloc(sizeof(Oid)*numPart);
2185+
2186+
partNumCols=0;
2187+
foreach(lc,wc->partitionClause)
2188+
{
2189+
SortGroupClause*sgc= (SortGroupClause*)lfirst(lc);
2190+
TargetEntry*tle=get_sortgroupclause_tle(sgc,subplan->targetlist);
2191+
2192+
Assert(OidIsValid(sgc->eqop));
2193+
partColIdx[partNumCols]=tle->resno;
2194+
partOperators[partNumCols]=sgc->eqop;
2195+
partNumCols++;
2196+
}
2197+
2198+
ordColIdx= (AttrNumber*)palloc(sizeof(AttrNumber)*numOrder);
2199+
ordOperators= (Oid*)palloc(sizeof(Oid)*numOrder);
2200+
2201+
ordNumCols=0;
2202+
foreach(lc,wc->orderClause)
2203+
{
2204+
SortGroupClause*sgc= (SortGroupClause*)lfirst(lc);
2205+
TargetEntry*tle=get_sortgroupclause_tle(sgc,subplan->targetlist);
2206+
2207+
Assert(OidIsValid(sgc->eqop));
2208+
ordColIdx[ordNumCols]=tle->resno;
2209+
ordOperators[ordNumCols]=sgc->eqop;
2210+
ordNumCols++;
2211+
}
22122212

22132213
/* And finally we can make the WindowAgg node */
22142214
plan=make_windowagg(tlist,
@@ -2234,112 +2234,6 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path)
22342234
returnplan;
22352235
}
22362236

2237-
/*
2238-
* get_column_info_for_window
2239-
*Get the partitioning/ordering column numbers and equality operators
2240-
*for a WindowAgg node.
2241-
*
2242-
* This depends on the behavior of planner.c's make_pathkeys_for_window!
2243-
*
2244-
* We are given the target WindowClause and an array of the input column
2245-
* numbers associated with the resulting pathkeys. In the easy case, there
2246-
* are the same number of pathkey columns as partitioning + ordering columns
2247-
* and we just have to copy some data around. However, it's possible that
2248-
* some of the original partitioning + ordering columns were eliminated as
2249-
* redundant during the transformation to pathkeys. (This can happen even
2250-
* though the parser gets rid of obvious duplicates. A typical scenario is a
2251-
* window specification "PARTITION BY x ORDER BY y" coupled with a clause
2252-
* "WHERE x = y" that causes the two sort columns to be recognized as
2253-
* redundant.)In that unusual case, we have to work a lot harder to
2254-
* determine which keys are significant.
2255-
*
2256-
* The method used here is a bit brute-force: add the sort columns to a list
2257-
* one at a time and note when the resulting pathkey list gets longer. But
2258-
* it's a sufficiently uncommon case that a faster way doesn't seem worth
2259-
* the amount of code refactoring that'd be needed.
2260-
*/
2261-
staticvoid
2262-
get_column_info_for_window(PlannerInfo*root,WindowClause*wc,List*tlist,
2263-
intnumSortCols,AttrNumber*sortColIdx,
2264-
int*partNumCols,
2265-
AttrNumber**partColIdx,
2266-
Oid**partOperators,
2267-
int*ordNumCols,
2268-
AttrNumber**ordColIdx,
2269-
Oid**ordOperators)
2270-
{
2271-
intnumPart=list_length(wc->partitionClause);
2272-
intnumOrder=list_length(wc->orderClause);
2273-
2274-
if (numSortCols==numPart+numOrder)
2275-
{
2276-
/* easy case */
2277-
*partNumCols=numPart;
2278-
*partColIdx=sortColIdx;
2279-
*partOperators=extract_grouping_ops(wc->partitionClause);
2280-
*ordNumCols=numOrder;
2281-
*ordColIdx=sortColIdx+numPart;
2282-
*ordOperators=extract_grouping_ops(wc->orderClause);
2283-
}
2284-
else
2285-
{
2286-
List*sortclauses;
2287-
List*pathkeys;
2288-
intscidx;
2289-
ListCell*lc;
2290-
2291-
/* first, allocate what's certainly enough space for the arrays */
2292-
*partNumCols=0;
2293-
*partColIdx= (AttrNumber*)palloc(numPart*sizeof(AttrNumber));
2294-
*partOperators= (Oid*)palloc(numPart*sizeof(Oid));
2295-
*ordNumCols=0;
2296-
*ordColIdx= (AttrNumber*)palloc(numOrder*sizeof(AttrNumber));
2297-
*ordOperators= (Oid*)palloc(numOrder*sizeof(Oid));
2298-
sortclauses=NIL;
2299-
pathkeys=NIL;
2300-
scidx=0;
2301-
foreach(lc,wc->partitionClause)
2302-
{
2303-
SortGroupClause*sgc= (SortGroupClause*)lfirst(lc);
2304-
List*new_pathkeys;
2305-
2306-
sortclauses=lappend(sortclauses,sgc);
2307-
new_pathkeys=make_pathkeys_for_sortclauses(root,
2308-
sortclauses,
2309-
tlist);
2310-
if (list_length(new_pathkeys)>list_length(pathkeys))
2311-
{
2312-
/* this sort clause is actually significant */
2313-
(*partColIdx)[*partNumCols]=sortColIdx[scidx++];
2314-
(*partOperators)[*partNumCols]=sgc->eqop;
2315-
(*partNumCols)++;
2316-
pathkeys=new_pathkeys;
2317-
}
2318-
}
2319-
foreach(lc,wc->orderClause)
2320-
{
2321-
SortGroupClause*sgc= (SortGroupClause*)lfirst(lc);
2322-
List*new_pathkeys;
2323-
2324-
sortclauses=lappend(sortclauses,sgc);
2325-
new_pathkeys=make_pathkeys_for_sortclauses(root,
2326-
sortclauses,
2327-
tlist);
2328-
if (list_length(new_pathkeys)>list_length(pathkeys))
2329-
{
2330-
/* this sort clause is actually significant */
2331-
(*ordColIdx)[*ordNumCols]=sortColIdx[scidx++];
2332-
(*ordOperators)[*ordNumCols]=sgc->eqop;
2333-
(*ordNumCols)++;
2334-
pathkeys=new_pathkeys;
2335-
}
2336-
}
2337-
/* complain if we didn't eat exactly the right number of sort cols */
2338-
if (scidx!=numSortCols)
2339-
elog(ERROR,"failed to deconstruct sort operators into partitioning/ordering operators");
2340-
}
2341-
}
2342-
23432237
/*
23442238
* create_setop_plan
23452239
*

‎src/backend/optimizer/plan/planner.c

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4603,8 +4603,7 @@ create_one_window_path(PlannerInfo *root,
46034603
path= (Path*)
46044604
create_windowagg_path(root,window_rel,path,window_target,
46054605
wflists->windowFuncs[wc->winref],
4606-
wc,
4607-
window_pathkeys);
4606+
wc);
46084607
}
46094608

46104609
add_path(window_rel,path);
@@ -5465,8 +5464,6 @@ make_window_input_target(PlannerInfo *root,
54655464
* The required ordering is first the PARTITION keys, then the ORDER keys.
54665465
* In the future we might try to implement windowing using hashing, in which
54675466
* case the ordering could be relaxed, but for now we always sort.
5468-
*
5469-
* Caution: if you change this, see createplan.c's get_column_info_for_window!
54705467
*/
54715468
staticList*
54725469
make_pathkeys_for_window(PlannerInfo*root,WindowClause*wc,

‎src/backend/optimizer/util/pathnode.c

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3072,19 +3072,17 @@ create_minmaxagg_path(PlannerInfo *root,
30723072
* 'target' is the PathTarget to be computed
30733073
* 'windowFuncs' is a list of WindowFunc structs
30743074
* 'winclause' is a WindowClause that is common to all the WindowFuncs
3075-
* 'winpathkeys' is the pathkeys for the PARTITION keys + ORDER keys
30763075
*
3077-
* Theactual sort order of the input must match winpathkeys, but might
3078-
*have additional keys after those.
3076+
* Theinput must be sorted according to the WindowClause's PARTITION keys
3077+
*plus ORDER BY keys.
30793078
*/
30803079
WindowAggPath*
30813080
create_windowagg_path(PlannerInfo*root,
30823081
RelOptInfo*rel,
30833082
Path*subpath,
30843083
PathTarget*target,
30853084
List*windowFuncs,
3086-
WindowClause*winclause,
3087-
List*winpathkeys)
3085+
WindowClause*winclause)
30883086
{
30893087
WindowAggPath*pathnode=makeNode(WindowAggPath);
30903088

@@ -3102,7 +3100,6 @@ create_windowagg_path(PlannerInfo *root,
31023100

31033101
pathnode->subpath=subpath;
31043102
pathnode->winclause=winclause;
3105-
pathnode->winpathkeys=winpathkeys;
31063103

31073104
/*
31083105
* For costing purposes, assume that there are no redundant partitioning

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp