Suspicious call of initial_cost_hashjoin()
try_partial_hashjoin_path() passes constant true to for the parallel_hash
argument of initial_cost_hashjoin(). Shouldn't it instead pass the
parallel_hash argument that it receives?
This is related to commit 1804284042e659e7d16904e7bbb0ad546394b6a3.
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at
On Fri, Dec 22, 2017 at 10:45 PM, Antonin Houska <ah@cybertec.at> wrote:
try_partial_hashjoin_path() passes constant true to for the parallel_hash
argument of initial_cost_hashjoin(). Shouldn't it instead pass the
parallel_hash argument that it receives?
Thanks. Yeah. When initial_cost_hashjoin() calls
get_parallel_divisor() on a non-partial inner path I think it would
return 1.0, so no damage was done there, but when
ExecChooseHashTableSize() receives try_combined_work_mem == true it
might underestimate the number of batches required for a partial hash
join without parallel hash, because it would incorrectly assume that a
single batch join could use the combined work_mem budget. This was
quite well hidden because ExecHashTableCreate() calls
ExecChooseHashTableSize() again (rather than reusing the results from
planning time), so the bad nbatch estimate doesn't show up anywhere.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
fix.patchapplication/octet-stream; name=fix.patchDownload
From 4f8a8c832fdf323e0d70c3134ec9516c161582d0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Fri, 22 Dec 2017 23:58:15 +1300
Subject: [PATCH] Fix costing of parallel hash joins.
Commit 1804284042e659e7d16904e7bbb0ad546394b6a3 established that single-batch
parallel-aware hash joins could create one large shared hash table using the
combined work_mem budget of all participants. The costing accidentally
assumed that parallel-oblivious hash joins could also do that. Repair.
Reported-By: Antonin Houska
Discussion: https://postgr.es/m/12441.1513935950%40localhost
---
src/backend/optimizer/path/joinpath.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index e774130ac8d..bc670cba4fe 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -814,7 +814,7 @@ try_partial_hashjoin_path(PlannerInfo *root,
* cost. Bail out right away if it looks terrible.
*/
initial_cost_hashjoin(root, &workspace, jointype, hashclauses,
- outer_path, inner_path, extra, true);
+ outer_path, inner_path, extra, parallel_hash);
if (!add_partial_path_precheck(joinrel, workspace.total_cost, NIL))
return;
--
2.15.0
Hi Antonin,
On 12/22/17 6:13 AM, Thomas Munro wrote:
On Fri, Dec 22, 2017 at 10:45 PM, Antonin Houska <ah@cybertec.at> wrote:
try_partial_hashjoin_path() passes constant true to for the parallel_hash
argument of initial_cost_hashjoin(). Shouldn't it instead pass the
parallel_hash argument that it receives?Thanks. Yeah. When initial_cost_hashjoin() calls
get_parallel_divisor() on a non-partial inner path I think it would
return 1.0, so no damage was done there, but when
ExecChooseHashTableSize() receives try_combined_work_mem == true it
might underestimate the number of batches required for a partial hash
join without parallel hash, because it would incorrectly assume that a
single batch join could use the combined work_mem budget. This was
quite well hidden because ExecHashTableCreate() calls
ExecChooseHashTableSize() again (rather than reusing the results from
planning time), so the bad nbatch estimate doesn't show up anywhere.
Does this look right to you? If so, can you sign up as a reviewer and
mark it Ready for Committer?
Thanks,
--
-David
david@pgmasters.net
David Steele <david@pgmasters.net> wrote:
On 12/22/17 6:13 AM, Thomas Munro wrote:
On Fri, Dec 22, 2017 at 10:45 PM, Antonin Houska <ah@cybertec.at> wrote:
try_partial_hashjoin_path() passes constant true to for the parallel_hash
argument of initial_cost_hashjoin(). Shouldn't it instead pass the
parallel_hash argument that it receives?Thanks. Yeah. When initial_cost_hashjoin() calls
get_parallel_divisor() on a non-partial inner path I think it would
return 1.0, so no damage was done there, but when
ExecChooseHashTableSize() receives try_combined_work_mem == true it
might underestimate the number of batches required for a partial hash
join without parallel hash, because it would incorrectly assume that a
single batch join could use the combined work_mem budget. This was
quite well hidden because ExecHashTableCreate() calls
ExecChooseHashTableSize() again (rather than reusing the results from
planning time), so the bad nbatch estimate doesn't show up anywhere.
Does this look right to you?
Yes, this is what I meant. The patch applies cleanly and the code compiles
well.
If so, can you sign up as a reviewer and mark it Ready for Committer?
Done.
Actually I think it'd be nice if the "parallel_hash" argument was mentioned in
the header comment of initial_cost_hashjoin() function, but not sure this is
worth returning the patch to the author.
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
On Fri, Mar 2, 2018 at 9:06 PM, Antonin Houska <ah@cybertec.at> wrote:
David Steele <david@pgmasters.net> wrote:
Does this look right to you?
Yes, this is what I meant. The patch applies cleanly and the code compiles
well.If so, can you sign up as a reviewer and mark it Ready for Committer?
Done.
Thanks.
Actually I think it'd be nice if the "parallel_hash" argument was mentioned in
the header comment of initial_cost_hashjoin() function, but not sure this is
worth returning the patch to the author.
Done.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
0001-Fix-costing-of-parallel-hash-joins-v2.patchapplication/octet-stream; name=0001-Fix-costing-of-parallel-hash-joins-v2.patchDownload
From d817d719c63c014f28376251012f56275e100d18 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Fri, 22 Dec 2017 23:58:15 +1300
Subject: [PATCH] Fix costing of parallel hash joins.
Commit 1804284042e659e7d16904e7bbb0ad546394b6a3 established that single-batch
parallel-aware hash joins could create one large shared hash table using the
combined work_mem budget of all participants. The costing accidentally
assumed that parallel-oblivious hash joins could also do that. The
documentation for initial_cost_hashjoin() also failed to mention the new
argument. Repair.
Author: Thomas Munro
Reported-By: Antonin Houska
Reviewed-By: Antonin Houska
Discussion: https://postgr.es/m/12441.1513935950%40localhost
---
src/backend/optimizer/path/costsize.c | 2 ++
src/backend/optimizer/path/joinpath.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index d8db0b29e1..36b3dfabb8 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -3143,6 +3143,8 @@ cached_scansel(PlannerInfo *root, RestrictInfo *rinfo, PathKey *pathkey)
* 'outer_path' is the outer input to the join
* 'inner_path' is the inner input to the join
* 'extra' contains miscellaneous information about the join
+ * 'parallel_hash' indicates that inner_path is partial and that a shared
+ * hash table will be built in parallel
*/
void
initial_cost_hashjoin(PlannerInfo *root, JoinCostWorkspace *workspace,
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 688f440b92..3fd3cc7670 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -814,7 +814,7 @@ try_partial_hashjoin_path(PlannerInfo *root,
* cost. Bail out right away if it looks terrible.
*/
initial_cost_hashjoin(root, &workspace, jointype, hashclauses,
- outer_path, inner_path, extra, true);
+ outer_path, inner_path, extra, parallel_hash);
if (!add_partial_path_precheck(joinrel, workspace.total_cost, NIL))
return;
--
2.15.1
On 3/2/18 05:01, Thomas Munro wrote:
On Fri, Mar 2, 2018 at 9:06 PM, Antonin Houska <ah@cybertec.at> wrote:
David Steele <david@pgmasters.net> wrote:
Does this look right to you?
Yes, this is what I meant. The patch applies cleanly and the code compiles
well.If so, can you sign up as a reviewer and mark it Ready for Committer?
Done.
Thanks.
Actually I think it'd be nice if the "parallel_hash" argument was mentioned in
the header comment of initial_cost_hashjoin() function, but not sure this is
worth returning the patch to the author.Done.
committed
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services