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

Commitb538e83

Browse files
committed
Be more careful about the shape of hashable subplan clauses.
nodeSubplan.c expects that the testexpr for a hashable ANY SubPlanhas the form of one or more OpExprs whose LHS is an expression of theouter query's, while the RHS is an expression over Params representingoutput columns of the subquery. However, the planner only went as faras verifying that the clauses were all binary OpExprs. This works99.99% of the time, because the clauses have the right shape whenemitted by the parser --- but it's possible for function inlining tobreak that, as reported by PegoraroF10. To fix, teach the plannerto check that the LHS and RHS contain the right things, or moreaccurately don't contain the wrong things. Given that this has beenbroken for years without anyone noticing, it seems sufficient to justgive up hashing when it happens, rather than go to the trouble ofcommuting the clauses back again (which wouldn't necessarily workanyway).While poking at that, I also noticed that nodeSubplan.c had a baked-inassumption that the number of hash clauses is identical to the numberof subquery output columns. Again, that's fine as far as parser outputgoes, but it's not hard to break it via function inlining. There seemslittle reason for that assumption though --- AFAICS, the only thingit's buying us is not having to store the number of hash clausesexplicitly. Adding code to the planner to reject such cases would takemore code than getting nodeSubplan.c to cope, so I fixed it that way.This has been broken for as long as we've had hashable SubPlans,so back-patch to all supported branches.Discussion:https://postgr.es/m/1549209182255-0.post@n3.nabble.com
1 parentb7cc21c commitb538e83

File tree

7 files changed

+219
-30
lines changed

7 files changed

+219
-30
lines changed

‎src/backend/executor/nodeSubplan.c

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -471,7 +471,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
471471
{
472472
SubPlan*subplan=node->subplan;
473473
PlanState*planstate=node->planstate;
474-
intncols=list_length(subplan->paramIds);
474+
intncols=node->numCols;
475475
ExprContext*innerecontext=node->innerecontext;
476476
MemoryContextoldcontext;
477477
longnbuckets;
@@ -878,11 +878,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
878878
ALLOCSET_SMALL_SIZES);
879879
/* and a short-lived exprcontext for function evaluation */
880880
sstate->innerecontext=CreateExprContext(estate);
881-
/* Silly little array of column numbers 1..n */
882-
ncols=list_length(subplan->paramIds);
883-
sstate->keyColIdx= (AttrNumber*)palloc(ncols*sizeof(AttrNumber));
884-
for (i=0;i<ncols;i++)
885-
sstate->keyColIdx[i]=i+1;
886881

887882
/*
888883
* We use ExecProject to evaluate the lefthand and righthand
@@ -914,13 +909,15 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
914909
(int)nodeTag(subplan->testexpr));
915910
oplist=NIL;/* keep compiler quiet */
916911
}
917-
Assert(list_length(oplist)==ncols);
912+
ncols=list_length(oplist);
918913

919914
lefttlist=righttlist=NIL;
915+
sstate->numCols=ncols;
916+
sstate->keyColIdx= (AttrNumber*)palloc(ncols*sizeof(AttrNumber));
920917
sstate->tab_eq_funcoids= (Oid*)palloc(ncols*sizeof(Oid));
918+
sstate->tab_collations= (Oid*)palloc(ncols*sizeof(Oid));
921919
sstate->tab_hash_funcs= (FmgrInfo*)palloc(ncols*sizeof(FmgrInfo));
922920
sstate->tab_eq_funcs= (FmgrInfo*)palloc(ncols*sizeof(FmgrInfo));
923-
sstate->tab_collations= (Oid*)palloc(ncols*sizeof(Oid));
924921
sstate->lhs_hash_funcs= (FmgrInfo*)palloc(ncols*sizeof(FmgrInfo));
925922
sstate->cur_eq_funcs= (FmgrInfo*)palloc(ncols*sizeof(FmgrInfo));
926923
/* we'll need the cross-type equality fns below, but not in sstate */
@@ -979,6 +976,9 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
979976
/* Set collation */
980977
sstate->tab_collations[i-1]=opexpr->inputcollid;
981978

979+
/* keyColIdx is just column numbers 1..n */
980+
sstate->keyColIdx[i-1]=i;
981+
982982
i++;
983983
}
984984

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

