From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Melanie Plageman <melanieplageman@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: HeikkiLinnakangas <hlinnaka@iki.fi>
Subject: Re: Trouble with hashagg spill I/O pattern and costing
Date: Fri, 29 May 2020 15:04:54 +0200
Message-ID: <20200529130454.el6qyuukj3ofs4eu@development> (raw)
In-Reply-To: <0fb3456095d469cc20a9ee7e76edfe9237dae24f.camel@j-davis.com>
References: <20200525225909.aifxjm53hw4xyzxd@development>
<CA+hUKG+q3Ma+sTKn2316Ofof6u21hh0cGJHcJ-fAnLxw=LXevw@mail.gmail.com>
<20200526141524.ohtvrp23vxosgivl@development>
<e913544f6c236d37e8b0597cabd086570b5806f2.camel@j-davis.com>
<20200526191511.d7pwz4awrvdm4p6j@development>
<0589f869969de2f3e9713689159c7c4fbe1fcfa5.camel@j-davis.com>
<CAAKRu_YNGQ-zw9rpXGtYhURMqdPk+HnMa+Wj-tnNx9cwENgDxg@mail.gmail.com>
<20200527090704.onkjogxeg34ib5cn@development>
<20200528185750.aamptuqdzrggqi5c@development>
<0fb3456095d469cc20a9ee7e76edfe9237dae24f.camel@j-davis.com>
On Thu, May 28, 2020 at 06:14:55PM -0700, Jeff Davis wrote:
>On Thu, 2020-05-28 at 20:57 +0200, Tomas Vondra wrote:
>> Attached is a patch adding CP_SMALL_TLIST to create_agg_plan and
>> create_groupingsets_plan.
>
>Looks good, except one question:
>
>Why would aggstrategy ever be MIXED when in create_agg_path? Wouldn't
>that only happen in create_groupingsets_path?
>
Ah, right. Yeah, we only need to check for AGG_HASH here. Moreover,
AGG_MIXED probably does not need the tlist tweak, because the input
should be pre-sorted as with AGG_SORTED.
And we should probably do similar check in the create_groupinsets_path,
I guess. At first I thought we can't do that before inspecting rollups,
which only happens later in the function, but now I see there's
aggstrategy in GroupingSetsPath too.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.cindex 9941dfe65e..eab57fd74b 100644--- a/src/backend/optimizer/plan/createplan.c+++ b/src/backend/optimizer/plan/createplan.c@@ -2113,12 +2113,21 @@ create_agg_plan(PlannerInfo *root, AggPath *best_path)
Plan *subplan;
List *tlist;
List *quals;
+ int flags;
/*
* Agg can project, so no need to be terribly picky about child tlist, but
- * we do need grouping columns to be available+ * we do need grouping columns to be available. We are a bit more careful+ * with hash aggregate, where we explicitly request small tlist to minimize+ * I/O needed for spilling (possibly not known already).
*/
- subplan = create_plan_recurse(root, best_path->subpath, CP_LABEL_TLIST);+ flags = CP_LABEL_TLIST;++ /* ensure small tlist for hash aggregate */+ if (best_path->aggstrategy == AGG_HASHED)+ flags |= CP_SMALL_TLIST;++ subplan = create_plan_recurse(root, best_path->subpath, flags);
tlist = build_path_tlist(root, &best_path->path);
@@ -2200,6 +2209,7 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
int maxref;
List *chain;
ListCell *lc;
+ int flags;
/* Shouldn't get here without grouping sets */
Assert(root->parse->groupingSets);
@@ -2207,9 +2217,17 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
/*
* Agg can project, so no need to be terribly picky about child tlist, but
- * we do need grouping columns to be available+ * we do need grouping columns to be available. We are a bit more careful+ * with hash aggregate, where we explicitly request small tlist to minimize+ * I/O needed for spilling (possibly not known already).
*/
- subplan = create_plan_recurse(root, best_path->subpath, CP_LABEL_TLIST);+ flags = CP_LABEL_TLIST;++ /* ensure small tlist for hash aggregate */+ if (best_path->aggstrategy == AGG_HASHED)+ flags |= CP_SMALL_TLIST;++ subplan = create_plan_recurse(root, best_path->subpath, flags);
/*
* Compute the mapping from tleSortGroupRef to column index in the child's
Attachments:
[text/plain] hashagg-tlist-fix-v2.patch (2.1K, ../20200529130454.el6qyuukj3ofs4eu@development/2-hashagg-tlist-fix-v2.patch)
download | inline diff:diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.cindex 9941dfe65e..eab57fd74b 100644--- a/src/backend/optimizer/plan/createplan.c+++ b/src/backend/optimizer/plan/createplan.c@@ -2113,12 +2113,21 @@ create_agg_plan(PlannerInfo *root, AggPath *best_path)
Plan *subplan;
List *tlist;
List *quals;
+ int flags;
/*
* Agg can project, so no need to be terribly picky about child tlist, but
- * we do need grouping columns to be available+ * we do need grouping columns to be available. We are a bit more careful+ * with hash aggregate, where we explicitly request small tlist to minimize+ * I/O needed for spilling (possibly not known already).
*/
- subplan = create_plan_recurse(root, best_path->subpath, CP_LABEL_TLIST);+ flags = CP_LABEL_TLIST;++ /* ensure small tlist for hash aggregate */+ if (best_path->aggstrategy == AGG_HASHED)+ flags |= CP_SMALL_TLIST;++ subplan = create_plan_recurse(root, best_path->subpath, flags);
tlist = build_path_tlist(root, &best_path->path);
@@ -2200,6 +2209,7 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
int maxref;
List *chain;
ListCell *lc;
+ int flags;
/* Shouldn't get here without grouping sets */
Assert(root->parse->groupingSets);
@@ -2207,9 +2217,17 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
/*
* Agg can project, so no need to be terribly picky about child tlist, but
- * we do need grouping columns to be available+ * we do need grouping columns to be available. We are a bit more careful+ * with hash aggregate, where we explicitly request small tlist to minimize+ * I/O needed for spilling (possibly not known already).
*/
- subplan = create_plan_recurse(root, best_path->subpath, CP_LABEL_TLIST);+ flags = CP_LABEL_TLIST;++ /* ensure small tlist for hash aggregate */+ if (best_path->aggstrategy == AGG_HASHED)+ flags |= CP_SMALL_TLIST;++ subplan = create_plan_recurse(root, best_path->subpath, flags);
/*
* Compute the mapping from tleSortGroupRef to column index in the child's
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, pgsql@j-davis.com, melanieplageman@gmail.com, thomas.munro@gmail.com, hlinnaka@iki.fi
Subject: Re: Trouble with hashagg spill I/O pattern and costing
In-Reply-To: <20200529130454.el6qyuukj3ofs4eu@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox