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

Commit24c19e9

Browse files
committed
Repair issues with faulty generation of merge-append plans.
create_merge_append_plan failed to honor the CP_EXACT_TLIST flag:it would generate the expected targetlist but then it felt free toadd resjunk sort targets to it. This demonstrably leads to assertionfailures in v11 and HEAD, and it's probably just accidental that wedon't see the same in older branches. I've not looked into whetherthere would be any real-world consequences in non-assert builds.In HEAD, create_append_plan has sprouted the same problem, so fixthat too (although we do not have any test cases that seem able toreach that bug). This is an oversight in commit3fc6e2d whichinvented the CP_EXACT_TLIST flag, so back-patch to 9.6 where thatcame in.convert_subquery_pathkeys would create pathkeys for subquery outputvalues if they match any EquivalenceClass known in the outer queryand are available in the subquery's syntactic targetlist. However,the second part of that condition is wrong, because such values mightnot appear in the subquery relation's reltarget list, which wouldmean that they couldn't be accessed above the level of the subqueryscan. We must check that they appear in the reltarget list, instead.This can lead to dropping knowledge about the subquery's sortordering, but I believe it's okay, because any sort key that theouter query actually has any interest in would appear in thereltarget list.This second issue is of very long standing, but right now there's noevidence that it causes observable problems before 9.6, so I refrainedfrom back-patching further than that. We can revisit that choice ifsomebody finds a way to make it cause problems in older branches.(Developing useful test cases for these issues is really problematic;fixing convert_subquery_pathkeys removes the only known way to exhibitthe create_merge_append_plan bug, and neither of the test cases addedby this patch causes a problem in all branches, even when consideringthe issues separately.)The second issue explains bug #15795 from Suresh Kumar R ("could notfind pathkey item to sort" with nested DISTINCT queries). I stumbledacross the first issue while investigating that.Discussion:https://postgr.es/m/15795-fadb56c8e44ee73c@postgresql.org
1 parent64084d6 commit24c19e9

File tree

4 files changed

+208
-30
lines changed

4 files changed

+208
-30
lines changed

‎src/backend/optimizer/path/pathkeys.c

Lines changed: 52 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ static bool pathkey_is_redundant(PathKey *new_pathkey, List *pathkeys);
3333
staticboolmatches_boolean_partition_clause(RestrictInfo*rinfo,
3434
RelOptInfo*partrel,
3535
intpartkeycol);
36+
staticVar*find_var_for_subquery_tle(RelOptInfo*rel,TargetEntry*tle);
3637
staticboolright_merge_direction(PlannerInfo*root,PathKey*pathkey);
3738

3839

