Re: [HACKERS] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY
I wrote: > Andrew Gierth writes: >> "Bruce" == Bruce Momjian writes: >> Bruce> Uh, did this ever get addressed? >> It did not. > It dropped off the radar screen (I think I'd assumed the patch would > appear in the next commitfest, which it didn't unless I missed something). > I'll make a note to look at it once I've finished with the timezone > abbreviation mess. I've pushed this patch with some further redesign of build_index_paths' API --- if we're going to have it reporting about what it found, we should extend that to the other case of non-amsearcharray indexes too. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY
Andrew Gierth writes: > "Bruce" == Bruce Momjian writes: > Bruce> Uh, did this ever get addressed? > It did not. It dropped off the radar screen (I think I'd assumed the patch would appear in the next commitfest, which it didn't unless I missed something). I'll make a note to look at it once I've finished with the timezone abbreviation mess. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY
> "Bruce" == Bruce Momjian writes: Bruce> Uh, did this ever get addressed? It did not. -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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_
Re: [HACKERS] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY
> "Tom" == Tom Lane 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
Re: [HACKERS] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY
Andrew Gierth writes: > commit 807a40c5 fixed a bug in handling of (new in 9.2) functionality > of ScalarArrayOpExpr in btree index quals, forcing the results of > scans including such a qual to be treated as unordered (because the > order can in fact be wrong). > However, this kills any chance of using the same index _without_ the > SAOP to get the benefit of its ordering. Hm, good point. > 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. I'm not convinced that that's a-priori unreasonable, since the SAOP will result in multiple index scans under the hood. Conceivably we ought to try the path with and with SAOPs all the time. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
