Hi,
On 09/18/2015 03:46 AM, Kyotaro HORIGUCHI wrote:
Hello,
At Thu, 17 Sep 2015 17:40:27 +0200, Tomas Vondra <tomas.von...@2ndquadrant.com> wrote
in <55fadeeb.4000...@2ndquadrant.com>
Yes, this seems sane. I've been poking at this a bit too, and I came
to the same plan in general, except that I think it's better to build
list of clauses that are *not* implied by the index, because that's
what we need both in cost_index and check_index_only.
I intended to isolate IndexOptInfo from belonging RelOptInfo but
the exclusion list also bonds them tightly, and one IndexOptInfo
belongs to only one RelOptInfo so no need to isolate. So
not-implied-restrictclauses in IndexOptInfo would be preferable.
It also seems to me that this change (arguably a bug fix) should
pretty much make the original patch irrelevant, because
check_index_only can simply walk over the new list.
Yeah. This seems to be a bug irrelevant to your index-only-scan
ptch.
Attached is a patch that makes this work correctly, I believe. The only
difference compared to the v1 of the patch is this piece of code in
match_clause_to_index (indexpath.c):
if ((index->indpred != NIL) &&
(predicate_implied_by(lappend(NIL, rinfo->clause), index->indpred)))
continue;
which effectively says that we should ignore clauses implied by the
index predicate.
This fixes the way we build IndexClauseSet for the two indexes -
originally we'd add the implied clause on the large index but not on the
small one (because it does not have the column referenced in the
condition). And as far as I can say, this also fixes all the costing
issues that I've described because cost_index sees the same thing for
both indexes.
It's also early enough to fix the actual path, so EXPLAIN shows the same
thing for both indexes (so no "Index Cond" present for only one of the
indexes).
I've originally tried to fix this in extract_nonindex_conditions, but
that's way too late - it can only fix the costing, not the path.
Arguably this part of the patch is a bugfix of an issue present on master.
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).
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?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 7069f60..d2c9f51 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -86,6 +86,7 @@
#include "optimizer/placeholder.h"
#include "optimizer/plancat.h"
#include "optimizer/planmain.h"
+#include "optimizer/predtest.h"
#include "optimizer/restrictinfo.h"
#include "parser/parsetree.h"
#include "utils/lsyscache.h"
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 9da5444..2d3c0f8 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -1798,13 +1798,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.
*/
/*
@@ -1819,6 +1819,28 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
{
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+ /*
+ * If the index is partial, we won't consider the clauses that are
+ * implied by the index predicate (those are not needed at runtime).
+ */
+ if (index->indpred != NIL)
+ {
+ bool implied = false;
+ List *clauses = NIL;
+
+ /* need a list for the 'implied_by' call */
+ clauses = lappend(clauses, (Node *) rinfo->clause);
+
+ implied = predicate_implied_by(clauses, index->indpred);
+
+ /* we generally don't free memory, but well ... */
+ list_free(clauses);
+
+ /* if the clause is implied by index predicate, skip it */
+ if (implied)
+ continue;
+ }
+
pull_varattnos((Node *) rinfo->clause, rel->relid, &attrs_used);
}
@@ -2138,6 +2160,15 @@ match_clause_to_index(IndexOptInfo *index,
for (indexcol = 0; indexcol < index->ncolumns; 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.
+ */
+ if ((index->indpred != NIL) &&
+ (predicate_implied_by(list_make1(rinfo->clause), index->indpred)))
+ continue;
+
if (match_clause_to_indexcol(index,
indexcol,
rinfo))
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
