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

Commitc5dd8ea

Browse files
committed
More fixes for lossy-GiST-distance-functions patch.
Paul Ramsey reported that commit35fcb1binduced a core dump on commuted ORDER BY expressions, because it wasassuming that the indexorderby expression could be found verbatim in therelevant equivalence class, but it wasn't there. We really don't needanything that complicated anyway; for the data types likely to be used forindex ORDER BY operators in the foreseeable future, the exprType() of theORDER BY expression will serve fine. (The case where we'd have to workharder is where the ORDER BY expression's result is only binary-compatiblewith the declared input type of the ordering operator; long before worryingabout that, one would need to get rid of GiST's hard-wired assumption thatsaid datatype is float8.)Aside from fixing that crash and adding a regression test for the case,I did some desultory code review:nodeIndexscan.c was likewise overthinking how hard it ought to work toidentify the datatype of the ORDER BY expressions.Add comments explaining how come nodeIndexscan.c can get away withsimplifying assumptions about NULLS LAST ordering and no backward scan.Revert no-longer-needed changes of find_ec_member_for_tle(); while thenew definition was no worse than the old, it wasn't better either, andit might cause back-patching pain.Revert entirely bogus additions to genam.h.
1 parentd4b538e commitc5dd8ea

File tree

5 files changed

+100
-58
lines changed

5 files changed

+100
-58
lines changed

‎src/backend/executor/nodeIndexscan.c

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include"executor/execdebug.h"
3131
#include"executor/nodeIndexscan.h"
3232
#include"lib/pairingheap.h"
33+
#include"nodes/nodeFuncs.h"
3334
#include"optimizer/clauses.h"
3435
#include"utils/array.h"
3536
#include"utils/datum.h"
@@ -159,9 +160,18 @@ IndexNextWithReorder(IndexScanState *node)
159160
bool*lastfetched_nulls;
160161
intcmp;
161162

162-
/* only forward scan is supported with reordering. */
163+
/*
164+
* Only forward scan is supported with reordering. Note: we can get away
165+
* with just Asserting here because the system will not try to run the
166+
* plan backwards if ExecSupportsBackwardScan() says it won't work.
167+
* Currently, that is guaranteed because no index AMs support both
168+
* amcanorderbyop and amcanbackward; if any ever do,
169+
* ExecSupportsBackwardScan() will need to consider indexorderbys
170+
* explicitly.
171+
*/
163172
Assert(!ScanDirectionIsBackward(((IndexScan*)node->ss.ps.plan)->indexorderdir));
164173
Assert(ScanDirectionIsForward(node->ss.ps.state->es_direction));
174+
165175
scandesc=node->iss_ScanDesc;
166176
econtext=node->ss.ps.ps_ExprContext;
167177
slot=node->ss.ss_ScanTupleSlot;
@@ -370,7 +380,11 @@ cmp_orderbyvals(const Datum *adist, const bool *anulls,
370380
{
371381
SortSupportssup=&node->iss_SortSupport[i];
372382

373-
/* Handle nulls. We only support NULLS LAST. */
383+
/*
384+
* Handle nulls. We only need to support NULLS LAST ordering, because
385+
* match_pathkeys_to_index() doesn't consider indexorderby
386+
* implementation otherwise.
387+
*/
374388
if (anulls[i]&& !bnulls[i])
375389
return1;
376390
elseif (!anulls[i]&&bnulls[i])
@@ -388,7 +402,7 @@ cmp_orderbyvals(const Datum *adist, const bool *anulls,
388402

389403
/*
390404
* Pairing heap provides getting topmost (greatest) element while KNN provides
391-
* ascending sort. That's why weinverse the sort order.
405+
* ascending sort. That's why weinvert the sort order.
392406
*/
393407
staticint
394408
reorderqueue_cmp(constpairingheap_node*a,constpairingheap_node*b,
@@ -917,46 +931,41 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
917931
{
918932
intnumOrderByKeys=indexstate->iss_NumOrderByKeys;
919933
inti;
920-
ListCell*lc;
934+
ListCell*lco;
935+
ListCell*lcx;
921936

922937
/*
923-
* Prepare sort support, and look up thedistance type for each ORDER
924-
*BYexpression.
938+
* Prepare sort support, and look up thedata type for each ORDER BY
939+
* expression.
925940
*/
926941
Assert(numOrderByKeys==list_length(node->indexorderbyops));
927-
indexstate->iss_SortSupport=
942+
Assert(numOrderByKeys==list_length(node->indexorderbyorig));
943+
indexstate->iss_SortSupport= (SortSupportData*)
928944
palloc0(numOrderByKeys*sizeof(SortSupportData));
929-
indexstate->iss_OrderByTypByVals=
945+
indexstate->iss_OrderByTypByVals= (bool*)
930946
palloc(numOrderByKeys*sizeof(bool));
931-
indexstate->iss_OrderByTypLens=
947+
indexstate->iss_OrderByTypLens= (int16*)
932948
palloc(numOrderByKeys*sizeof(int16));
933949
i=0;
934-
foreach(lc,node->indexorderbyops)
950+
forboth(lco,node->indexorderbyops,lcx,node->indexorderbyorig)
935951
{
936-
Oidorderbyop=lfirst_oid(lc);
937-
OidorderbyType;
938-
Oidopfamily;
939-
int16strategy;
952+
Oidorderbyop=lfirst_oid(lco);
953+
Node*orderbyexpr= (Node*)lfirst(lcx);
954+
OidorderbyType=exprType(orderbyexpr);
940955

941956
PrepareSortSupportFromOrderingOp(orderbyop,
942957
&indexstate->iss_SortSupport[i]);
943-
944-
if (!get_ordering_op_properties(orderbyop,
945-
&opfamily,&orderbyType,&strategy))
946-
elog(ERROR,"operator %u is not a valid ordering operator",
947-
orderbyop);
948-
949958
get_typlenbyval(orderbyType,
950959
&indexstate->iss_OrderByTypLens[i],
951960
&indexstate->iss_OrderByTypByVals[i]);
952961
i++;
953962
}
954963

955964
/* allocate arrays to hold the re-calculated distances */
956-
indexstate->iss_OrderByValues=
957-
palloc(indexstate->iss_NumOrderByKeys*sizeof(Datum));
958-
indexstate->iss_OrderByNulls=
959-
palloc(indexstate->iss_NumOrderByKeys*sizeof(bool));
965+
indexstate->iss_OrderByValues= (Datum*)
966+
palloc(numOrderByKeys*sizeof(Datum));
967+
indexstate->iss_OrderByNulls= (bool*)
968+
palloc(numOrderByKeys*sizeof(bool));
960969

961970
/* and initialize the reorder queue */
962971
indexstate->iss_ReorderQueue=pairingheap_allocate(reorderqueue_cmp,

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

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#include"access/stratnum.h"
2323
#include"access/sysattr.h"
2424
#include"catalog/pg_class.h"
25-
#include"catalog/pg_operator.h"
2625
#include"foreign/fdwapi.h"
2726
#include"miscadmin.h"
2827
#include"nodes/makefuncs.h"
@@ -172,8 +171,8 @@ static Plan *prepare_sort_from_pathkeys(PlannerInfo *root,
172171
Oid**p_sortOperators,
173172
Oid**p_collations,
174173
bool**p_nullsFirst);
175-
staticEquivalenceMember*find_ec_member_for_expr(EquivalenceClass*ec,
176-
Expr*tlexpr,
174+
staticEquivalenceMember*find_ec_member_for_tle(EquivalenceClass*ec,
175+
TargetEntry*tle,
177176
Relidsrelids);
178177
staticMaterial*make_material(Plan*lefttree);
179178

@@ -1325,36 +1324,35 @@ create_indexscan_plan(PlannerInfo *root,
13251324
}
13261325

13271326
/*
1328-
* If there are ORDER BY expressions, look up the sort operators for
1329-
*their datatypes.
1327+
* If there are ORDER BY expressions, look up the sort operators for their
1328+
*result datatypes.
13301329
*/
1331-
if (best_path->path.pathkeys&&indexorderbys)
1330+
if (indexorderbys)
13321331
{
13331332
ListCell*pathkeyCell,
13341333
*exprCell;
13351334

13361335
/*
1337-
* PathKey contains pointer to the equivalence class, but that's not
1338-
* enough because we need the expression's datatype to look up the
1339-
* sort operator in the operator family. We have to dig out the
1340-
* equivalence member for the datatype.
1336+
* PathKey contains OID of the btree opfamily we're sorting by, but
1337+
* that's not quite enough because we need the expression's datatype
1338+
* to look up the sort operator in the operator family.
13411339
*/
1340+
Assert(list_length(best_path->path.pathkeys)==list_length(indexorderbys));
13421341
forboth(pathkeyCell,best_path->path.pathkeys,exprCell,indexorderbys)
13431342
{
13441343
PathKey*pathkey= (PathKey*)lfirst(pathkeyCell);
1345-
Expr*expr= (Expr*)lfirst(exprCell);
1346-
EquivalenceMember*em;
1347-
1348-
/* Find equivalence member for the order by expression */
1349-
em=find_ec_member_for_expr(pathkey->pk_eclass,expr,NULL);
1344+
Node*expr= (Node*)lfirst(exprCell);
1345+
Oidexprtype=exprType(expr);
1346+
Oidsortop;
13501347

13511348
/* Get sort operator from opfamily */
1352-
indexorderbyops=
1353-
lappend_oid(indexorderbyops,
1354-
get_opfamily_member(pathkey->pk_opfamily,
1355-
em->em_datatype,
1356-
em->em_datatype,
1357-
pathkey->pk_strategy));
1349+
sortop=get_opfamily_member(pathkey->pk_opfamily,
1350+
exprtype,
1351+
exprtype,
1352+
pathkey->pk_strategy);
1353+
if (!OidIsValid(sortop))
1354+
elog(ERROR,"failed to find sort operator for ORDER BY expression");
1355+
indexorderbyops=lappend_oid(indexorderbyops,sortop);
13581356
}
13591357
}
13601358

@@ -4100,7 +4098,7 @@ prepare_sort_from_pathkeys(PlannerInfo *root, Plan *lefttree, List *pathkeys,
41004098
tle=get_tle_by_resno(tlist,reqColIdx[numsortkeys]);
41014099
if (tle)
41024100
{
4103-
em=find_ec_member_for_expr(ec,tle->expr,relids);
4101+
em=find_ec_member_for_tle(ec,tle,relids);
41044102
if (em)
41054103
{
41064104
/* found expr at right place in tlist */
@@ -4131,7 +4129,7 @@ prepare_sort_from_pathkeys(PlannerInfo *root, Plan *lefttree, List *pathkeys,
41314129
foreach(j,tlist)
41324130
{
41334131
tle= (TargetEntry*)lfirst(j);
4134-
em=find_ec_member_for_expr(ec,tle->expr,relids);
4132+
em=find_ec_member_for_tle(ec,tle,relids);
41354133
if (em)
41364134
{
41374135
/* found expr already in tlist */
@@ -4252,21 +4250,23 @@ prepare_sort_from_pathkeys(PlannerInfo *root, Plan *lefttree, List *pathkeys,
42524250
}
42534251

42544252
/*
4255-
*find_ec_member_for_expr
4256-
*Locate an EquivalenceClass member matching the givenexpression, if any
4253+
*find_ec_member_for_tle
4254+
*Locate an EquivalenceClass member matching the givenTLE, if any
42574255
*
42584256
* Child EC members are ignored unless they match 'relids'.
42594257
*/
42604258
staticEquivalenceMember*
4261-
find_ec_member_for_expr(EquivalenceClass*ec,
4262-
Expr*expr,
4263-
Relidsrelids)
4259+
find_ec_member_for_tle(EquivalenceClass*ec,
4260+
TargetEntry*tle,
4261+
Relidsrelids)
42644262
{
4263+
Expr*tlexpr;
42654264
ListCell*lc;
42664265

42674266
/* We ignore binary-compatible relabeling on both ends */
4268-
while (expr&&IsA(expr,RelabelType))
4269-
expr= ((RelabelType*)expr)->arg;
4267+
tlexpr=tle->expr;
4268+
while (tlexpr&&IsA(tlexpr,RelabelType))
4269+
tlexpr= ((RelabelType*)tlexpr)->arg;
42704270

42714271
foreach(lc,ec->ec_members)
42724272
{
@@ -4292,7 +4292,7 @@ find_ec_member_for_expr(EquivalenceClass *ec,
42924292
while (emexpr&&IsA(emexpr,RelabelType))
42934293
emexpr= ((RelabelType*)emexpr)->arg;
42944294

4295-
if (equal(emexpr,expr))
4295+
if (equal(emexpr,tlexpr))
42964296
returnem;
42974297
}
42984298

‎src/include/access/genam.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,7 @@ extern void index_restrpos(IndexScanDesc scan);
147147
externItemPointerindex_getnext_tid(IndexScanDescscan,
148148
ScanDirectiondirection);
149149
externHeapTupleindex_fetch_heap(IndexScanDescscan);
150-
externboolindex_get_heap_values(IndexScanDescscan,ItemPointerheapPtr,
151-
Datumvalues[INDEX_MAX_KEYS],boolisnull[INDEX_MAX_KEYS]);
152150
externHeapTupleindex_getnext(IndexScanDescscan,ScanDirectiondirection);
153-
154151
externint64index_getbitmap(IndexScanDescscan,TIDBitmap*bitmap);
155152

156153
externIndexBulkDeleteResult*index_bulk_delete(IndexVacuumInfo*info,

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,34 @@ order by p <-> point(0.2, 0.2);
8686
(0.5,0.5)
8787
(11 rows)
8888

89+
-- Check commuted case as well
90+
explain (costs off)
91+
select p from gist_tbl where p <@ box(point(0,0), point(0.5, 0.5))
92+
order by point(0.1, 0.1) <-> p;
93+
QUERY PLAN
94+
--------------------------------------------------------
95+
Index Only Scan using gist_tbl_point_index on gist_tbl
96+
Index Cond: (p <@ '(0.5,0.5),(0,0)'::box)
97+
Order By: (p <-> '(0.1,0.1)'::point)
98+
(3 rows)
99+
100+
select p from gist_tbl where p <@ box(point(0,0), point(0.5, 0.5))
101+
order by point(0.1, 0.1) <-> p;
102+
p
103+
-------------
104+
(0.1,0.1)
105+
(0.15,0.15)
106+
(0.05,0.05)
107+
(0,0)
108+
(0.2,0.2)
109+
(0.25,0.25)
110+
(0.3,0.3)
111+
(0.35,0.35)
112+
(0.4,0.4)
113+
(0.45,0.45)
114+
(0.5,0.5)
115+
(11 rows)
116+
89117
drop index gist_tbl_point_index;
90118
-- Test index-only scan with box opclass
91119
create index gist_tbl_box_index on gist_tbl using gist (b);

‎src/test/regress/sql/gist.sql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,14 @@ order by p <-> point(0.2, 0.2);
6161
select pfrom gist_tblwhere p<@box(point(0,0),point(0.5,0.5))
6262
order by p<->point(0.2,0.2);
6363

64+
-- Check commuted case as well
65+
explain (costs off)
66+
select pfrom gist_tblwhere p<@box(point(0,0),point(0.5,0.5))
67+
order bypoint(0.1,0.1)<-> p;
68+
69+
select pfrom gist_tblwhere p<@box(point(0,0),point(0.5,0.5))
70+
order bypoint(0.1,0.1)<-> p;
71+
6472
dropindex gist_tbl_point_index;
6573

6674
-- Test index-only scan with box opclass

0 commit comments

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