Lines changed: 55 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ typedef struct inline_cte_walker_context
6969
staticNode*build_subplan(PlannerInfo*root,Plan*plan,PlannerInfo*subroot,
7070
List*plan_params,
7171
SubLinkTypesubLinkType,intsubLinkId,
72-
Node*testexpr,booladjust_testexpr,
72+
Node*testexpr,List*testexpr_paramids,
7373
boolunknownEqFalse);
7474
staticList*generate_subquery_params(PlannerInfo*root,List*tlist,
7575
List**paramIds);
@@ -81,7 +81,8 @@ static Node *convert_testexpr(PlannerInfo *root,
8181
staticNode*convert_testexpr_mutator(Node*node,
8282
convert_testexpr_context*context);
8383
staticboolsubplan_is_hashable(Plan*plan);
84-
staticbooltestexpr_is_hashable(Node*testexpr);
84+
staticbooltestexpr_is_hashable(Node*testexpr,List*param_ids);
85+
staticbooltest_opexpr_is_hashable(OpExpr*testexpr,List*param_ids);
8586
staticboolhash_ok_operator(OpExpr*expr);
8687
staticboolcontain_dml(Node*node);
8788
staticboolcontain_dml_walker(Node*node,void*context);
@@ -237,7 +238,7 @@ make_subplan(PlannerInfo *root, Query *orig_subquery,
237238
/* And convert to SubPlan or InitPlan format. */
238239
result=build_subplan(root,plan,subroot,plan_params,
239240
subLinkType,subLinkId,
240-
testexpr,true,isTopQual);
241+
testexpr,NIL,isTopQual);
241242

242243
/*
243244
* If it's a correlated EXISTS with an unimportant targetlist, we might be
@@ -291,12 +292,11 @@ make_subplan(PlannerInfo *root, Query *orig_subquery,
291292
plan_params,
292293
ANY_SUBLINK,0,
293294
newtestexpr,
294-
false, true));
295+
paramIds,
296+
true));
295297
/* Check we got what we expected */
296298
Assert(hashplan->parParam==NIL);
297299
Assert(hashplan->useHashTable);
298-
/* build_subplan won't have filled in paramIds */
299-
hashplan->paramIds=paramIds;
300300

301301
/* Leave it to the executor to decide which plan to use */
302302
asplan=makeNode(AlternativeSubPlan);
@@ -319,7 +319,7 @@ static Node *
319319
build_subplan(PlannerInfo*root,Plan*plan,PlannerInfo*subroot,
320320
List*plan_params,
321321
SubLinkTypesubLinkType,intsubLinkId,
322-
Node*testexpr,booladjust_testexpr,
322+
Node*testexpr,List*testexpr_paramids,
323323
boolunknownEqFalse)
324324
{
325325
Node*result;
@@ -484,10 +484,10 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
484484
else
485485
{
486486
/*
487-
* Adjust the Params in the testexpr, unless callersaid it's not
488-
*needed.
487+
* Adjust the Params in the testexpr, unless calleralready took care
488+
*of it (as indicated by passing a list of Param IDs).
489489
*/
490-
if (testexpr&&adjust_testexpr)
490+
if (testexpr&&testexpr_paramids==NIL)
491491
{
492492
List*params;
493493

@@ -499,7 +499,10 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
499499
params);
500500
}
501501
else
502+
{
502503
splan->testexpr=testexpr;
504+
splan->paramIds=testexpr_paramids;
505+
}
503506

504507
/*
505508
* We can't convert subplans of ALL_SUBLINK or ANY_SUBLINK types to
@@ -511,7 +514,7 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
511514
if (subLinkType==ANY_SUBLINK&&
512515
splan->parParam==NIL&&
513516
subplan_is_hashable(plan)&&
514-
testexpr_is_hashable(splan->testexpr))
517+
testexpr_is_hashable(splan->testexpr,splan->paramIds))
515518
splan->useHashTable= true;
516519

517520
/*
@@ -734,24 +737,20 @@ subplan_is_hashable(Plan *plan)
734737

735738
/*
736739
* testexpr_is_hashable: is an ANY SubLink's test expression hashable?
740+
*
741+
* To identify LHS vs RHS of the hash expression, we must be given the
742+
* list of output Param IDs of the SubLink's subquery.
737743
*/
738744
staticbool
739-
testexpr_is_hashable(Node*testexpr)
745+
testexpr_is_hashable(Node*testexpr,List*param_ids)
740746
{
741747
/*
742748
* The testexpr must be a single OpExpr, or an AND-clause containing only
743-
* OpExprs.
744-
*
745-
* The combining operators must be hashable and strict. The need for
746-
* hashability is obvious, since we want to use hashing. Without
747-
* strictness, behavior in the presence of nulls is too unpredictable. We
748-
* actually must assume even more than plain strictness: they can't yield
749-
* NULL for non-null inputs, either (see nodeSubplan.c). However, hash
750-
* indexes and hash joins assume that too.
749+
* OpExprs, each of which satisfy test_opexpr_is_hashable().
751750
*/
752751
if (testexpr&&IsA(testexpr,OpExpr))
753752
{
754-
if (hash_ok_operator((OpExpr*)testexpr))
753+
if (test_opexpr_is_hashable((OpExpr*)testexpr,param_ids))
755754
return true;
756755
}
757756
elseif (is_andclause(testexpr))
@@ -764,7 +763,7 @@ testexpr_is_hashable(Node *testexpr)
764763

765764
if (!IsA(andarg,OpExpr))
766765
return false;
767-
if (!hash_ok_operator((OpExpr*)andarg))
766+
if (!test_opexpr_is_hashable((OpExpr*)andarg,param_ids))
768767
return false;
769768
}
770769
return true;
@@ -773,6 +772,40 @@ testexpr_is_hashable(Node *testexpr)
773772
return false;
774773
}
775774

775+
staticbool
776+
test_opexpr_is_hashable(OpExpr*testexpr,List*param_ids)
777+
{
778+
/*
779+
* The combining operator must be hashable and strict. The need for
780+
* hashability is obvious, since we want to use hashing. Without
781+
* strictness, behavior in the presence of nulls is too unpredictable. We
782+
* actually must assume even more than plain strictness: it can't yield
783+
* NULL for non-null inputs, either (see nodeSubplan.c). However, hash
784+
* indexes and hash joins assume that too.
785+
*/
786+
if (!hash_ok_operator(testexpr))
787+
return false;
788+
789+
/*
790+
* The left and right inputs must belong to the outer and inner queries
791+
* respectively; hence Params that will be supplied by the subquery must
792+
* not appear in the LHS, and Vars of the outer query must not appear in
793+
* the RHS. (Ordinarily, this must be true because of the way that the
794+
* parser builds an ANY SubLink's testexpr ... but inlining of functions
795+
* could have changed the expression's structure, so we have to check.
796+
* Such cases do not occur often enough to be worth trying to optimize, so
797+
* we don't worry about trying to commute the clause or anything like
798+
* that; we just need to be sure not to build an invalid plan.)
799+
*/
800+
if (list_length(testexpr->args)!=2)
801+
return false;
802+
if (contain_exec_param((Node*)linitial(testexpr->args),param_ids))
803+
return false;
804+
if (contain_var_clause((Node*)lsecond(testexpr->args)))
805+
return false;
806+
return true;
807+
}
808+
776809
/*
777810
* Check expression is hashable + strict
778811
*

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ static bool contain_volatile_functions_not_nextval_walker(Node *node, void *cont
108108
staticboolmax_parallel_hazard_walker(Node*node,
109109
max_parallel_hazard_context*context);
110110
staticboolcontain_nonstrict_functions_walker(Node*node,void*context);
111+
staticboolcontain_exec_param_walker(Node*node,List*param_ids);
111112
staticboolcontain_context_dependent_node(Node*clause);
112113
staticboolcontain_context_dependent_node_walker(Node*node,int*flags);
113114
staticboolcontain_leaked_vars_walker(Node*node,void*context);
@@ -1221,6 +1222,40 @@ contain_nonstrict_functions_walker(Node *node, void *context)
12211222
context);
12221223
}
12231224

1225+
/*****************************************************************************
1226+
*Check clauses for Params
1227+
*****************************************************************************/
1228+
1229+
/*
1230+
* contain_exec_param
1231+
* Recursively search for PARAM_EXEC Params within a clause.
1232+
*
1233+
* Returns true if the clause contains any PARAM_EXEC Param with a paramid
1234+
* appearing in the given list of Param IDs. Does not descend into
1235+
* subqueries!
1236+
*/
1237+
bool
1238+
contain_exec_param(Node*clause,List*param_ids)
1239+
{
1240+
returncontain_exec_param_walker(clause,param_ids);
1241+
}
1242+
1243+
staticbool
1244+
contain_exec_param_walker(Node*node,List*param_ids)
1245+
{
1246+
if (node==NULL)
1247+
return false;
1248+
if (IsA(node,Param))
1249+
{
1250+
Param*p= (Param*)node;
1251+
1252+
if (p->paramkind==PARAM_EXEC&&
1253+
list_member_int(param_ids,p->paramid))
1254+
return true;
1255+
}
1256+
returnexpression_tree_walker(node,contain_exec_param_walker,param_ids);
1257+
}
1258+
12241259
/*****************************************************************************
12251260
*Check clauses for context-dependent nodes
12261261
*****************************************************************************/

‎src/include/nodes/execnodes.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -867,6 +867,8 @@ typedef struct SubPlanState
867867
MemoryContexthashtablecxt;/* memory context containing hash tables */
868868
MemoryContexthashtempcxt;/* temp memory context for hash tables */
869869
ExprContext*innerecontext;/* econtext for computing inner tuples */
870+
intnumCols;/* number of columns being hashed */
871+
/* each of the remaining fields is an array of length numCols: */
870872
AttrNumber*keyColIdx;/* control data for hash tables */
871873
Oid*tab_eq_funcoids;/* equality func oids for table
872874
* datatype(s) */

‎src/include/optimizer/clauses.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ extern bool contain_subplans(Node *clause);
3838
externcharmax_parallel_hazard(Query*parse);
3939
externboolis_parallel_safe(PlannerInfo*root,Node*node);
4040
externboolcontain_nonstrict_functions(Node*clause);
41+
externboolcontain_exec_param(Node*clause,List*param_ids);
4142
externboolcontain_leaked_vars(Node*clause);
4243

4344
externRelidsfind_nonnullable_rels(Node*clause);

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

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,7 @@ insert into outer_text values ('a', null);
757757
insert into outer_text values ('b', null);
758758
create temp table inner_text (c1 text, c2 text);
759759
insert into inner_text values ('a', null);
760+
insert into inner_text values ('123', '456');
760761
select * from outer_text where (f1, f2) not in (select * from inner_text);
761762
f1 | f2
762763
----+----
@@ -797,6 +798,82 @@ select '1'::text in (select '1'::name union all select '1'::name);
797798
t
798799
(1 row)
799800

801+
--
802+
-- Test that we don't try to use a hashed subplan if the simplified
803+
-- testexpr isn't of the right shape
804+
--
805+
-- this fails by default, of course
806+
select * from int8_tbl where q1 in (select c1 from inner_text);
807+
ERROR: operator does not exist: bigint = text
808+
LINE 1: select * from int8_tbl where q1 in (select c1 from inner_tex...
809+
^
810+
HINT: No operator matches the given name and argument types. You might need to add explicit type casts.
811+
begin;
812+
-- make an operator to allow it to succeed
813+
create function bogus_int8_text_eq(int8, text) returns boolean
814+
language sql as 'select $1::text = $2';
815+
create operator = (procedure=bogus_int8_text_eq, leftarg=int8, rightarg=text);
816+
explain (costs off)
817+
select * from int8_tbl where q1 in (select c1 from inner_text);
818+
QUERY PLAN
819+
--------------------------------
820+
Seq Scan on int8_tbl
821+
Filter: (hashed SubPlan 1)
822+
SubPlan 1
823+
-> Seq Scan on inner_text
824+
(4 rows)
825+
826+
select * from int8_tbl where q1 in (select c1 from inner_text);
827+
q1 | q2
828+
-----+------------------
829+
123 | 456
830+
123 | 4567890123456789
831+
(2 rows)
832+
833+
-- inlining of this function results in unusual number of hash clauses,
834+
-- which we can still cope with
835+
create or replace function bogus_int8_text_eq(int8, text) returns boolean
836+
language sql as 'select $1::text = $2 and $1::text = $2';
837+
explain (costs off)
838+
select * from int8_tbl where q1 in (select c1 from inner_text);
839+
QUERY PLAN
840+
--------------------------------
841+
Seq Scan on int8_tbl
842+
Filter: (hashed SubPlan 1)
843+
SubPlan 1
844+
-> Seq Scan on inner_text
845+
(4 rows)
846+
847+
select * from int8_tbl where q1 in (select c1 from inner_text);
848+
q1 | q2
849+
-----+------------------
850+
123 | 456
851+
123 | 4567890123456789
852+
(2 rows)
853+
854+
-- inlining of this function causes LHS and RHS to be switched,
855+
-- which we can't cope with, so hashing should be abandoned
856+
create or replace function bogus_int8_text_eq(int8, text) returns boolean
857+
language sql as 'select $2 = $1::text';
858+
explain (costs off)
859+
select * from int8_tbl where q1 in (select c1 from inner_text);
860+
QUERY PLAN
861+
--------------------------------------
862+
Seq Scan on int8_tbl
863+
Filter: (SubPlan 1)
864+
SubPlan 1
865+
-> Materialize
866+
-> Seq Scan on inner_text
867+
(5 rows)
868+
869+
select * from int8_tbl where q1 in (select c1 from inner_text);
870+
q1 | q2
871+
-----+------------------
872+
123 | 456
873+
123 | 4567890123456789
874+
(2 rows)
875+
876+
rollback; -- to get rid of the bogus operator
800877
--
801878
-- Test case for planner bug with nested EXISTS handling
802879
--

0 commit comments

Comments
 (0)

[8]ページ先頭

©2009-2025 Movatter.jp