Hello,
> The attached patch applies on the latest v5 patch and will
> address above issues. (And modifies expected files, which are the
> manifestation of this improovement).
As you see, it is a quite bad choice. Ugly and unreadable and
fragile.
The cause of this seeming mismatch would be the place to hold
indexrinfos. It is determined only by baserestrictinfo and
indpred. Any other components are not involved. So IndexClauseSet
is found not to be the best place after all, I suppose.
Instead, I came to think that the better place is
IndexOptInfo. Partial indexes are examined in check_partial_index
and it seems to be the most proper place to check this so far.
Thougts?
regards,
At Tue, 06 Oct 2015 15:12:02 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
<horiguchi.kyot...@lab.ntt.co.jp> wrote in
<20151006.151202.58051959.horiguchi.kyot...@lab.ntt.co.jp>
> Hello,
>
> At Thu, 01 Oct 2015 01:36:51 +0200, Tomas Vondra
> <tomas.von...@2ndquadrant.com> wrote in <560c7213.3010...@2ndquadrant.com>
> > > Good point. I think we may simply point indexrinfos to the existing
> > > list
> > > of restrictions in that case - we don't need to copy it. So no
> > > additional memory / CPU consumption, and it should make the code
> > > working
> > > with it a bit simpler.
> >
> > Attached is v5 of the patch improving the comments (rephrasing, moving
> > before function etc.).
>
> Looks good (for me).
>
> > I also attempted to fix the issue with empty list for non-partial
> > indexes by simply setting it to rel->baserestrictinfo in
> > match_restriction_clauses_to_index (for non-partial indexes),
>
> Although it is not the cause of these errors (or any errors on
> regress), build_paths_for_OR (which doesn't seem to be called in
> regression tests) doesn't set indexrinfos
> properly. match_clauses_to_index can commonize(?) the process in
> return for additional list copy and concat. The attached diff is
> doing in the latter method. Avoiding the copies will make the
> code a bit complicated.
>
> But anyway regtests doesn't say whether it is sane or not:( (TODO)
>
> > but that
> > results in a bunch of
> >
> > ERROR: variable not found in subplan target list
> >
> > during "make installcheck". I can't quite wrap my head around why.
>
> During considerartion on parameterized joins in
> get_join_index_paths, caluseset fed to get_index_paths is
> generated from given join (j|e)clausesets. So completing the
> clauseset with necessary restrictinfo from rcaluseset fixes the
> problem.
>
> The attached patch applies on the latest v5 patch and will
> address above issues. (And modifies expected files, which are the
> manifestation of this improovement).
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 9801a9e83a108cead33c843021d604fc8b5defcc Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Wed, 30 Sep 2015 09:04:36 +0900
Subject: [PATCH] Use IndexOptInfo to store index rinfos.
Instead of IndexClauseSet, this patch stores rinfos excluding clauses
implied by index predicates (indexrinfos) into IndexOptInfo. The
reason to do there is that the indexrinfos are determined only by base
restrictinfos on parent relation and index predicates. IndexClauseSet
is used several places irrelevant to such things.
---
src/backend/optimizer/path/costsize.c | 5 ++-
src/backend/optimizer/path/indxpath.c | 66 +++++++++++++++++++++++++++-----
src/backend/optimizer/util/pathnode.c | 1 +
src/include/nodes/relation.h | 7 ++++
src/test/regress/expected/aggregates.out | 8 +---
5 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 1b61fd9..ce042ac 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -88,6 +88,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"
@@ -383,7 +384,7 @@ cost_index(IndexPath *path, PlannerInfo *root, double loop_count)
path->path.rows = path->path.param_info->ppi_rows;
/* qpquals come from the rel's restriction clauses and ppi_clauses */
qpquals = list_concat(
- extract_nonindex_conditions(baserel->baserestrictinfo,
+ extract_nonindex_conditions(path->indexrinfos,
path->indexquals),
extract_nonindex_conditions(path->path.param_info->ppi_clauses,
path->indexquals));
@@ -392,7 +393,7 @@ cost_index(IndexPath *path, PlannerInfo *root, double loop_count)
{
path->path.rows = baserel->rows;
/* qpquals come from just the rel's restriction clauses */
- qpquals = extract_nonindex_conditions(baserel->baserestrictinfo,
+ qpquals = extract_nonindex_conditions(path->indexrinfos,
path->indexquals);
}
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 9da5444..222ff00 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.
*/
/*
@@ -1814,8 +1814,8 @@ 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)
+ /* Add all the attributes used by index restriction clauses. */
+ foreach(lc, index->indrinfos)
{
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
@@ -2020,7 +2020,7 @@ static void
match_restriction_clauses_to_index(RelOptInfo *rel, IndexOptInfo *index,
IndexClauseSet *clauseset)
{
- match_clauses_to_index(index, rel->baserestrictinfo, clauseset);
+ match_clauses_to_index(index, index->indrinfos, clauseset);
}
/*
@@ -2686,6 +2686,12 @@ check_partial_indexes(PlannerInfo *root, RelOptInfo *rel)
{
IndexOptInfo *index = (IndexOptInfo *) lfirst(lc);
+ /*
+ * index rinfos are the same to baseristrict infos for non-partial
+ * indexes
+ */
+ index->indrinfos = rel->baserestrictinfo;
+
if (index->indpred == NIL)
continue; /* ignore non-partial indexes */
@@ -2744,6 +2750,7 @@ check_partial_indexes(PlannerInfo *root, RelOptInfo *rel)
foreach(lc, rel->indexlist)
{
IndexOptInfo *index = (IndexOptInfo *) lfirst(lc);
+ ListCell *lcr;
if (index->indpred == NIL)
continue; /* ignore non-partial indexes */
@@ -2752,6 +2759,45 @@ check_partial_indexes(PlannerInfo *root, RelOptInfo *rel)
continue; /* don't repeat work if already proven OK */
index->predOK = predicate_implied_by(index->indpred, clauselist);
+
+ /*
+ * If needed, construct restrictinfo for this index excluding them
+ * that are not implied by index predicates.
+ */
+
+ /* Search for the first rinfo that is implied by indpred */
+ foreach (lcr, rel->baserestrictinfo)
+ {
+ RestrictInfo *rinfo = (RestrictInfo *) lfirst(lcr);
+
+ if (predicate_implied_by(list_make1(rinfo->clause),
+ index->indpred))
+ break;
+ }
+
+ /* This index needs rinfos different from baserestrictinfo */
+ if (lcr)
+ {
+ ListCell *lcr2;
+ bool needs_check = false;
+
+ index->indrinfos = NIL;
+
+ foreach (lcr2, rel->baserestrictinfo)
+ {
+ RestrictInfo *rinfo2 = (RestrictInfo *) lfirst(lcr2);
+ if (lcr2 == lcr)
+ {
+ needs_check = true;
+ continue;
+ }
+ if (!needs_check ||
+ !predicate_implied_by(list_make1(rinfo2->clause),
+ index->indpred))
+ index->indrinfos = lappend(index->indrinfos, rinfo2);
+
+ }
+ }
}
}
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 1895a68..5910560 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -788,6 +788,7 @@ create_index_path(PlannerInfo *root,
pathnode->indexclauses = indexclauses;
pathnode->indexquals = indexquals;
pathnode->indexqualcols = indexqualcols;
+ pathnode->indexrinfos = index->indrinfos;
pathnode->indexorderbys = indexorderbys;
pathnode->indexorderbycols = indexorderbycols;
pathnode->indexscandir = indexscandir;
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index 6cf2e24..f21a5b6 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -553,6 +553,8 @@ typedef struct IndexOptInfo
List *indexprs; /* expressions for non-simple index columns */
List *indpred; /* predicate if a partial index, else NIL */
+ List *indrinfos; /* baseristrict info which are not implied by
+ * indpred */
List *indextlist; /* targetlist representing index columns */
@@ -792,6 +794,10 @@ typedef struct Path
* index column, so 'indexqualcols' must form a nondecreasing sequence.
* (The order of multiple quals for the same index column is unspecified.)
*
+ * 'indexrinfos' is a list of RestrictInfo nodes from the query's WHERE
+ * or JOIN conditions, excluding those implied by the index predicate
+ * (if the index is not partial, the list includes all restriction clauses).
+ *
* 'indexorderbys', if not NIL, is a list of ORDER BY expressions that have
* been found to be usable as ordering operators for an amcanorderbyop index.
* The list must match the path's pathkeys, ie, one expression per pathkey
@@ -826,6 +832,7 @@ typedef struct IndexPath
List *indexclauses;
List *indexquals;
List *indexqualcols;
+ List *indexrinfos;
List *indexorderbys;
List *indexorderbycols;
ScanDirection indexscandir;
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index de826b5..88f6a02 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -780,7 +780,6 @@ explain (costs off)
-> 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)
InitPlan 2 (returns $1)
-> Limit
-> Merge Append
@@ -792,8 +791,7 @@ explain (costs off)
-> Index Only Scan using minmaxtest2i on minmaxtest2 minmaxtest2_1
Index Cond: (f1 IS NOT NULL)
-> Index Only Scan Backward using minmaxtest3i on minmaxtest3 minmaxtest3_1
- Index Cond: (f1 IS NOT NULL)
-(25 rows)
+(23 rows)
select min(f1), max(f1) from minmaxtest;
min | max
@@ -819,7 +817,6 @@ explain (costs off)
-> 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)
InitPlan 2 (returns $1)
-> Limit
-> Merge Append
@@ -831,9 +828,8 @@ explain (costs off)
-> Index Only Scan using minmaxtest2i on minmaxtest2 minmaxtest2_1
Index Cond: (f1 IS NOT NULL)
-> Index Only Scan Backward using minmaxtest3i on minmaxtest3 minmaxtest3_1
- Index Cond: (f1 IS NOT NULL)
-> Result
-(27 rows)
+(25 rows)
select distinct min(f1), max(f1) from minmaxtest;
min | max
--
1.8.3.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
