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
