From: Tom Lane Date: Fri, 15 Feb 2019 00:37:30 +0000 (-0500) Subject: Simplify the planner's new representation of indexable clauses a little. X-Git-Tag: REL_12_BETA1~740 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=8fd3fdd85a3e22f04b2cb0949450f63cb48952cd;p=postgresql Simplify the planner's new representation of indexable clauses a little. In commit 1a8d5afb0, I thought it'd be a good idea to define IndexClause.indexquals as NIL in the most common case where the given clause (IndexClause.rinfo) is usable exactly as-is. It'd be more consistent to define the indexquals in that case as being a one-element list containing IndexClause.rinfo, but I thought saving the palloc overhead for making such a list would be worthwhile. In hindsight, that was a great example of "premature optimization is the root of all evil": it's complicated everyplace that needs to deal with the indexquals, requiring duplicative code to handle both the simple case and the not-simple case. I'd initially found that tolerable but it's getting less so as I mop up some areas that I'd not touched in 1a8d5afb0. In any case, two more pallocs during a planner run are surely at the noise level (a conclusion confirmed by a bit of microbenchmarking). So let's change this decision before it becomes set in stone, and insist that IndexClause.indexquals always be a valid list of the actual index quals for the clause. Discussion: https://postgr.es/m/24586.1550106354@sss.pgh.pa.us --- diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index e0933fc24d..3434219dbd 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -2435,7 +2435,7 @@ match_clause_to_indexcol(PlannerInfo *root, { iclause = makeNode(IndexClause); iclause->rinfo = rinfo; - iclause->indexquals = NIL; + iclause->indexquals = list_make1(rinfo); iclause->lossy = false; iclause->indexcol = indexcol; iclause->indexcols = NIL; @@ -2599,7 +2599,7 @@ match_opclause_to_indexcol(PlannerInfo *root, { iclause = makeNode(IndexClause); iclause->rinfo = rinfo; - iclause->indexquals = NIL; + iclause->indexquals = list_make1(rinfo); iclause->lossy = false; iclause->indexcol = indexcol; iclause->indexcols = NIL; @@ -2819,7 +2819,7 @@ match_saopclause_to_indexcol(RestrictInfo *rinfo, IndexClause *iclause = makeNode(IndexClause); iclause->rinfo = rinfo; - iclause->indexquals = NIL; + iclause->indexquals = list_make1(rinfo); iclause->lossy = false; iclause->indexcol = indexcol; iclause->indexcols = NIL; @@ -3078,7 +3078,7 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo, * usable as index quals. */ if (var_on_left && !iclause->lossy) - iclause->indexquals = NIL; + iclause->indexquals = list_make1(rinfo); else { /* diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index c7645acad2..236f506cfb 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -3075,11 +3075,8 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual, Assert(!rinfo->pseudoconstant); subquals = lappend(subquals, rinfo->clause); - if (iclause->indexquals) - subindexquals = list_concat(subindexquals, - get_actual_clauses(iclause->indexquals)); - else - subindexquals = lappend(subindexquals, rinfo->clause); + subindexquals = list_concat(subindexquals, + get_actual_clauses(iclause->indexquals)); if (rinfo->parent_ec) subindexECs = lappend(subindexECs, rinfo->parent_ec); } @@ -4491,33 +4488,18 @@ fix_indexqual_references(PlannerInfo *root, IndexPath *index_path, { IndexClause *iclause = lfirst_node(IndexClause, lc); int indexcol = iclause->indexcol; + ListCell *lc2; - if (iclause->indexquals == NIL) + foreach(lc2, iclause->indexquals) { - /* rinfo->clause is directly usable as an indexqual */ - Node *clause = (Node *) iclause->rinfo->clause; + RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc2); + Node *clause = (Node *) rinfo->clause; stripped_indexquals = lappend(stripped_indexquals, clause); clause = fix_indexqual_clause(root, index, indexcol, clause, iclause->indexcols); fixed_indexquals = lappend(fixed_indexquals, clause); } - else - { - /* Process the derived indexquals */ - ListCell *lc2; - - foreach(lc2, iclause->indexquals) - { - RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc2); - Node *clause = (Node *) rinfo->clause; - - stripped_indexquals = lappend(stripped_indexquals, clause); - clause = fix_indexqual_clause(root, index, indexcol, - clause, iclause->indexcols); - fixed_indexquals = lappend(fixed_indexquals, clause); - } - } } *stripped_indexquals_p = stripped_indexquals; diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index c25357a008..0ceb72eaea 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -197,8 +197,6 @@ static bool get_actual_variable_range(PlannerInfo *root, Oid sortop, Datum *min, Datum *max); static RelOptInfo *find_join_input_rel(PlannerInfo *root, Relids relids); -static IndexQualInfo *deconstruct_indexqual(RestrictInfo *rinfo, - IndexOptInfo *index, int indexcol); static List *add_predicate_to_quals(IndexOptInfo *index, List *indexQuals); @@ -5263,16 +5261,13 @@ get_index_quals(List *indexclauses) foreach(lc, indexclauses) { IndexClause *iclause = lfirst_node(IndexClause, lc); + ListCell *lc2; - if (iclause->indexquals == NIL) + foreach(lc2, iclause->indexquals) { - /* rinfo->clause is directly usable as an indexqual */ - result = lappend(result, iclause->rinfo); - } - else - { - /* report the derived indexquals */ - result = list_concat(result, list_copy(iclause->indexquals)); + RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc2); + + result = lappend(result, rinfo); } } return result; @@ -5282,83 +5277,58 @@ List * deconstruct_indexquals(IndexPath *path) { List *result = NIL; - IndexOptInfo *index = path->indexinfo; ListCell *lc; foreach(lc, path->indexclauses) { IndexClause *iclause = lfirst_node(IndexClause, lc); int indexcol = iclause->indexcol; - IndexQualInfo *qinfo; + ListCell *lc2; - if (iclause->indexquals == NIL) - { - /* rinfo->clause is directly usable as an indexqual */ - qinfo = deconstruct_indexqual(iclause->rinfo, index, indexcol); - result = lappend(result, qinfo); - } - else + foreach(lc2, iclause->indexquals) { - /* Process the derived indexquals */ - ListCell *lc2; + RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc2); + Expr *clause = rinfo->clause; + IndexQualInfo *qinfo; - foreach(lc2, iclause->indexquals) - { - RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc2); + qinfo = (IndexQualInfo *) palloc(sizeof(IndexQualInfo)); + qinfo->rinfo = rinfo; + qinfo->indexcol = indexcol; - qinfo = deconstruct_indexqual(rinfo, index, indexcol); - result = lappend(result, qinfo); + if (IsA(clause, OpExpr)) + { + qinfo->clause_op = ((OpExpr *) clause)->opno; + qinfo->other_operand = get_rightop(clause); } - } - } - return result; -} - -static IndexQualInfo * -deconstruct_indexqual(RestrictInfo *rinfo, IndexOptInfo *index, int indexcol) -{ - { - Expr *clause; - IndexQualInfo *qinfo; - - clause = rinfo->clause; - - qinfo = (IndexQualInfo *) palloc(sizeof(IndexQualInfo)); - qinfo->rinfo = rinfo; - qinfo->indexcol = indexcol; + else if (IsA(clause, RowCompareExpr)) + { + RowCompareExpr *rc = (RowCompareExpr *) clause; - if (IsA(clause, OpExpr)) - { - qinfo->clause_op = ((OpExpr *) clause)->opno; - qinfo->other_operand = get_rightop(clause); - } - else if (IsA(clause, RowCompareExpr)) - { - RowCompareExpr *rc = (RowCompareExpr *) clause; + qinfo->clause_op = linitial_oid(rc->opnos); + qinfo->other_operand = (Node *) rc->rargs; + } + else if (IsA(clause, ScalarArrayOpExpr)) + { + ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) clause; - qinfo->clause_op = linitial_oid(rc->opnos); - qinfo->other_operand = (Node *) rc->rargs; - } - else if (IsA(clause, ScalarArrayOpExpr)) - { - ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) clause; + qinfo->clause_op = saop->opno; + qinfo->other_operand = (Node *) lsecond(saop->args); + } + else if (IsA(clause, NullTest)) + { + qinfo->clause_op = InvalidOid; + qinfo->other_operand = NULL; + } + else + { + elog(ERROR, "unsupported indexqual type: %d", + (int) nodeTag(clause)); + } - qinfo->clause_op = saop->opno; - qinfo->other_operand = (Node *) lsecond(saop->args); - } - else if (IsA(clause, NullTest)) - { - qinfo->clause_op = InvalidOid; - qinfo->other_operand = NULL; - } - else - { - elog(ERROR, "unsupported indexqual type: %d", - (int) nodeTag(clause)); + result = lappend(result, qinfo); } - - return qinfo; } + return result; } /* diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 616ec3b3db..a008ae07da 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -1185,26 +1185,20 @@ typedef struct IndexPath * conditions is done by a planner support function attached to the * indexclause's top-level function or operator. * - * If indexquals is NIL, it means that rinfo->clause is directly usable as - * an indexqual. Otherwise indexquals contains one or more directly-usable - * indexqual conditions extracted from the given clause. The 'lossy' flag - * indicates whether the indexquals are semantically equivalent to the - * original clause, or form a weaker condition. - * - * Currently, entries in indexquals are RestrictInfos, but they could perhaps - * be bare clauses instead; the only advantage of making them RestrictInfos - * is the possibility of caching cost and selectivity information across - * multiple uses, and it's not clear that that's really worth the price of - * constructing RestrictInfos for them. Note however that the extended-stats - * machinery won't do anything with non-RestrictInfo clauses, so that would - * have to be fixed. + * indexquals is a list of RestrictInfos for the directly-usable index + * conditions associated with this IndexClause. In the simplest case + * it's a one-element list whose member is iclause->rinfo. Otherwise, + * it contains one or more directly-usable indexqual conditions extracted + * from the given clause. The 'lossy' flag indicates whether the + * indexquals are semantically equivalent to the original clause, or + * represent a weaker condition. * * Normally, indexcol is the index of the single index column the clause * works on, and indexcols is NIL. But if the clause is a RowCompareExpr, * indexcol is the index of the leading column, and indexcols is a list of * all the affected columns. (Note that indexcols matches up with the - * columns of the actual indexable RowCompareExpr, which might be in - * indexquals rather than rinfo.) + * columns of the actual indexable RowCompareExpr in indexquals, which + * might be different from the original in rinfo.) * * An IndexPath's IndexClause list is required to be ordered by index * column, i.e. the indexcol values must form a nondecreasing sequence. @@ -1214,7 +1208,7 @@ typedef struct IndexClause { NodeTag type; struct RestrictInfo *rinfo; /* original restriction or join clause */ - List *indexquals; /* indexqual(s) derived from it, or NIL */ + List *indexquals; /* indexqual(s) derived from it */ bool lossy; /* are indexquals a lossy version of clause? */ AttrNumber indexcol; /* index column the clause uses (zero-based) */ List *indexcols; /* multiple index columns, if RowCompare */