Thank you for the comment. The new version is attached.
Some issues has not been addressed but the rest will be addresses
in the next version.
At Thu, 31 Mar 2016 08:42:50 +0200, Tomas Vondra <tomas.von...@2ndquadrant.com>
wrote in <90d885f2-e5ce-6668-226f-c817154e4...@2ndquadrant.com>
> On 03/31/2016 01:36 AM, Tom Lane wrote:
> > Kevin Grittner <kgri...@gmail.com> writes:
> >> I'm taking my name off as committer and marking it "Ready for
> >> Committer". If someone else wants to comment on the issues where
> >> Tom and Kyotaro-san still seem unsatisfied to the point where I
> >> can get my head around it, I could maybe take it back on as
> >> committer -- if anyone feels that could be a net win.
> >
> > I'll pick it up. In a quick scan I see some things I don't like, but
> > nothing insoluble:
> >
> > * Having plancat.c initialize the per-IndexOptInfo restriction lists
> > * seems
> > fairly bizarre. I realize that Tomas or Kyotaro probably did that in
> > response to my complaint about having check_partial_indexes have
> > side-effects on non-partial indexes, but this isn't an improvement.
I felt the same thing for it. It is for discussion. It is the
earliest point where we can initialize baserestrictinfo of each
IndexOptInfo. And the original location is just after and is the
latest point. Reverted.
> > For one thing, it means we will produce an incorrect plan if
> > more restriction clauses are added to the rel after plancat.c
> > runs, as the comment for check_partial_indexes contemplates.
Mmm. Thank you. I overlooked that. The following code seems
saying related thing.
> if (index->predOK)
> continue; /* don't repeat work if already proven OK */
So, index restrinctioninfo should be calculated even if
index->predOK is already true. But refactroring suggested by the
following comment will fix this.
> > (I *think* that comment may be obsolete, but I'm not sure.)
> > I think a better answer to that complaint is to rename
> > check_partial_indexes to something else, and more importantly
> > adjust its header comment to reflect its expanded
> > responsibilities --- as the patch I was commenting on at the
> > time failed to do.
Ok, I removed the index baserestrictinfo (a bit long to repeat..)
creation out of check_partial_indexes and named
rebuild_index_restrictinfo. (calling for better names..)
This loosens the restriction on the timing to trim an index
restrictinfo. It is now moved to create_index_paths.
> > * It took me some time to convince myself that the patch doesn't break
> > generation of plans for EvalPlanQual. I eventually found where it
> > skips removal of restrictions for target relations, but that's
> > drastically undercommented.
It is originally in create_indexscan_plan and had no
documentation about EPQ. But I also want such documentation there
so I added it on in rebuild_index_restrictinfo.
> > * Likewise, there's inadequate documentation of why it doesn't break
> > bitmap scans, which also have to be able to recheck correctly.
This trims clauses that implied by index predicate, which
donesn't need recheck even if it is a bitmap scan, I believe. Is
it wrong? The original comment on check_index_only said that,
> * 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.
This seems to me that such clauses are safely ignored. But not
for attributes used only in index quals, because of possible
lossy charcter of the index. It seems quite reasonable. This
optimization won't be hold if this comment is wrong.
> > * I'm not sure that we have any regression test cases covering the
> > above two points, but we should.
I agree. Will try to add in the next version, maybe posted tomorrow.
> > * The comments leave a lot to be desired in general, eg there are
> > repeated failures to update comments only a few lines away from a
> > change.
I'll recheck that and fix in the next version.
> Kyotaro,
>
> I may look into fixing those issues early next week, but that's fairly
> close to the freeze. Also, I'll have to look into parts that I'm not
> very familiar with (like the EvalPlanQual), so feel free to address
> those issues ;-)
Aye sir!
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From aee7790b3f02826a94b4e7195ffb50afa398cf26 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Wed, 9 Mar 2016 16:37:00 +0900
Subject: [PATCH] v11
---
src/backend/nodes/outfuncs.c | 1 +
src/backend/optimizer/path/costsize.c | 8 ++--
src/backend/optimizer/path/indxpath.c | 80 +++++++++++++++++++++++++++-----
src/backend/optimizer/plan/createplan.c | 49 ++++++++++++-------
src/backend/optimizer/util/plancat.c | 6 +++
src/include/nodes/relation.h | 6 ++-
src/test/regress/expected/aggregates.out | 8 +---
7 files changed, 118 insertions(+), 40 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 5b71c95..5b511cd 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2128,6 +2128,7 @@ _outIndexOptInfo(StringInfo str, const IndexOptInfo *node)
WRITE_OID_FIELD(relam);
/* indexprs is redundant since we print indextlist */
WRITE_NODE_FIELD(indpred);
+ WRITE_NODE_FIELD(baserestrictinfo);
WRITE_NODE_FIELD(indextlist);
WRITE_BOOL_FIELD(predOK);
WRITE_BOOL_FIELD(unique);
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index b86fc5e..b14d0ee 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -438,18 +438,16 @@ cost_index(IndexPath *path, PlannerInfo *root, double loop_count)
if (path->path.param_info)
{
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,
- path->indexquals),
+ extract_nonindex_conditions(path->indexinfo->baserestrictinfo,
+ path->indexquals),
extract_nonindex_conditions(path->path.param_info->ppi_clauses,
path->indexquals));
}
else
{
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->indexinfo->baserestrictinfo,
path->indexquals);
}
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index b48f5f2..0603287 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -30,6 +30,7 @@
#include "optimizer/pathnode.h"
#include "optimizer/paths.h"
#include "optimizer/predtest.h"
+#include "optimizer/prep.h"
#include "optimizer/restrictinfo.h"
#include "optimizer/var.h"
#include "utils/builtins.h"
@@ -167,6 +168,8 @@ static void match_pathkeys_to_index(IndexOptInfo *index, List *pathkeys,
List **clause_columns_p);
static Expr *match_clause_to_ordering_op(IndexOptInfo *index,
int indexcol, Expr *clause, Oid pk_opfamily);
+static bool rebuild_index_restrictinfo(PlannerInfo *root, RelOptInfo *rel,
+ IndexOptInfo *index);
static bool ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel,
EquivalenceClass *ec, EquivalenceMember *em,
void *arg);
@@ -255,6 +258,12 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
Assert(index->ncolumns <= INDEX_MAX_KEYS);
/*
+ * baseristrictinfo could be trimmed by index predicates so the code
+ * hereafter looks the trimmed restrctinfo.
+ */
+ index->baserestrictinfo = rel->baserestrictinfo;
+
+ /*
* Ignore partial indexes that do not match the query.
* (generate_bitmap_or_paths() might be able to do something with
* them, but that's of no concern here.)
@@ -263,6 +272,12 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
continue;
/*
+ * We can seafely remove restrict clauses that implied by index
+ * predicates.
+ */
+ rebuild_index_restrictinfo(root, rel, index);
+
+ /*
* Identify the restriction clauses that can match the index.
*/
MemSet(&rclauseset, 0, sizeof(rclauseset));
@@ -1800,14 +1815,11 @@ 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.
+
+ * The attributes used only in index quals won't need to be checked at
+ * runtime 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.
*/
/*
@@ -1817,8 +1829,8 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
*/
pull_varattnos((Node *) rel->reltarget->exprs, 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->baserestrictinfo)
{
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
@@ -2023,7 +2035,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->baserestrictinfo, clauseset);
}
/*
@@ -2758,6 +2770,52 @@ check_partial_indexes(PlannerInfo *root, RelOptInfo *rel)
}
}
+/*
+ * rebuild_index_restrictinfo
+ * Build restriction info for usable partial indexes. This may be called
+ * after adding more restrictions to the rel. So we should re-calculate
+ * index restrictinfo for all available partial indexes.
+ */
+static bool
+rebuild_index_restrictinfo(PlannerInfo *root, RelOptInfo *rel,
+ IndexOptInfo *index)
+{
+ ListCell *lcr;
+ bool trimmed = false;
+
+ /*
+ * We first quickly check if we can safely remove clauses. EvalPlanQual
+ * requires recheck on heap tuples so no index clasues cannot be removd.
+ */
+ if (!index->predOK ||
+ rel->relid == root->parse->resultRelation ||
+ get_plan_rowmark(root->rowMarks, rel->relid) != NULL)
+ return false;
+
+ /*
+ * We could avoid creating a new baserestrictinfo for the index if it
+ * will match the one for the relation, but the few cycles saved from
+ * such an optimization are not worth the code complexity. Just create
+ * it unconditionally even though the lists may match.
+ */
+ index->baserestrictinfo = NIL;
+ foreach (lcr, rel->baserestrictinfo)
+ {
+ RestrictInfo *rinfo = (RestrictInfo *) lfirst(lcr);
+
+ /* add to index clauses if initial or not implied */
+ if (contain_mutable_functions((Node *) rinfo->clause) ||
+ !predicate_implied_by(list_make1(rinfo->clause),
+ index->indpred))
+ index->baserestrictinfo =
+ lappend(index->baserestrictinfo, rinfo);
+ else
+ trimmed = true;
+ }
+
+ return trimmed;
+}
+
/****************************************************************************
* ---- ROUTINES TO CHECK EXTERNALLY-VISIBLE CONDITIONS ----
****************************************************************************/
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 994983b..be882c7 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -71,6 +71,7 @@
static Plan *create_plan_recurse(PlannerInfo *root, Path *best_path,
int flags);
+static List *get_baserestrictinfo(PlannerInfo *root, Path *best_path);
static Plan *create_scan_plan(PlannerInfo *root, Path *best_path,
int flags);
static List *build_path_tlist(PlannerInfo *root, Path *path);
@@ -478,6 +479,27 @@ create_plan_recurse(PlannerInfo *root, Path *best_path, int flags)
}
/*
+ * get_baserestrictinfo
+ * Return the list of restriction clauses for the path.
+ *
+ * For index paths we return the list produced by by check_partial_indexes,
+ * i.e. without clauses implied by the index predicate (if present).
+ */
+static List *
+get_baserestrictinfo(PlannerInfo *root, Path *best_path)
+{
+ switch (best_path->pathtype)
+ {
+ case T_IndexScan:
+ case T_IndexOnlyScan:
+ Assert(IsA(best_path, IndexPath));
+ return ((IndexPath *) best_path)->indexinfo->baserestrictinfo;
+ default:
+ return best_path->parent->baserestrictinfo;
+ }
+}
+
+/*
* create_scan_plan
* Create a scan plan for the parent relation of 'best_path'.
*/
@@ -493,9 +515,11 @@ create_scan_plan(PlannerInfo *root, Path *best_path, int flags)
/*
* Extract the relevant restriction clauses from the parent relation. The
* executor must apply all these restrictions during the scan, except for
- * pseudoconstants which we'll take care of below.
+ * pseudoconstants which we'll take care of below. Some types of paths
+ * override the baserestrictinfo of the parent relation (e.g., a partial
+ * index excludes redundant index conditions).
*/
- scan_clauses = rel->baserestrictinfo;
+ scan_clauses = get_baserestrictinfo(root, best_path);
/*
* If this is a parameterized scan, we also need to enforce all the join
@@ -2405,21 +2429,14 @@ create_indexscan_plan(PlannerInfo *root,
continue; /* simple duplicate */
if (is_redundant_derived_clause(rinfo, indexquals))
continue; /* derived from same EquivalenceClass */
- if (!contain_mutable_functions((Node *) rinfo->clause))
- {
- List *clausel = list_make1(rinfo->clause);
+ /*
+ * scan_clauses don't contain clauses implied by index predicate. See
+ * check_partial_indexes() for detail.
+ */
+ if (!contain_mutable_functions((Node *) rinfo->clause) &&
+ predicate_implied_by(list_make1(rinfo->clause), indexquals))
+ continue; /* provably implied by indexquals */
- if (predicate_implied_by(clausel, indexquals))
- continue; /* provably implied by indexquals */
- if (best_path->indexinfo->indpred)
- {
- if (baserelid != root->parse->resultRelation &&
- get_plan_rowmark(root->rowMarks, baserelid) == NULL)
- if (predicate_implied_by(clausel,
- best_path->indexinfo->indpred))
- continue; /* implied by index predicate */
- }
- }
qpqual = lappend(qpqual, rinfo);
}
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 546067b..b662859 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -331,6 +331,12 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
*/
info->indexprs = RelationGetIndexExpressions(indexRelation);
info->indpred = RelationGetIndexPredicate(indexRelation);
+
+ /*
+ * Index baserestrictinfo cannot be calculated yet; leave it NULL
+ * for now.
+ */
+ info->baserestrictinfo = NULL;
if (info->indexprs && varno != 1)
ChangeVarNodes((Node *) info->indexprs, 1, varno, 0);
if (info->indpred && varno != 1)
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index 641446c..ef166ab 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -592,9 +592,11 @@ typedef struct IndexOptInfo
List *indexprs; /* expressions for non-simple index columns */
List *indpred; /* predicate if a partial index, else NIL */
-
+ List *baserestrictinfo; /* a list of RestrictInfo nodes from the
+ * query's WHERE or JOIN conditions, maybe
+ * excluding those implied by the index
+ * predicate */
List *indextlist; /* targetlist representing index columns */
-
bool predOK; /* true if predicate matches query */
bool unique; /* true if a unique index */
bool immediate; /* is uniqueness enforced immediately? */
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 601bdb4..3ff6691 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
@@ -818,7 +816,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
@@ -830,11 +827,10 @@ 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)
-> Sort
Sort Key: ($0), ($1)
-> Result
-(28 rows)
+(26 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
