Re: Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY

Uh, did this ever get addressed?

---------------------------------------------------------------------------

On Sun, Jul  6, 2014 at 08:56:00PM +0100, Andrew Gierth wrote:
> >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> 
>  >> I've experimented with the attached patch, which detects when this
>  >> situation might have occurred and does another pass to try and
>  >> build ordered scans without the SAOP condition. However, the
>  >> results may not be quite ideal, because at least in some test
>  >> queries (not all) the scan with the SAOP included in the
>  >> indexquals is being costed higher than the same scan with the SAOP
>  >> moved to a Filter, which seems unreasonable.
> 
>  Tom> I'm not convinced that that's a-priori unreasonable, since the
>  Tom> SAOP will result in multiple index scans under the hood.
>  Tom> Conceivably we ought to try the path with and with SAOPs all the
>  Tom> time.
> 
> OK, here's a patch that always retries on lower SAOP clauses, assuming
> that a SAOP in the first column is always applicable - or is even that
> assumption too strong?
> 
> -- 
> Andrew (irc:RhodiumToad)
> 

> diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
> index 42dcb11..cfcfbfc 100644
> --- a/src/backend/optimizer/path/indxpath.c
> +++ b/src/backend/optimizer/path/indxpath.c
> @@ -50,7 +50,8 @@ typedef enum
>  {
>      SAOP_PER_AM,                /* Use ScalarArrayOpExpr if amsearcharray */
>      SAOP_ALLOW,                    /* Use ScalarArrayOpExpr for all indexes */
> -    SAOP_REQUIRE                /* Require ScalarArrayOpExpr to be used */
> +    SAOP_REQUIRE,                /* Require ScalarArrayOpExpr to be used */
> +    SAOP_SKIP_LOWER                /* Require lower ScalarArrayOpExpr to be eliminated */
>  } SaOpControl;
>  
>  /* Whether we are looking for plain indexscan, bitmap scan, or either */
> @@ -118,7 +119,8 @@ static void get_index_paths(PlannerInfo *root, RelOptInfo *rel,
>  static List *build_index_paths(PlannerInfo *root, RelOptInfo *rel,
>                    IndexOptInfo *index, IndexClauseSet *clauses,
>                    bool useful_predicate,
> -                  SaOpControl saop_control, ScanTypeControl scantype);
> +                  SaOpControl saop_control, ScanTypeControl scantype,
> +                  bool *saop_retry);
>  static List *build_paths_for_OR(PlannerInfo *root, RelOptInfo *rel,
>                     List *clauses, List *other_clauses);
>  static List *generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
> @@ -734,6 +736,7 @@ get_index_paths(PlannerInfo *root, RelOptInfo *rel,
>  {
>      List       *indexpaths;
>      ListCell   *lc;
> +    bool       saop_retry = false;
>  
>      /*
>       * Build simple index paths using the clauses.  Allow ScalarArrayOpExpr
> @@ -742,7 +745,23 @@ get_index_paths(PlannerInfo *root, RelOptInfo *rel,
>      indexpaths = build_index_paths(root, rel,
>                                     index, clauses,
>                                     index->predOK,
> -                                   SAOP_PER_AM, ST_ANYSCAN);
> +                                   SAOP_PER_AM, ST_ANYSCAN, &saop_retry);
> +
> +    /*
> +     * If we allowed any ScalarArrayOpExprs on an index with a useful sort
> +     * ordering, then try again without them, since otherwise we miss important
> +     * paths where the index ordering is relevant.
> +     */
> +    if (saop_retry)
> +    {
> +        indexpaths = list_concat(indexpaths,
> +                                 build_index_paths(root, rel,
> +                                                   index, clauses,
> +                                                   index->predOK,
> +                                                   SAOP_SKIP_LOWER,
> +                                                   ST_ANYSCAN,
> +                                                   NULL));
> +    }
>  
>      /*
>       * Submit all the ones that can form plain IndexScan plans to add_path. (A
> @@ -779,7 +798,7 @@ get_index_paths(PlannerInfo *root, RelOptInfo *rel,
>          indexpaths = build_index_paths(root, rel,
>                                         index, clauses,
>                                         false,
> -                                       SAOP_REQUIRE, ST_BITMAPSCAN);
> +                                       SAOP_REQUIRE, ST_BITMAPSCAN, NULL);
>          *bitindexpaths = list_concat(*bitindexpaths, indexpaths);
>      }
>  }
> @@ -816,12 +835,14 @@ get_index_paths(PlannerInfo *root, RelOptInfo *rel,
>   * 'useful_predicate' indicates whether the index has a useful predicate
>   * 'saop_control' indicates whether ScalarArrayOpExpr clauses can be used
>   * 'scantype' indicates whether we need plain or bitmap scan support
> + * 'saop_retry' indicates whether a SAOP_SKIP_LOWER retry is worthwhile
>   */
>  static List *
>  build_index_paths(PlannerInfo *root, RelOptInfo *rel,
>                    IndexOptInfo *index, IndexClauseSet *clauses,
>                    bool useful_predicate,
> -                  SaOpControl saop_control, ScanTypeControl scantype)
> +                  SaOpControl saop_control, ScanTypeControl scantype,
> +                  bool *saop_retry)
>  {
>      List       *result = NIL;
>      IndexPath  *ipath;
> @@ -877,7 +898,9 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
>       * assuming that the scan result is ordered.  (Actually, the result is
>       * still ordered if there are equality constraints for all earlier
>       * columns, but it seems too expensive and non-modular for this code to be
> -     * aware of that refinement.)
> +     * aware of that refinement.) But if saop_control is SAOP_SKIP_LOWER, we
> +     * skip exactly these clauses that break sorting, and don't bother
> +     * building any paths otherwise.
>       *
>       * We also build a Relids set showing which outer rels are required by the
>       * selected clauses.  Any lateral_relids are included in that, but not
> @@ -901,9 +924,13 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
>                  /* Ignore if not supported by index */
>                  if (saop_control == SAOP_PER_AM && !index->amsearcharray)
>                      continue;
> -                found_clause = true;
>                  if (indexcol > 0)
> +                {
>                      found_lower_saop_clause = true;
> +                    if (saop_control == SAOP_SKIP_LOWER)
> +                        continue;
> +                }
> +                found_clause = true;
>              }
>              else
>              {
> @@ -928,6 +955,29 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
>              return NIL;
>      }
>  
> +    if (saop_control == SAOP_SKIP_LOWER)
> +    {
> +        if (!found_lower_saop_clause)
> +            return NIL;
> +        found_lower_saop_clause = false;
> +    }
> +    else if (found_lower_saop_clause)
> +    {
> +        /*
> +         * If we have a lower SAOP clause, we should leave it up to the cost
> +         * estimates to decide whether to use it in the scan or punt it to a
> +         * filter clause, rather than try and second-guess the AM's cost
> +         * estimate mechanism.  In addition, we need to consider the path
> +         * without the SAOP on the basis that it might give us an ordering
> +         * which overcomes any cost advantage of using the SAOP as an index
> +         * qual.  So either way, we flag it up so that the caller can do
> +         * another pass over the same index with SAOP_SKIP_LOWER to catch
> +         * such cases.
> +         */
> +        if (saop_retry)
> +            *saop_retry = true;
> +    }
> +
>      /* We do not want the index's rel itself listed in outer_relids */
>      outer_relids = bms_del_member(outer_relids, rel->relid);
>      /* Enforce convention that outer_relids is exactly NULL if empty */
> @@ -1137,7 +1187,7 @@ build_paths_for_OR(PlannerInfo *root, RelOptInfo *rel,
>          indexpaths = build_index_paths(root, rel,
>                                         index, &clauseset,
>                                         useful_predicate,
> -                                       SAOP_ALLOW, ST_BITMAPSCAN);
> +                                       SAOP_ALLOW, ST_BITMAPSCAN, NULL);
>          result = list_concat(result, indexpaths);
>      }
>  

> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +