Re: Proposed refactoring of planner header files - Mailing list pgsql-hackers

[ moving this discussion to a more appropriate thread; let's keep the
  original thread for discussing whether bloom actually needs fixed ]

Over in
https://www.postgresql.org/message-id/CAMkU=1yHfC+Gu84UFsz4hWn=C7tgQFMLiEQcto1Y-8WDE96vaw@mail.gmail.com

Jeff Janes <jeff.janes@gmail.com> writes:
> On Tue, Feb 12, 2019 at 4:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm just in the midst of refactoring that stuff, so if you have
>> suggestions, let's hear 'em.

> The goal would be that I can copy the entire definition of
> genericcostestimate into blcost.c, change the function's name, and get it
> to compile.  I don't know the correct way accomplish that.

The private functions that genericcostestimate needs are
get_index_quals, add_predicate_to_quals, other_operands_eval_cost,
and orderby_operands_eval_cost.  I don't mind exporting those,
although they should get names more appropriate to being global,
and I think the latter two should be merged, as attached.

Another thing that I've been thinking about here is that
deconstruct_indexquals() and the IndexQualInfo struct probably ought
to go away.  That was useful code given our previous convoluted data
structure for index quals, but now its purposes of hiding clause
commutation and associating the right index column with each clause
are vestigial.  The other goal that I originally had in inventing
that code was to allow cost estimation methods to disregard the
details of which clause type each indexqual actually is.  But looking
at how it's being used, it's generally not the case that callers get
to ignore that, which is unsurprising when you think about it: there's
enough behavioral difference between say OpExpr and ScalarArrayOpExpr
that it's kinda silly to think we could really paper it over.

So the attached patch includes a proof-of-concept for getting rid of
IndexQualInfo.  I didn't carry it to completion: gincostestimate is
still using that struct.  The thing that was blocking me was that
given the definition that IndexClause.indexquals is NIL if we're
supposed to use the original clause as-is, it's really hard to write
code that processes all the indexquals without duplicating logic.
That is, you tend to end up with something along the lines of
this, inside a loop over the IndexClauses:

    if (iclause->indexquals == NIL)
        do_something_with(iclause->rinfo);
    else
        foreach(lc, iclause->indexquals)
            do_something_with(lfirst(lc));

If you look at deconstruct_indexquals you'll see that I got around
that by making a subroutine for do_something_with, but it's not
convenient to do that if the code needs access to local variables
in the surrounding function.  In btcostestimate I instead did

    if (iclause->indexquals)
        indexquals = iclause->indexquals;
    else
        indexquals = list_make1(iclause->rinfo);

    foreach(lc, indexquals)
        do_something_with(lfirst(lc));

but I don't like that much either.  It'd be better I think if
we changed the definition of IndexClause.indexquals to always
be nonempty, and just be a singleton list of the original
iclause->rinfo in the simple case.  I'd rejected that approach
to begin with on the grounds that (a) letting it be NIL saves
a list_make1() in the common case, and (b) it seemed a bit
dodgy to have the original clause doubly-linked in this structure.
But argument (a) falls apart completely if places like btcostestimate
are doing their own list_make1 because the data structure is too
hard to work with.  And I don't think (b) holds much water either,
since we doubly-link RestrictInfos all over the place.

So I'm thinking about going and changing the definition of that
data structure before it's set in stone.

Comments?  Does anyone know of third-party AMs that are using
IndexQualInfo and would be sad to have it go away?

            regards, tom lane

diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
index 2d8a7f1..f9fe57f 100644
*** a/contrib/bloom/blcost.c
--- b/contrib/bloom/blcost.c
*************** blcostestimate(PlannerInfo *root, IndexP
*** 27,45 ****
                 double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
-     List       *qinfos;
      GenericCosts costs;

-     /* Do preliminary analysis of indexquals */
-     qinfos = deconstruct_indexquals(path);
-
      MemSet(&costs, 0, sizeof(costs));

      /* We have to visit all index tuples anyway */
      costs.numIndexTuples = index->tuples;

      /* Use generic estimate */
!     genericcostestimate(root, path, loop_count, qinfos, &costs);

      *indexStartupCost = costs.indexStartupCost;
      *indexTotalCost = costs.indexTotalCost;
--- 27,41 ----
                 double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
      GenericCosts costs;

      MemSet(&costs, 0, sizeof(costs));

      /* We have to visit all index tuples anyway */
      costs.numIndexTuples = index->tuples;

      /* Use generic estimate */
!     genericcostestimate(root, path, loop_count, &costs);

      *indexStartupCost = costs.indexStartupCost;
      *indexTotalCost = costs.indexTotalCost;
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index b9f99fa..3e30b57 100644
*** a/src/backend/utils/adt/selfuncs.c
--- b/src/backend/utils/adt/selfuncs.c
*************** static Const *string_to_const(const char
*** 228,234 ****
  static Const *string_to_bytea_const(const char *str, size_t str_len);
  static IndexQualInfo *deconstruct_indexqual(RestrictInfo *rinfo,
                        IndexOptInfo *index, int indexcol);
- static List *add_predicate_to_quals(IndexOptInfo *index, List *indexQuals);


  /*
--- 228,233 ----
*************** string_to_bytea_const(const char *str, s
*** 6570,6578 ****
   *-------------------------------------------------------------------------
   */

! /* Extract the actual indexquals (as RestrictInfos) from an IndexClause list */
! static List *
! get_index_quals(List *indexclauses)
  {
      List       *result = NIL;
      ListCell   *lc;
--- 6569,6579 ----
   *-------------------------------------------------------------------------
   */

! /*
!  * Extract the actual indexquals (as RestrictInfos) from an IndexClause list
!  */
! List *
! get_quals_from_indexclauses(List *indexclauses)
  {
      List       *result = NIL;
      ListCell   *lc;
*************** deconstruct_indexqual(RestrictInfo *rinf
*** 6679,6732 ****
  }

  /*
!  * Simple function to compute the total eval cost of the "other operands"
!  * in an IndexQualInfo list.  Since we know these will be evaluated just
   * once per scan, there's no need to distinguish startup from per-row cost.
-  */
- static Cost
- other_operands_eval_cost(PlannerInfo *root, List *qinfos)
- {
-     Cost        qual_arg_cost = 0;
-     ListCell   *lc;
-
-     foreach(lc, qinfos)
-     {
-         IndexQualInfo *qinfo = (IndexQualInfo *) lfirst(lc);
-         QualCost    index_qual_cost;
-
-         cost_qual_eval_node(&index_qual_cost, qinfo->other_operand, root);
-         qual_arg_cost += index_qual_cost.startup + index_qual_cost.per_tuple;
-     }
-     return qual_arg_cost;
- }
-
- /*
-  * Get other-operand eval cost for an index orderby list.
   *
!  * Index orderby expressions aren't represented as RestrictInfos (since they
!  * aren't boolean, usually).  So we can't apply deconstruct_indexquals to
!  * them.  However, they are much simpler to deal with since they are always
!  * OpExprs and the index column is always on the left.
   */
! static Cost
! orderby_operands_eval_cost(PlannerInfo *root, IndexPath *path)
  {
      Cost        qual_arg_cost = 0;
      ListCell   *lc;

!     foreach(lc, path->indexorderbys)
      {
          Expr       *clause = (Expr *) lfirst(lc);
          Node       *other_operand;
          QualCost    index_qual_cost;

          if (IsA(clause, OpExpr))
          {
!             other_operand = get_rightop(clause);
          }
          else
          {
!             elog(ERROR, "unsupported indexorderby type: %d",
                   (int) nodeTag(clause));
              other_operand = NULL;    /* keep compiler quiet */
          }
--- 6680,6737 ----
  }

  /*
!  * Compute the total evaluation cost of the comparison operands in a list
!  * of index qual expressions.  Since we know these will be evaluated just
   * once per scan, there's no need to distinguish startup from per-row cost.
   *
!  * This can be used either on the result of get_quals_from_indexclauses(),
!  * or directly on an indexorderbys list.  In both cases, we expect that the
!  * index key expression is on the left side of binary clauses.
   */
! Cost
! index_other_operands_eval_cost(PlannerInfo *root, List *indexquals)
  {
      Cost        qual_arg_cost = 0;
      ListCell   *lc;

!     foreach(lc, indexquals)
      {
          Expr       *clause = (Expr *) lfirst(lc);
          Node       *other_operand;
          QualCost    index_qual_cost;

+         /*
+          * Index quals will have RestrictInfos, indexorderbys won't.  Look
+          * through RestrictInfo if present.
+          */
+         if (IsA(clause, RestrictInfo))
+             clause = ((RestrictInfo *) clause)->clause;
+
          if (IsA(clause, OpExpr))
          {
!             OpExpr       *op = (OpExpr *) clause;
!
!             other_operand = (Node *) lsecond(op->args);
!         }
!         else if (IsA(clause, RowCompareExpr))
!         {
!             RowCompareExpr *rc = (RowCompareExpr *) clause;
!
!             other_operand = (Node *) rc->rargs;
!         }
!         else if (IsA(clause, ScalarArrayOpExpr))
!         {
!             ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) clause;
!
!             other_operand = (Node *) lsecond(saop->args);
!         }
!         else if (IsA(clause, NullTest))
!         {
!             other_operand = NULL;
          }
          else
          {
!             elog(ERROR, "unsupported indexqual type: %d",
                   (int) nodeTag(clause));
              other_operand = NULL;    /* keep compiler quiet */
          }
*************** void
*** 6741,6751 ****
  genericcostestimate(PlannerInfo *root,
                      IndexPath *path,
                      double loop_count,
-                     List *qinfos,
                      GenericCosts *costs)
  {
      IndexOptInfo *index = path->indexinfo;
!     List       *indexQuals = get_index_quals(path->indexclauses);
      List       *indexOrderBys = path->indexorderbys;
      Cost        indexStartupCost;
      Cost        indexTotalCost;
--- 6746,6755 ----
  genericcostestimate(PlannerInfo *root,
                      IndexPath *path,
                      double loop_count,
                      GenericCosts *costs)
  {
      IndexOptInfo *index = path->indexinfo;
!     List       *indexQuals = get_quals_from_indexclauses(path->indexclauses);
      List       *indexOrderBys = path->indexorderbys;
      Cost        indexStartupCost;
      Cost        indexTotalCost;
*************** genericcostestimate(PlannerInfo *root,
*** 6767,6773 ****
       * given indexquals to produce a more accurate idea of the index
       * selectivity.
       */
!     selectivityQuals = add_predicate_to_quals(index, indexQuals);

      /*
       * Check for ScalarArrayOpExpr index quals, and estimate the number of
--- 6771,6777 ----
       * given indexquals to produce a more accurate idea of the index
       * selectivity.
       */
!     selectivityQuals = add_predicate_to_index_quals(index, indexQuals);

      /*
       * Check for ScalarArrayOpExpr index quals, and estimate the number of
*************** genericcostestimate(PlannerInfo *root,
*** 6910,6917 ****
       * Detecting that that might be needed seems more expensive than it's
       * worth, though, considering all the other inaccuracies here ...
       */
!     qual_arg_cost = other_operands_eval_cost(root, qinfos) +
!         orderby_operands_eval_cost(root, path);
      qual_op_cost = cpu_operator_cost *
          (list_length(indexQuals) + list_length(indexOrderBys));

--- 6914,6921 ----
       * Detecting that that might be needed seems more expensive than it's
       * worth, though, considering all the other inaccuracies here ...
       */
!     qual_arg_cost = index_other_operands_eval_cost(root, indexQuals) +
!         index_other_operands_eval_cost(root, indexOrderBys);
      qual_op_cost = cpu_operator_cost *
          (list_length(indexQuals) + list_length(indexOrderBys));

*************** genericcostestimate(PlannerInfo *root,
*** 6956,6963 ****
   * predicate_implied_by() and clauselist_selectivity(), but might be
   * problematic if the result were passed to other things.
   */
! static List *
! add_predicate_to_quals(IndexOptInfo *index, List *indexQuals)
  {
      List       *predExtraQuals = NIL;
      ListCell   *lc;
--- 6960,6967 ----
   * predicate_implied_by() and clauselist_selectivity(), but might be
   * problematic if the result were passed to other things.
   */
! List *
! add_predicate_to_index_quals(IndexOptInfo *index, List *indexQuals)
  {
      List       *predExtraQuals = NIL;
      ListCell   *lc;
*************** btcostestimate(PlannerInfo *root, IndexP
*** 6985,6991 ****
                 double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
-     List       *qinfos;
      GenericCosts costs;
      Oid            relid;
      AttrNumber    colnum;
--- 6989,6994 ----
*************** btcostestimate(PlannerInfo *root, IndexP
*** 7000,7008 ****
      double        num_sa_scans;
      ListCell   *lc;

-     /* Do preliminary analysis of indexquals */
-     qinfos = deconstruct_indexquals(path);
-
      /*
       * For a btree scan, only leading '=' quals plus inequality quals for the
       * immediately next attribute contribute to index selectivity (these are
--- 7003,7008 ----
*************** btcostestimate(PlannerInfo *root, IndexP
*** 7026,7083 ****
      found_saop = false;
      found_is_null_op = false;
      num_sa_scans = 1;
!     foreach(lc, qinfos)
      {
!         IndexQualInfo *qinfo = (IndexQualInfo *) lfirst(lc);
!         RestrictInfo *rinfo = qinfo->rinfo;
!         Expr       *clause = rinfo->clause;
!         Oid            clause_op;
!         int            op_strategy;

!         if (indexcol != qinfo->indexcol)
          {
              /* Beginning of a new column's quals */
              if (!eqQualHere)
                  break;            /* done if no '=' qual for indexcol */
              eqQualHere = false;
              indexcol++;
!             if (indexcol != qinfo->indexcol)
                  break;            /* no quals at all for indexcol */
          }

!         if (IsA(clause, ScalarArrayOpExpr))
!         {
!             int            alength = estimate_array_length(qinfo->other_operand);

!             found_saop = true;
!             /* count up number of SA scans induced by indexBoundQuals only */
!             if (alength > 1)
!                 num_sa_scans *= alength;
!         }
!         else if (IsA(clause, NullTest))
          {
!             NullTest   *nt = (NullTest *) clause;

!             if (nt->nulltesttype == IS_NULL)
              {
!                 found_is_null_op = true;
!                 /* IS NULL is like = for selectivity determination purposes */
!                 eqQualHere = true;
              }
!         }

!         /* check for equality operator */
!         clause_op = qinfo->clause_op;
!         if (OidIsValid(clause_op))
!         {
!             op_strategy = get_op_opfamily_strategy(clause_op,
!                                                    index->opfamily[indexcol]);
!             Assert(op_strategy != 0);    /* not a member of opfamily?? */
!             if (op_strategy == BTEqualStrategyNumber)
!                 eqQualHere = true;
!         }

!         indexBoundQuals = lappend(indexBoundQuals, rinfo);
      }

      /*
--- 7026,7109 ----
      found_saop = false;
      found_is_null_op = false;
      num_sa_scans = 1;
!     foreach(lc, path->indexclauses)
      {
!         IndexClause *iclause = lfirst_node(IndexClause, lc);
!         List       *indexquals;
!         ListCell   *lc2;

!         if (indexcol != iclause->indexcol)
          {
              /* Beginning of a new column's quals */
              if (!eqQualHere)
                  break;            /* done if no '=' qual for indexcol */
              eqQualHere = false;
              indexcol++;
!             if (indexcol != iclause->indexcol)
                  break;            /* no quals at all for indexcol */
          }

!         /* Examine each indexqual associated with this index clause */
!         if (iclause->indexquals)
!             indexquals = iclause->indexquals;
!         else
!             indexquals = list_make1(iclause->rinfo);

!         foreach(lc2, indexquals)
          {
!             RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc2);
!             Expr       *clause = rinfo->clause;
!             Oid            clause_op = InvalidOid;
!             int            op_strategy;

!             if (IsA(clause, OpExpr))
              {
!                 OpExpr       *op = (OpExpr *) clause;
!
!                 clause_op = op->opno;
              }
!             else if (IsA(clause, RowCompareExpr))
!             {
!                 RowCompareExpr *rc = (RowCompareExpr *) clause;

!                 clause_op = linitial_oid(rc->opnos);
!             }
!             else if (IsA(clause, ScalarArrayOpExpr))
!             {
!                 ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) clause;
!                 Node       *other_operand = (Node *) lsecond(saop->args);
!                 int            alength = estimate_array_length(other_operand);

!                 clause_op = saop->opno;
!                 found_saop = true;
!                 /* count number of SA scans induced by indexBoundQuals only */
!                 if (alength > 1)
!                     num_sa_scans *= alength;
!             }
!             else if (IsA(clause, NullTest))
!             {
!                 NullTest   *nt = (NullTest *) clause;
!
!                 if (nt->nulltesttype == IS_NULL)
!                 {
!                     found_is_null_op = true;
!                     /* IS NULL is like = for selectivity purposes */
!                     eqQualHere = true;
!                 }
!             }
!
!             /* check for equality operator */
!             if (OidIsValid(clause_op))
!             {
!                 op_strategy = get_op_opfamily_strategy(clause_op,
!                                                        index->opfamily[indexcol]);
!                 Assert(op_strategy != 0);    /* not a member of opfamily?? */
!                 if (op_strategy == BTEqualStrategyNumber)
!                     eqQualHere = true;
!             }
!
!             indexBoundQuals = lappend(indexBoundQuals, rinfo);
!         }
      }

      /*
*************** btcostestimate(PlannerInfo *root, IndexP
*** 7102,7108 ****
           * index-bound quals to produce a more accurate idea of the number of
           * rows covered by the bound conditions.
           */
!         selectivityQuals = add_predicate_to_quals(index, indexBoundQuals);

          btreeSelectivity = clauselist_selectivity(root, selectivityQuals,
                                                    index->rel->relid,
--- 7128,7134 ----
           * index-bound quals to produce a more accurate idea of the number of
           * rows covered by the bound conditions.
           */
!         selectivityQuals = add_predicate_to_index_quals(index, indexBoundQuals);

          btreeSelectivity = clauselist_selectivity(root, selectivityQuals,
                                                    index->rel->relid,
*************** btcostestimate(PlannerInfo *root, IndexP
*** 7124,7130 ****
      MemSet(&costs, 0, sizeof(costs));
      costs.numIndexTuples = numIndexTuples;

!     genericcostestimate(root, path, loop_count, qinfos, &costs);

      /*
       * Add a CPU-cost component to represent the costs of initial btree
--- 7150,7156 ----
      MemSet(&costs, 0, sizeof(costs));
      costs.numIndexTuples = numIndexTuples;

!     genericcostestimate(root, path, loop_count, &costs);

      /*
       * Add a CPU-cost component to represent the costs of initial btree
*************** hashcostestimate(PlannerInfo *root, Inde
*** 7271,7285 ****
                   Selectivity *indexSelectivity, double *indexCorrelation,
                   double *indexPages)
  {
-     List       *qinfos;
      GenericCosts costs;

-     /* Do preliminary analysis of indexquals */
-     qinfos = deconstruct_indexquals(path);
-
      MemSet(&costs, 0, sizeof(costs));

!     genericcostestimate(root, path, loop_count, qinfos, &costs);

      /*
       * A hash index has no descent costs as such, since the index AM can go
--- 7297,7307 ----
                   Selectivity *indexSelectivity, double *indexCorrelation,
                   double *indexPages)
  {
      GenericCosts costs;

      MemSet(&costs, 0, sizeof(costs));

!     genericcostestimate(root, path, loop_count, &costs);

      /*
       * A hash index has no descent costs as such, since the index AM can go
*************** gistcostestimate(PlannerInfo *root, Inde
*** 7320,7335 ****
                   double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
-     List       *qinfos;
      GenericCosts costs;
      Cost        descentCost;

-     /* Do preliminary analysis of indexquals */
-     qinfos = deconstruct_indexquals(path);
-
      MemSet(&costs, 0, sizeof(costs));

!     genericcostestimate(root, path, loop_count, qinfos, &costs);

      /*
       * We model index descent costs similarly to those for btree, but to do
--- 7342,7353 ----
                   double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
      GenericCosts costs;
      Cost        descentCost;

      MemSet(&costs, 0, sizeof(costs));

!     genericcostestimate(root, path, loop_count, &costs);

      /*
       * We model index descent costs similarly to those for btree, but to do
*************** spgcostestimate(PlannerInfo *root, Index
*** 7381,7396 ****
                  double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
-     List       *qinfos;
      GenericCosts costs;
      Cost        descentCost;

-     /* Do preliminary analysis of indexquals */
-     qinfos = deconstruct_indexquals(path);
-
      MemSet(&costs, 0, sizeof(costs));

!     genericcostestimate(root, path, loop_count, qinfos, &costs);

      /*
       * We model index descent costs similarly to those for btree, but to do
--- 7399,7410 ----
                  double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
      GenericCosts costs;
      Cost        descentCost;

      MemSet(&costs, 0, sizeof(costs));

!     genericcostestimate(root, path, loop_count, &costs);

      /*
       * We model index descent costs similarly to those for btree, but to do
*************** gincostestimate(PlannerInfo *root, Index
*** 7730,7737 ****
                  double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
!     List       *indexQuals = get_index_quals(path->indexclauses);
!     List       *indexOrderBys = path->indexorderbys;
      List       *qinfos;
      ListCell   *l;
      List       *selectivityQuals;
--- 7744,7750 ----
                  double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
!     List       *indexQuals = get_quals_from_indexclauses(path->indexclauses);
      List       *qinfos;
      ListCell   *l;
      List       *selectivityQuals;
*************** gincostestimate(PlannerInfo *root, Index
*** 7837,7843 ****
       * quals to produce a more accurate idea of the number of rows covered by
       * the bound conditions.
       */
!     selectivityQuals = add_predicate_to_quals(index, indexQuals);

      /* Estimate the fraction of main-table tuples that will be visited */
      *indexSelectivity = clauselist_selectivity(root, selectivityQuals,
--- 7850,7856 ----
       * quals to produce a more accurate idea of the number of rows covered by
       * the bound conditions.
       */
!     selectivityQuals = add_predicate_to_index_quals(index, indexQuals);

      /* Estimate the fraction of main-table tuples that will be visited */
      *indexSelectivity = clauselist_selectivity(root, selectivityQuals,
*************** gincostestimate(PlannerInfo *root, Index
*** 8017,8028 ****
          dataPagesFetched * spc_random_page_cost;

      /*
!      * Add on index qual eval costs, much as in genericcostestimate
       */
!     qual_arg_cost = other_operands_eval_cost(root, qinfos) +
!         orderby_operands_eval_cost(root, path);
!     qual_op_cost = cpu_operator_cost *
!         (list_length(indexQuals) + list_length(indexOrderBys));

      *indexStartupCost += qual_arg_cost;
      *indexTotalCost += qual_arg_cost;
--- 8030,8040 ----
          dataPagesFetched * spc_random_page_cost;

      /*
!      * Add on index qual eval costs, much as in genericcostestimate.  But we
!      * can disregard indexorderbys, since GIN doesn't support those.
       */
!     qual_arg_cost = index_other_operands_eval_cost(root, indexQuals);
!     qual_op_cost = cpu_operator_cost * list_length(indexQuals);

      *indexStartupCost += qual_arg_cost;
      *indexTotalCost += qual_arg_cost;
*************** brincostestimate(PlannerInfo *root, Inde
*** 8040,8050 ****
                   double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
!     List       *indexQuals = get_index_quals(path->indexclauses);
      double        numPages = index->pages;
      RelOptInfo *baserel = index->rel;
      RangeTblEntry *rte = planner_rt_fetch(baserel->relid, root);
-     List       *qinfos;
      Cost        spc_seq_page_cost;
      Cost        spc_random_page_cost;
      double        qual_arg_cost;
--- 8052,8061 ----
                   double *indexPages)
  {
      IndexOptInfo *index = path->indexinfo;
!     List       *indexQuals = get_quals_from_indexclauses(path->indexclauses);
      double        numPages = index->pages;
      RelOptInfo *baserel = index->rel;
      RangeTblEntry *rte = planner_rt_fetch(baserel->relid, root);
      Cost        spc_seq_page_cost;
      Cost        spc_random_page_cost;
      double        qual_arg_cost;
*************** brincostestimate(PlannerInfo *root, Inde
*** 8082,8092 ****
       */
      *indexCorrelation = 0;

!     qinfos = deconstruct_indexquals(path);
!     foreach(l, qinfos)
      {
!         IndexQualInfo *qinfo = (IndexQualInfo *) lfirst(l);
!         AttrNumber    attnum = index->indexkeys[qinfo->indexcol];

          /* attempt to lookup stats in relation for this index column */
          if (attnum != 0)
--- 8093,8102 ----
       */
      *indexCorrelation = 0;

!     foreach(l, path->indexclauses)
      {
!         IndexClause *iclause = lfirst_node(IndexClause, l);
!         AttrNumber    attnum = index->indexkeys[iclause->indexcol];

          /* attempt to lookup stats in relation for this index column */
          if (attnum != 0)
*************** brincostestimate(PlannerInfo *root, Inde
*** 8121,8127 ****
               */

              /* get the attnum from the 0-based index. */
!             attnum = qinfo->indexcol + 1;

              if (get_index_stats_hook &&
                  (*get_index_stats_hook) (root, index->indexoid, attnum, &vardata))
--- 8131,8137 ----
               */

              /* get the attnum from the 0-based index. */
!             attnum = iclause->indexcol + 1;

              if (get_index_stats_hook &&
                  (*get_index_stats_hook) (root, index->indexoid, attnum, &vardata))
*************** brincostestimate(PlannerInfo *root, Inde
*** 8200,8209 ****

      /*
       * Compute the index qual costs, much as in genericcostestimate, to add to
!      * the index costs.
       */
!     qual_arg_cost = other_operands_eval_cost(root, qinfos) +
!         orderby_operands_eval_cost(root, path);

      /*
       * Compute the startup cost as the cost to read the whole revmap
--- 8210,8219 ----

      /*
       * Compute the index qual costs, much as in genericcostestimate, to add to
!      * the index costs.  We can disregard indexorderbys, since BRIN doesn't
!      * support those.
       */
!     qual_arg_cost = index_other_operands_eval_cost(root, indexQuals);

      /*
       * Compute the startup cost as the cost to read the whole revmap
diff --git a/src/include/utils/selfuncs.h b/src/include/utils/selfuncs.h
index 087b56f..a617884 100644
*** a/src/include/utils/selfuncs.h
--- b/src/include/utils/selfuncs.h
*************** extern void estimate_hash_bucket_stats(P
*** 213,222 ****
                             Selectivity *mcv_freq,
                             Selectivity *bucketsize_frac);

  extern List *deconstruct_indexquals(IndexPath *path);
  extern void genericcostestimate(PlannerInfo *root, IndexPath *path,
                      double loop_count,
-                     List *qinfos,
                      GenericCosts *costs);

  /* Functions in array_selfuncs.c */
--- 213,224 ----
                             Selectivity *mcv_freq,
                             Selectivity *bucketsize_frac);

+ extern List *get_quals_from_indexclauses(List *indexclauses);
  extern List *deconstruct_indexquals(IndexPath *path);
+ extern Cost index_other_operands_eval_cost(PlannerInfo *root, List *indexquals);
+ extern List *add_predicate_to_index_quals(IndexOptInfo *index, List *indexQuals);
  extern void genericcostestimate(PlannerInfo *root, IndexPath *path,
                      double loop_count,
                      GenericCosts *costs);

  /* Functions in array_selfuncs.c */