From c5dd8ead403f85bd041590d2e3e79b72830472d4 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 21 May 2015 19:47:48 -0400
Subject: [PATCH] More fixes for lossy-GiST-distance-functions patch.
Paul Ramsey reported that commit 35fcb1b3d038a501f3f4c87c05630095abaaadab
induced a core dump on commuted ORDER BY expressions, because it was
assuming that the indexorderby expression could be found verbatim in the
relevant equivalence class, but it wasn't there. We really don't need
anything that complicated anyway; for the data types likely to be used for
index ORDER BY operators in the foreseeable future, the exprType() of the
ORDER BY expression will serve fine. (The case where we'd have to work
harder is where the ORDER BY expression's result is only binary-compatible
with the declared input type of the ordering operator; long before worrying
about that, one would need to get rid of GiST's hard-wired assumption that
said 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 to
identify the datatype of the ORDER BY expressions.
Add comments explaining how come nodeIndexscan.c can get away with
simplifying assumptions about NULLS LAST ordering and no backward scan.
Revert no-longer-needed changes of find_ec_member_for_tle(); while the
new definition was no worse than the old, it wasn't better either, and
it might cause back-patching pain.
Revert entirely bogus additions to genam.h.
---
src/backend/executor/nodeIndexscan.c | 57 +++++++++++++----------
src/backend/optimizer/plan/createplan.c | 62 ++++++++++++-------------
src/include/access/genam.h | 3 --
src/test/regress/expected/gist.out | 28 +++++++++++
src/test/regress/sql/gist.sql | 8 ++++
5 files changed, 100 insertions(+), 58 deletions(-)
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index 96d34beaf43..d79c1aa8ca9 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -30,6 +30,7 @@
#include "executor/execdebug.h"
#include "executor/nodeIndexscan.h"
#include "lib/pairingheap.h"
+#include "nodes/nodeFuncs.h"
#include "optimizer/clauses.h"
#include "utils/array.h"
#include "utils/datum.h"
@@ -159,9 +160,18 @@ IndexNextWithReorder(IndexScanState *node)
bool *lastfetched_nulls;
int cmp;
- /* only forward scan is supported with reordering. */
+ /*
+ * Only forward scan is supported with reordering. Note: we can get away
+ * with just Asserting here because the system will not try to run the
+ * plan backwards if ExecSupportsBackwardScan() says it won't work.
+ * Currently, that is guaranteed because no index AMs support both
+ * amcanorderbyop and amcanbackward; if any ever do,
+ * ExecSupportsBackwardScan() will need to consider indexorderbys
+ * explicitly.
+ */
Assert(!ScanDirectionIsBackward(((IndexScan *) node->ss.ps.plan)->indexorderdir));
Assert(ScanDirectionIsForward(node->ss.ps.state->es_direction));
+
scandesc = node->iss_ScanDesc;
econtext = node->ss.ps.ps_ExprContext;
slot = node->ss.ss_ScanTupleSlot;
@@ -370,7 +380,11 @@ cmp_orderbyvals(const Datum *adist, const bool *anulls,
{
SortSupport ssup = &node->iss_SortSupport[i];
- /* Handle nulls. We only support NULLS LAST. */
+ /*
+ * Handle nulls. We only need to support NULLS LAST ordering, because
+ * match_pathkeys_to_index() doesn't consider indexorderby
+ * implementation otherwise.
+ */
if (anulls[i] && !bnulls[i])
return 1;
else if (!anulls[i] && bnulls[i])
@@ -388,7 +402,7 @@ cmp_orderbyvals(const Datum *adist, const bool *anulls,
/*
* Pairing heap provides getting topmost (greatest) element while KNN provides
- * ascending sort. That's why we inverse the sort order.
+ * ascending sort. That's why we invert the sort order.
*/
static int
reorderqueue_cmp(const pairingheap_node *a, const pairingheap_node *b,
@@ -917,35 +931,30 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
{
int numOrderByKeys = indexstate->iss_NumOrderByKeys;
int i;
- ListCell *lc;
+ ListCell *lco;
+ ListCell *lcx;
/*
- * Prepare sort support, and look up the distance type for each ORDER
- * BY expression.
+ * Prepare sort support, and look up the data type for each ORDER BY
+ * expression.
*/
Assert(numOrderByKeys == list_length(node->indexorderbyops));
- indexstate->iss_SortSupport =
+ Assert(numOrderByKeys == list_length(node->indexorderbyorig));
+ indexstate->iss_SortSupport = (SortSupportData *)
palloc0(numOrderByKeys * sizeof(SortSupportData));
- indexstate->iss_OrderByTypByVals =
+ indexstate->iss_OrderByTypByVals = (bool *)
palloc(numOrderByKeys * sizeof(bool));
- indexstate->iss_OrderByTypLens =
+ indexstate->iss_OrderByTypLens = (int16 *)
palloc(numOrderByKeys * sizeof(int16));
i = 0;
- foreach(lc, node->indexorderbyops)
+ forboth(lco, node->indexorderbyops, lcx, node->indexorderbyorig)
{
- Oid orderbyop = lfirst_oid(lc);
- Oid orderbyType;
- Oid opfamily;
- int16 strategy;
+ Oid orderbyop = lfirst_oid(lco);
+ Node *orderbyexpr = (Node *) lfirst(lcx);
+ Oid orderbyType = exprType(orderbyexpr);
PrepareSortSupportFromOrderingOp(orderbyop,
&indexstate->iss_SortSupport[i]);
-
- if (!get_ordering_op_properties(orderbyop,
- &opfamily, &orderbyType, &strategy))
- elog(ERROR, "operator %u is not a valid ordering operator",
- orderbyop);
-
get_typlenbyval(orderbyType,
&indexstate->iss_OrderByTypLens[i],
&indexstate->iss_OrderByTypByVals[i]);
@@ -953,10 +962,10 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
}
/* allocate arrays to hold the re-calculated distances */
- indexstate->iss_OrderByValues =
- palloc(indexstate->iss_NumOrderByKeys * sizeof(Datum));
- indexstate->iss_OrderByNulls =
- palloc(indexstate->iss_NumOrderByKeys * sizeof(bool));
+ indexstate->iss_OrderByValues = (Datum *)
+ palloc(numOrderByKeys * sizeof(Datum));
+ indexstate->iss_OrderByNulls = (bool *)
+ palloc(numOrderByKeys * sizeof(bool));
/* and initialize the reorder queue */
indexstate->iss_ReorderQueue = pairingheap_allocate(reorderqueue_cmp,
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 36015ea6c4b..b47ef466dc1 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -22,7 +22,6 @@
#include "access/stratnum.h"
#include "access/sysattr.h"
#include "catalog/pg_class.h"
-#include "catalog/pg_operator.h"
#include "foreign/fdwapi.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
@@ -172,8 +171,8 @@ static Plan *prepare_sort_from_pathkeys(PlannerInfo *root,
Oid **p_sortOperators,
Oid **p_collations,
bool **p_nullsFirst);
-static EquivalenceMember *find_ec_member_for_expr(EquivalenceClass *ec,
- Expr *tlexpr,
+static EquivalenceMember *find_ec_member_for_tle(EquivalenceClass *ec,
+ TargetEntry *tle,
Relids relids);
static Material *make_material(Plan *lefttree);
@@ -1325,36 +1324,35 @@ create_indexscan_plan(PlannerInfo *root,
}
/*
- * If there are ORDER BY expressions, look up the sort operators for
- * their datatypes.
+ * If there are ORDER BY expressions, look up the sort operators for their
+ * result datatypes.
*/
- if (best_path->path.pathkeys && indexorderbys)
+ if (indexorderbys)
{
ListCell *pathkeyCell,
*exprCell;
/*
- * PathKey contains pointer to the equivalence class, but that's not
- * enough because we need the expression's datatype to look up the
- * sort operator in the operator family. We have to dig out the
- * equivalence member for the datatype.
+ * PathKey contains OID of the btree opfamily we're sorting by, but
+ * that's not quite enough because we need the expression's datatype
+ * to look up the sort operator in the operator family.
*/
+ Assert(list_length(best_path->path.pathkeys) == list_length(indexorderbys));
forboth(pathkeyCell, best_path->path.pathkeys, exprCell, indexorderbys)
{
PathKey *pathkey = (PathKey *) lfirst(pathkeyCell);
- Expr *expr = (Expr *) lfirst(exprCell);
- EquivalenceMember *em;
-
- /* Find equivalence member for the order by expression */
- em = find_ec_member_for_expr(pathkey->pk_eclass, expr, NULL);
+ Node *expr = (Node *) lfirst(exprCell);
+ Oid exprtype = exprType(expr);
+ Oid sortop;
/* Get sort operator from opfamily */
- indexorderbyops =
- lappend_oid(indexorderbyops,
- get_opfamily_member(pathkey->pk_opfamily,
- em->em_datatype,
- em->em_datatype,
- pathkey->pk_strategy));
+ sortop = get_opfamily_member(pathkey->pk_opfamily,
+ exprtype,
+ exprtype,
+ pathkey->pk_strategy);
+ if (!OidIsValid(sortop))
+ elog(ERROR, "failed to find sort operator for ORDER BY expression");
+ indexorderbyops = lappend_oid(indexorderbyops, sortop);
}
}
@@ -4100,7 +4098,7 @@ prepare_sort_from_pathkeys(PlannerInfo *root, Plan *lefttree, List *pathkeys,
tle = get_tle_by_resno(tlist, reqColIdx[numsortkeys]);
if (tle)
{
- em = find_ec_member_for_expr(ec, tle->expr, relids);
+ em = find_ec_member_for_tle(ec, tle, relids);
if (em)
{
/* found expr at right place in tlist */
@@ -4131,7 +4129,7 @@ prepare_sort_from_pathkeys(PlannerInfo *root, Plan *lefttree, List *pathkeys,
foreach(j, tlist)
{
tle = (TargetEntry *) lfirst(j);
- em = find_ec_member_for_expr(ec, tle->expr, relids);
+ em = find_ec_member_for_tle(ec, tle, relids);
if (em)
{
/* found expr already in tlist */
@@ -4252,21 +4250,23 @@ prepare_sort_from_pathkeys(PlannerInfo *root, Plan *lefttree, List *pathkeys,
}
/*
- * find_ec_member_for_expr
- * Locate an EquivalenceClass member matching the given expression, if any
+ * find_ec_member_for_tle
+ * Locate an EquivalenceClass member matching the given TLE, if any
*
* Child EC members are ignored unless they match 'relids'.
*/
static EquivalenceMember *
-find_ec_member_for_expr(EquivalenceClass *ec,
- Expr *expr,
- Relids relids)
+find_ec_member_for_tle(EquivalenceClass *ec,
+ TargetEntry *tle,
+ Relids relids)
{
+ Expr *tlexpr;
ListCell *lc;
/* We ignore binary-compatible relabeling on both ends */
- while (expr && IsA(expr, RelabelType))
- expr = ((RelabelType *) expr)->arg;
+ tlexpr = tle->expr;
+ while (tlexpr && IsA(tlexpr, RelabelType))
+ tlexpr = ((RelabelType *) tlexpr)->arg;
foreach(lc, ec->ec_members)
{
@@ -4292,7 +4292,7 @@ find_ec_member_for_expr(EquivalenceClass *ec,
while (emexpr && IsA(emexpr, RelabelType))
emexpr = ((RelabelType *) emexpr)->arg;
- if (equal(emexpr, expr))
+ if (equal(emexpr, tlexpr))
return em;
}
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index f129c4b58f9..d86590ac111 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -147,10 +147,7 @@ extern void index_restrpos(IndexScanDesc scan);
extern ItemPointer index_getnext_tid(IndexScanDesc scan,
ScanDirection direction);
extern HeapTuple index_fetch_heap(IndexScanDesc scan);
-extern bool index_get_heap_values(IndexScanDesc scan, ItemPointer heapPtr,
- Datum values[INDEX_MAX_KEYS], bool isnull[INDEX_MAX_KEYS]);
extern HeapTuple index_getnext(IndexScanDesc scan, ScanDirection direction);
-
extern int64 index_getbitmap(IndexScanDesc scan, TIDBitmap *bitmap);
extern IndexBulkDeleteResult *index_bulk_delete(IndexVacuumInfo *info,
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index 42f6891ffee..c2ab9c92c38 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -86,6 +86,34 @@ order by p <-> point(0.2, 0.2);
(0.5,0.5)
(11 rows)
+-- Check commuted case as well
+explain (costs off)
+select p from gist_tbl where p <@ box(point(0,0), point(0.5, 0.5))
+order by point(0.1, 0.1) <-> p;
+ QUERY PLAN
+--------------------------------------------------------
+ Index Only Scan using gist_tbl_point_index on gist_tbl
+ Index Cond: (p <@ '(0.5,0.5),(0,0)'::box)
+ Order By: (p <-> '(0.1,0.1)'::point)
+(3 rows)
+
+select p from gist_tbl where p <@ box(point(0,0), point(0.5, 0.5))
+order by point(0.1, 0.1) <-> p;
+ p
+-------------
+ (0.1,0.1)
+ (0.15,0.15)
+ (0.05,0.05)
+ (0,0)
+ (0.2,0.2)
+ (0.25,0.25)
+ (0.3,0.3)
+ (0.35,0.35)
+ (0.4,0.4)
+ (0.45,0.45)
+ (0.5,0.5)
+(11 rows)
+
drop index gist_tbl_point_index;
-- Test index-only scan with box opclass
create index gist_tbl_box_index on gist_tbl using gist (b);
diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql
index d6cbc21717f..8b87972b4b8 100644
--- a/src/test/regress/sql/gist.sql
+++ b/src/test/regress/sql/gist.sql
@@ -61,6 +61,14 @@ order by p <-> point(0.2, 0.2);
select p from gist_tbl where p <@ box(point(0,0), point(0.5, 0.5))
order by p <-> point(0.2, 0.2);
+-- Check commuted case as well
+explain (costs off)
+select p from gist_tbl where p <@ box(point(0,0), point(0.5, 0.5))
+order by point(0.1, 0.1) <-> p;
+
+select p from gist_tbl where p <@ box(point(0,0), point(0.5, 0.5))
+order by point(0.1, 0.1) <-> p;
+
drop index gist_tbl_point_index;
-- Test index-only scan with box opclass
--
GitLab