From d18e334c6547582ef93e3156ddaba03ee29757da Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 18 May 2006 17:12:10 +0000 Subject: [PATCH] Fix thinko in recent changes to handle ScalarArrayOpExpr as an indexable condition: when there are multiple possible index paths involving ScalarArrayOpExprs, they are logically to be ANDed together not ORed. This thinko was a direct consequence of trying to put the processing inside generate_bitmap_or_paths(), which I now see was a bit too cute. So pull it out and make the callers do it separately (there are only two that need it anyway). Partially responds to bug #2441 from Arjen van der Meijden. There are some additional infelicities exposed by his example, but they are also in 8.1.x, while this mistake is not. --- src/backend/optimizer/path/indxpath.c | 118 +++++++++++++++++++++------------- 1 file changed, 74 insertions(+), 44 deletions(-) diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index f5c4dcf18a..935494e077 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -9,7 +9,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.204 2006/04/09 18:18:41 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.205 2006/05/18 17:12:10 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -50,6 +50,10 @@ static List *find_usable_indexes(PlannerInfo *root, RelOptInfo *rel, bool istoplevel, bool isjoininner, Relids outer_relids, SaOpControl saop_control); +static List *find_saop_paths(PlannerInfo *root, RelOptInfo *rel, + List *clauses, List *outer_clauses, + bool istoplevel, bool isjoininner, + Relids outer_relids); static Path *choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel, List *paths); static int bitmap_path_comparator(const void *a, const void *b); static Cost bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel, List *paths); @@ -190,6 +194,15 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel) bitindexpaths = list_concat(bitindexpaths, indexpaths); /* + * Likewise, generate paths using ScalarArrayOpExpr clauses; these can't + * be simple indexscans but they can be used in bitmap scans. + */ + indexpaths = find_saop_paths(root, rel, + rel->baserestrictinfo, NIL, + true, false, NULL); + bitindexpaths = list_concat(bitindexpaths, indexpaths); + + /* * If we found anything usable, generate a BitmapHeapPath for the most * promising combination of bitmap index paths. */ @@ -279,10 +292,6 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel, * predOK index to an arm of an OR, which would be a legal but * pointlessly inefficient plan. (A better plan will be generated by * just scanning the predOK index alone, no OR.) - * - * If saop_control is SAOP_REQUIRE and istoplevel is false, the caller - * is only interested in indexquals involving ScalarArrayOps, so don't - * set useful_predicate to true. */ useful_predicate = false; if (index->indpred != NIL) @@ -308,8 +317,7 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel, if (!predicate_implied_by(index->indpred, all_clauses)) continue; /* can't use it at all */ - if (saop_control != SAOP_REQUIRE && - !predicate_implied_by(index->indpred, outer_clauses)) + if (!predicate_implied_by(index->indpred, outer_clauses)) useful_predicate = true; } } @@ -399,10 +407,53 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel, /* + * find_saop_paths + * Find all the potential indexpaths that make use of ScalarArrayOpExpr + * clauses. The executor only supports these in bitmap scans, not + * plain indexscans, so we need to segregate them from the normal case. + * Otherwise, same API as find_usable_indexes(). + * Returns a list of IndexPaths. + */ +static List * +find_saop_paths(PlannerInfo *root, RelOptInfo *rel, + List *clauses, List *outer_clauses, + bool istoplevel, bool isjoininner, + Relids outer_relids) +{ + bool have_saop = false; + ListCell *l; + + /* + * Since find_usable_indexes is relatively expensive, don't bother to + * run it unless there are some top-level ScalarArrayOpExpr clauses. + */ + foreach(l, clauses) + { + RestrictInfo *rinfo = (RestrictInfo *) lfirst(l); + + Assert(IsA(rinfo, RestrictInfo)); + if (IsA(rinfo->clause, ScalarArrayOpExpr)) + { + have_saop = true; + break; + } + } + if (!have_saop) + return NIL; + + return find_usable_indexes(root, rel, + clauses, outer_clauses, + istoplevel, isjoininner, + outer_relids, + SAOP_REQUIRE); +} + + +/* * generate_bitmap_or_paths - * Look through the list of clauses to find OR clauses and - * ScalarArrayOpExpr clauses, and generate a BitmapOrPath for each one - * we can handle that way. Return a list of the generated BitmapOrPaths. + * Look through the list of clauses to find OR clauses, and generate + * a BitmapOrPath for each one we can handle that way. Return a list + * of the generated BitmapOrPaths. * * outer_clauses is a list of additional clauses that can be assumed true * for the purpose of generating indexquals, but are not to be searched for @@ -416,7 +467,6 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel, { List *result = NIL; List *all_clauses; - bool have_saop = false; ListCell *l; /* @@ -433,16 +483,9 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel, ListCell *j; Assert(IsA(rinfo, RestrictInfo)); - /* - * In this loop we ignore RestrictInfos that aren't ORs; but take - * note of ScalarArrayOpExpr for later. - */ + /* Ignore RestrictInfos that aren't ORs */ if (!restriction_is_or_clause(rinfo)) - { - if (IsA(rinfo->clause, ScalarArrayOpExpr)) - have_saop = true; continue; - } /* * We must be able to match at least one index to each of the arms of @@ -516,29 +559,6 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel, } } - /* - * If we saw any top-level ScalarArrayOpExpr clauses, see if we can - * generate a bitmap index path that uses those but not any OR clauses. - */ - if (have_saop) - { - List *pathlist; - Path *bitmapqual; - - pathlist = find_usable_indexes(root, rel, - clauses, - outer_clauses, - false, - isjoininner, - outer_relids, - SAOP_REQUIRE); - if (pathlist != NIL) - { - bitmapqual = (Path *) create_bitmap_or_path(root, rel, pathlist); - result = lappend(result, bitmapqual); - } - } - return result; } @@ -1429,8 +1449,8 @@ best_inner_indexscan(PlannerInfo *root, RelOptInfo *rel, SAOP_FORBID); /* - * Generate BitmapOrPaths for any suitable OR-clauses or ScalarArrayOpExpr - * clauses present in the clause list. + * Generate BitmapOrPaths for any suitable OR-clauses present in the + * clause list. */ bitindexpaths = generate_bitmap_or_paths(root, rel, clause_list, NIL, @@ -1438,6 +1458,16 @@ best_inner_indexscan(PlannerInfo *root, RelOptInfo *rel, outer_relids); /* + * Likewise, generate paths using ScalarArrayOpExpr clauses; these can't + * be simple indexscans but they can be used in bitmap scans. + */ + bitindexpaths = list_concat(bitindexpaths, + find_saop_paths(root, rel, + clause_list, NIL, + false, true, + outer_relids)); + + /* * Include the regular index paths in bitindexpaths. */ bitindexpaths = list_concat(bitindexpaths, list_copy(indexpaths)); -- 2.11.0