Hi,
On 09/26/2015 01:28 PM, Tomas Vondra wrote:
The patch does not change the check_index_only implementation - it
still needs to check the clauses, just like in v1 of the patch. To
make this re-check unnecessary, we'd have to stick the remaining
clauses somewhere, so that check_index_only can use only the filtered
list (instead of walking through the complete list of restrictions).
After thinking about this a bit more, I realized we already have a good
place for keeping this list - IndexClauseSet. So I've done that, and
removed most of the code from check_index_only - it still needs to
decide whether to use the full list of restrictions (regular indexes) or
the filtered list (for partial indexes).
Calling predicate_implied_by in match_clause_to_index makes the check a
bit earlier, compared to the v1 of the patch. So theoretically there
might be cases where we'd interrupt the execution between those places,
and the v1 would not pay the price for the call. But those places are
fairly close together, so I find that unlikely and I've been unable to
reproduce a plausible example of such regression despite trying.
The only regression test this breaks is "aggregates", and I believe
that's actually correct because it changes the plans like this:
-> Index Only Scan Backward using minmaxtest2i on minmaxtest2
Index Cond: (f1 IS NOT NULL)
-> Index Only Scan using minmaxtest3i on minmaxtest3
- Index Cond: (f1 IS NOT NULL)
i.e. it removes the Index Condition from scans of minmaxtest3i, which is
perfectly sensible because the index is defined like this:
create index minmaxtest3i on minmaxtest3(f1) where f1 is not null;
So it's partial index and that condition is implied by the predicate
(it's actually exactly the same).
I haven't fixed those regression tests for now.
Or perhaps we can do the check in match_clause_to_index, pretty much
for free? If the clause is not implied by the index predicate (which
we know thanks to the fix), and we don't assign it to any of the
index columns, it means we can'd use IOS, no?
After thinking about this a bit more, this is clearly nonsense. It fails
on conditions referencing multiple columns (WHERE a=b) which can't be
assigned to a single index column. So the logic would have to be much
more complicated, effectively doing what check_index_only is doing just
a tiny bit later.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 9da5444..d257441 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -59,6 +59,7 @@ typedef struct
bool nonempty; /* True if lists are not all empty */
/* Lists of RestrictInfos, one per index column */
List *indexclauses[INDEX_MAX_KEYS];
+ List *rclauses; /* clauses not implied by predicate */
} IndexClauseSet;
/* Per-path data used within choose_bitmap_and() */
@@ -129,7 +130,7 @@ static PathClauseUsage *classify_index_clause_usage(Path *path,
static Relids get_bitmap_tree_required_outer(Path *bitmapqual);
static void find_indexpath_quals(Path *bitmapqual, List **quals, List **preds);
static int find_list_position(Node *node, List **nodelist);
-static bool check_index_only(RelOptInfo *rel, IndexOptInfo *index);
+static bool check_index_only(RelOptInfo *rel, IndexOptInfo *index, List *clauses);
static double get_loop_count(PlannerInfo *root, Index cur_relid, Relids outer_relids);
static double adjust_rowcount_for_semijoins(PlannerInfo *root,
Index cur_relid,
@@ -1019,7 +1020,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
* index data retrieval anyway.
*/
index_only_scan = (scantype != ST_BITMAPSCAN &&
- check_index_only(rel, index));
+ check_index_only(rel, index, clauses->rclauses));
/*
* 4. Generate an indexscan path if there are relevant restriction clauses
@@ -1782,7 +1783,7 @@ find_list_position(Node *node, List **nodelist)
* Determine whether an index-only scan is possible for this index.
*/
static bool
-check_index_only(RelOptInfo *rel, IndexOptInfo *index)
+check_index_only(RelOptInfo *rel, IndexOptInfo *index, List *clauses)
{
bool result;
Bitmapset *attrs_used = NULL;
@@ -1790,6 +1791,8 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
ListCell *lc;
int i;
+ List *rclauses;
+
/* Index-only scans must be enabled */
if (!enable_indexonlyscan)
return false;
@@ -1798,13 +1801,13 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
* Check that all needed attributes of the relation are available from the
* index.
*
- * XXX this is overly conservative for partial indexes, since we will
- * consider attributes involved in the index predicate as required even
- * though the predicate won't need to be checked at runtime. (The same is
- * true for attributes used only in index quals, if we are certain that
- * the index is not lossy.) However, it would be quite expensive to
- * determine that accurately at this point, so for now we take the easy
- * way out.
+ * For partial indexes we won't consider attributes involved in clauses
+ * implied by the index predicate, as those won't be needed at runtime.
+ *
+ * XXX The same is true for attributes used only in index quals, if we
+ * are certain that the index is not lossy. However, it would be quite
+ * expensive to determine that accurately at this point, so for now we
+ * take the easy way out.
*/
/*
@@ -1814,8 +1817,17 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
*/
pull_varattnos((Node *) rel->reltargetlist, rel->relid, &attrs_used);
- /* Add all the attributes used by restriction clauses. */
- foreach(lc, rel->baserestrictinfo)
+ /*
+ * For partial indexes use the filtered clauses, otherwise use the
+ * baserestrictinfo directly for non-partial indexes.
+ */
+ rclauses = (index->indpred != NIL) ? clauses : rel->baserestrictinfo;
+
+ /*
+ * Add all the attributes used by restriction clauses (only those not
+ * implied by the index predicate for partial indexes).
+ */
+ foreach(lc, rclauses)
{
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
@@ -2136,6 +2148,30 @@ match_clause_to_index(IndexOptInfo *index,
{
int indexcol;
+
+ /*
+ * For partial indexes we skip clauses that are implied by the index
+ * predicate. We don't need to re-evaluate those, and the columns
+ * may not even be in the index itself, just in the predicate.
+ *
+ * We also keep clauses not implied by the index predicate so that
+ * check_index_only does not have to call predicate_implied_by again.
+ * For regular indexes we'll use baserestrictinfo list directly.
+ *
+ * XXX We must do this before trying to match the index to index
+ * columns, because the index predicates may use columns not
+ * used in the index itself.
+ */
+ if (index->indpred != NIL)
+ {
+ if (predicate_implied_by(list_make1(rinfo->clause),
+ index->indpred))
+ return; /* ignore clauses implied by index */
+
+ /* track clauses not implied */
+ clauseset->rclauses = lappend(clauseset->rclauses, rinfo);
+ }
+
for (indexcol = 0; indexcol < index->ncolumns; indexcol++)
{
if (match_clause_to_indexcol(index,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