@@ -771,9 +772,11 @@ build_expression_pathkey(PlannerInfo *root,
771772
* 'subquery_pathkeys': the subquery's output pathkeys, in its terms.
772773
* 'subquery_tlist': the subquery's output targetlist, in its terms.
773774
*
774-
* It is not necessary for caller to do truncate_useless_pathkeys(),
775-
* because we select keys in a way that takes usefulness of the keys into
776-
* account.
775+
* We intentionally don't do truncate_useless_pathkeys() here, because there
776+
* are situations where seeing the raw ordering of the subquery is helpful.
777+
* For example, if it returns ORDER BY x DESC, that may prompt us to
778+
* construct a mergejoin using DESC order rather than ASC order; but the
779+
* right_merge_direction heuristic would have us throw the knowledge away.
777780
*/
778781
List*
779782
convert_subquery_pathkeys(PlannerInfo*root,RelOptInfo*rel,
@@ -799,22 +802,22 @@ convert_subquery_pathkeys(PlannerInfo *root, RelOptInfo *rel,
799802
* that same targetlist entry.
800803
*/
801804
TargetEntry*tle;
805+
Var*outer_var;
802806

803807
if (sub_eclass->ec_sortref==0)/* can't happen */
804808
elog(ERROR,"volatile EquivalenceClass has no sortref");
805809
tle=get_sortgroupref_tle(sub_eclass->ec_sortref,subquery_tlist);
806810
Assert(tle);
807-
/* resjunk items aren't visible to outer query */
808-
if (!tle->resjunk)
811+
/* Is TLE actually available to the outer query? */
812+
outer_var=find_var_for_subquery_tle(rel,tle);
813+
if (outer_var)
809814
{
810815
/* We can represent this sub_pathkey */
811816
EquivalenceMember*sub_member;
812-
Expr*outer_expr;
813817
EquivalenceClass*outer_ec;
814818

815819
Assert(list_length(sub_eclass->ec_members)==1);
816820
sub_member= (EquivalenceMember*)linitial(sub_eclass->ec_members);
817-
outer_expr= (Expr*)makeVarFromTargetEntry(rel->relid,tle);
818821

819822
/*
820823
* Note: it might look funny to be setting sortref = 0 for a
@@ -828,7 +831,7 @@ convert_subquery_pathkeys(PlannerInfo *root, RelOptInfo *rel,
828831
*/
829832
outer_ec=
830833
get_eclass_for_sort_expr(root,
831-
outer_expr,
834+
(Expr*)outer_var,
832835
NULL,
833836
sub_eclass->ec_opfamilies,
834837
sub_member->em_datatype,
@@ -885,14 +888,15 @@ convert_subquery_pathkeys(PlannerInfo *root, RelOptInfo *rel,
885888
foreach(k,subquery_tlist)
886889
{
887890
TargetEntry*tle= (TargetEntry*)lfirst(k);
891+
Var*outer_var;
888892
Expr*tle_expr;
889-
Expr*outer_expr;
890893
EquivalenceClass*outer_ec;
891894
PathKey*outer_pk;
892895
intscore;
893896

894-
/* resjunk items aren't visible to outer query */
895-
if (tle->resjunk)
897+
/* Is TLE actually available to the outer query? */
898+
outer_var=find_var_for_subquery_tle(rel,tle);
899+
if (!outer_var)
896900
continue;
897901

898902
/*
@@ -907,16 +911,9 @@ convert_subquery_pathkeys(PlannerInfo *root, RelOptInfo *rel,
907911
if (!equal(tle_expr,sub_expr))
908912
continue;
909913

910-
/*
911-
* Build a representation of this targetlist entry as an
912-
* outer Var.
913-
*/
914-
outer_expr= (Expr*)makeVarFromTargetEntry(rel->relid,
915-
tle);
916-
917-
/* See if we have a matching EC for that */
914+
/* See if we have a matching EC for the TLE */
918915
outer_ec=get_eclass_for_sort_expr(root,
919-
outer_expr,
916+
(Expr*)outer_var,
920917
NULL,
921918
sub_eclass->ec_opfamilies,
922919
sub_expr_type,
@@ -973,6 +970,41 @@ convert_subquery_pathkeys(PlannerInfo *root, RelOptInfo *rel,
973970
returnretval;
974971
}
975972

973+
/*
974+
* find_var_for_subquery_tle
975+
*
976+
* If the given subquery tlist entry is due to be emitted by the subquery's
977+
* scan node, return a Var for it, else return NULL.
978+
*
979+
* We need this to ensure that we don't return pathkeys describing values
980+
* that are unavailable above the level of the subquery scan.
981+
*/
982+
staticVar*
983+
find_var_for_subquery_tle(RelOptInfo*rel,TargetEntry*tle)
984+
{
985+
ListCell*lc;
986+
987+
/* If the TLE is resjunk, it's certainly not visible to the outer query */
988+
if (tle->resjunk)
989+
returnNULL;
990+
991+
/* Search the rel's targetlist to see what it will return */
992+
foreach(lc,rel->reltarget->exprs)
993+
{
994+
Var*var= (Var*)lfirst(lc);
995+
996+
/* Ignore placeholders */
997+
if (!IsA(var,Var))
998+
continue;
999+
Assert(var->varno==rel->relid);
1000+
1001+
/* If we find a Var referencing this TLE, we're good */
1002+
if (var->varattno==tle->resno)
1003+
returncopyObject(var);/* Make a copy for safety */
1004+
}
1005+
returnNULL;
1006+
}
1007+
9761008
/*
9771009
* build_join_pathkeys
9781010
* Build the path keys for a join relation constructed by mergejoin or

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

Lines changed: 55 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,10 @@ static List *get_gating_quals(PlannerInfo *root, List *quals);
8181
staticPlan*create_gating_plan(PlannerInfo*root,Path*path,Plan*plan,
8282
List*gating_quals);
8383
staticPlan*create_join_plan(PlannerInfo*root,JoinPath*best_path);
84-
staticPlan*create_append_plan(PlannerInfo*root,AppendPath*best_path);
85-
staticPlan*create_merge_append_plan(PlannerInfo*root,MergeAppendPath*best_path);
84+
staticPlan*create_append_plan(PlannerInfo*root,AppendPath*best_path,
85+
intflags);
86+
staticPlan*create_merge_append_plan(PlannerInfo*root,MergeAppendPath*best_path,
87+
intflags);
8688
staticResult*create_group_result_plan(PlannerInfo*root,
8789
GroupResultPath*best_path);
8890
staticProjectSet*create_project_set_plan(PlannerInfo*root,ProjectSetPath*best_path);
@@ -390,11 +392,13 @@ create_plan_recurse(PlannerInfo *root, Path *best_path, int flags)
390392
break;
391393
caseT_Append:
392394
plan=create_append_plan(root,
393-
(AppendPath*)best_path);
395+
(AppendPath*)best_path,
396+
flags);
394397
break;
395398
caseT_MergeAppend:
396399
plan=create_merge_append_plan(root,
397-
(MergeAppendPath*)best_path);
400+
(MergeAppendPath*)best_path,
401+
flags);
398402
break;
399403
caseT_Result:
400404
if (IsA(best_path,ProjectionPath))
@@ -1054,10 +1058,12 @@ create_join_plan(PlannerInfo *root, JoinPath *best_path)
10541058
* Returns a Plan node.
10551059
*/
10561060
staticPlan*
1057-
create_append_plan(PlannerInfo*root,AppendPath*best_path)
1061+
create_append_plan(PlannerInfo*root,AppendPath*best_path,intflags)
10581062
{
10591063
Append*plan;
10601064
List*tlist=build_path_tlist(root,&best_path->path);
1065+
intorig_tlist_length=list_length(tlist);
1066+
booltlist_was_changed= false;
10611067
List*pathkeys=best_path->path.pathkeys;
10621068
List*subplans=NIL;
10631069
ListCell*subpaths;
@@ -1112,7 +1118,12 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path)
11121118

11131119
if (pathkeys!=NIL)
11141120
{
1115-
/* Compute sort column info, and adjust the Append's tlist as needed */
1121+
/*
1122+
* Compute sort column info, and adjust the Append's tlist as needed.
1123+
* Because we pass adjust_tlist_in_place = true, we may ignore the
1124+
* function result; it must be the same plan node. However, we then
1125+
* need to detect whether any tlist entries were added.
1126+
*/
11161127
(void)prepare_sort_from_pathkeys((Plan*)plan,pathkeys,
11171128
best_path->path.parent->relids,
11181129
NULL,
@@ -1122,6 +1133,7 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path)
11221133
&nodeSortOperators,
11231134
&nodeCollations,
11241135
&nodeNullsFirst);
1136+
tlist_was_changed= (orig_tlist_length!=list_length(plan->plan.targetlist));
11251137
}
11261138

11271139
/* Build the plan for each child */
@@ -1231,7 +1243,20 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path)
12311243

12321244
copy_generic_path_info(&plan->plan, (Path*)best_path);
12331245

1234-
return (Plan*)plan;
1246+
/*
1247+
* If prepare_sort_from_pathkeys added sort columns, but we were told to
1248+
* produce either the exact tlist or a narrow tlist, we should get rid of
1249+
* the sort columns again. We must inject a projection node to do so.
1250+
*/
1251+
if (tlist_was_changed&& (flags& (CP_EXACT_TLIST |CP_SMALL_TLIST)))
1252+
{
1253+
tlist=list_truncate(list_copy(plan->plan.targetlist),
1254+
orig_tlist_length);
1255+
returninject_projection_plan((Plan*)plan,tlist,
1256+
plan->plan.parallel_safe);
1257+
}
1258+
else
1259+
return (Plan*)plan;
12351260
}
12361261

12371262
/*
@@ -1242,11 +1267,14 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path)
12421267
* Returns a Plan node.
12431268
*/
12441269
staticPlan*
1245-
create_merge_append_plan(PlannerInfo*root,MergeAppendPath*best_path)
1270+
create_merge_append_plan(PlannerInfo*root,MergeAppendPath*best_path,
1271+
intflags)
12461272
{
12471273
MergeAppend*node=makeNode(MergeAppend);
12481274
Plan*plan=&node->plan;
12491275
List*tlist=build_path_tlist(root,&best_path->path);
1276+
intorig_tlist_length=list_length(tlist);
1277+
booltlist_was_changed;
12501278
List*pathkeys=best_path->path.pathkeys;
12511279
List*subplans=NIL;
12521280
ListCell*subpaths;
@@ -1265,7 +1293,12 @@ create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path)
12651293
plan->lefttree=NULL;
12661294
plan->righttree=NULL;
12671295

1268-
/* Compute sort column info, and adjust MergeAppend's tlist as needed */
1296+
/*
1297+
* Compute sort column info, and adjust MergeAppend's tlist as needed.
1298+
* Because we pass adjust_tlist_in_place = true, we may ignore the
1299+
* function result; it must be the same plan node. However, we then need
1300+
* to detect whether any tlist entries were added.
1301+
*/
12691302
(void)prepare_sort_from_pathkeys(plan,pathkeys,
12701303
best_path->path.parent->relids,
12711304
NULL,
@@ -1275,6 +1308,7 @@ create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path)
12751308
&node->sortOperators,
12761309
&node->collations,
12771310
&node->nullsFirst);
1311+
tlist_was_changed= (orig_tlist_length!=list_length(plan->targetlist));
12781312

12791313
/*
12801314
* Now prepare the child plans. We must apply prepare_sort_from_pathkeys
@@ -1371,7 +1405,18 @@ create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path)
13711405
node->mergeplans=subplans;
13721406
node->part_prune_info=partpruneinfo;
13731407

1374-
return (Plan*)node;
1408+
/*
1409+
* If prepare_sort_from_pathkeys added sort columns, but we were told to
1410+
* produce either the exact tlist or a narrow tlist, we should get rid of
1411+
* the sort columns again. We must inject a projection node to do so.
1412+
*/
1413+
if (tlist_was_changed&& (flags& (CP_EXACT_TLIST |CP_SMALL_TLIST)))
1414+
{
1415+
tlist=list_truncate(list_copy(plan->targetlist),orig_tlist_length);
1416+
returninject_projection_plan(plan,tlist,plan->parallel_safe);
1417+
}
1418+
else
1419+
returnplan;
13751420
}
13761421

13771422
/*

‎src/test/regress/expected/union.out

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -915,6 +915,79 @@ ORDER BY x;
915915
2 | 4
916916
(1 row)
917917

918+
-- Test cases where the native ordering of a sub-select has more pathkeys
919+
-- than the outer query cares about
920+
explain (costs off)
921+
select distinct q1 from
922+
(select distinct * from int8_tbl i81
923+
union all
924+
select distinct * from int8_tbl i82) ss
925+
where q2 = q2;
926+
QUERY PLAN
927+
----------------------------------------------------------
928+
Unique
929+
-> Merge Append
930+
Sort Key: "*SELECT* 1".q1
931+
-> Subquery Scan on "*SELECT* 1"
932+
-> Unique
933+
-> Sort
934+
Sort Key: i81.q1, i81.q2
935+
-> Seq Scan on int8_tbl i81
936+
Filter: (q2 IS NOT NULL)
937+
-> Subquery Scan on "*SELECT* 2"
938+
-> Unique
939+
-> Sort
940+
Sort Key: i82.q1, i82.q2
941+
-> Seq Scan on int8_tbl i82
942+
Filter: (q2 IS NOT NULL)
943+
(15 rows)
944+
945+
select distinct q1 from
946+
(select distinct * from int8_tbl i81
947+
union all
948+
select distinct * from int8_tbl i82) ss
949+
where q2 = q2;
950+
q1
951+
------------------
952+
123
953+
4567890123456789
954+
(2 rows)
955+
956+
explain (costs off)
957+
select distinct q1 from
958+
(select distinct * from int8_tbl i81
959+
union all
960+
select distinct * from int8_tbl i82) ss
961+
where -q1 = q2;
962+
QUERY PLAN
963+
--------------------------------------------------------
964+
Unique
965+
-> Merge Append
966+
Sort Key: "*SELECT* 1".q1
967+
-> Subquery Scan on "*SELECT* 1"
968+
-> Unique
969+
-> Sort
970+
Sort Key: i81.q1, i81.q2
971+
-> Seq Scan on int8_tbl i81
972+
Filter: ((- q1) = q2)
973+
-> Subquery Scan on "*SELECT* 2"
974+
-> Unique
975+
-> Sort
976+
Sort Key: i82.q1, i82.q2
977+
-> Seq Scan on int8_tbl i82
978+
Filter: ((- q1) = q2)
979+
(15 rows)
980+
981+
select distinct q1 from
982+
(select distinct * from int8_tbl i81
983+
union all
984+
select distinct * from int8_tbl i82) ss
985+
where -q1 = q2;
986+
q1
987+
------------------
988+
4567890123456789
989+
(1 row)
990+
918991
-- Test proper handling of parameterized appendrel paths when the
919992
-- potential join qual is expensive
920993
create function expensivefunc(int) returns int

‎src/test/regress/sql/union.sql

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,34 @@ SELECT * FROM
379379
WHERE x>3
380380
ORDER BY x;
381381

382+
-- Test cases where the native ordering of a sub-select has more pathkeys
383+
-- than the outer query cares about
384+
explain (costs off)
385+
select distinct q1from
386+
(select distinct*from int8_tbl i81
387+
union all
388+
select distinct*from int8_tbl i82) ss
389+
where q2= q2;
390+
391+
select distinct q1from
392+
(select distinct*from int8_tbl i81
393+
union all
394+
select distinct*from int8_tbl i82) ss
395+
where q2= q2;
396+
397+
explain (costs off)
398+
select distinct q1from
399+
(select distinct*from int8_tbl i81
400+
union all
401+
select distinct*from int8_tbl i82) ss
402+
where-q1= q2;
403+
404+
select distinct q1from
405+
(select distinct*from int8_tbl i81
406+
union all
407+
select distinct*from int8_tbl i82) ss
408+
where-q1= q2;
409+
382410
-- Test proper handling of parameterized appendrel paths when the
383411
-- potential join qual is expensive
384412
createfunctionexpensivefunc(int) returnsint

0 commit comments

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