Uh, did this ever get addressed?

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

On Sun, Jul  6, 2014 at 08:56:00PM +0100, Andrew Gierth wrote:
> >>>>> "Tom" == Tom Lane <t...@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  <br...@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +


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

Reply via email to