From f867ce5518202a4e625dc41b7036fec47ee0e09e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 7 Feb 2016 12:29:17 -0500 Subject: [PATCH] ExecHashRemoveNextSkewBucket must physically copy tuples to main hashtable. Commit 45f6240a8fa9d355 added an assumption in ExecHashIncreaseNumBatches and ExecHashIncreaseNumBuckets that they could find all tuples in the main hash table by iterating over the "dense storage" introduced by that patch. However, ExecHashRemoveNextSkewBucket continued its old practice of simply re-linking deleted skew tuples into the main table's hashchains. Hence, such tuples got lost during any subsequent increase in nbatch or nbuckets, and would never get joined, as reported in bug #13908 from Seth P. I (tgl) think that the aforesaid commit has got multiple design issues and should be reworked rather completely; but there is no time for that right now, so band-aid the problem by making ExecHashRemoveNextSkewBucket physically copy deleted skew tuples into the "dense storage" arena. The added test case is able to exhibit the problem by means of fooling the planner with a WHERE condition that it will underestimate the selectivity of, causing the initial nbatch estimate to be too small. Tomas Vondra and Tom Lane. Thanks to David Johnston for initial investigation into the bug report. --- src/backend/executor/nodeHash.c | 15 +++++++++++++-- src/test/regress/expected/join.out | 28 ++++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 16 ++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c index 47160e4aa0..9ed09a7b0c 100644 --- a/src/backend/executor/nodeHash.c +++ b/src/backend/executor/nodeHash.c @@ -1575,8 +1575,19 @@ ExecHashRemoveNextSkewBucket(HashJoinTable hashtable) if (batchno == hashtable->curbatch) { /* Move the tuple to the main hash table */ - hashTuple->next = hashtable->buckets[bucketno]; - hashtable->buckets[bucketno] = hashTuple; + HashJoinTuple copyTuple; + + /* + * We must copy the tuple into the dense storage, else it will not + * be found by, eg, ExecHashIncreaseNumBatches. + */ + copyTuple = (HashJoinTuple) dense_alloc(hashtable, tupleSize); + memcpy(copyTuple, hashTuple, tupleSize); + pfree(hashTuple); + + copyTuple->next = hashtable->buckets[bucketno]; + hashtable->buckets[bucketno] = copyTuple; + /* We have reduced skew space, but overall space doesn't change */ hashtable->spaceUsedSkew -= tupleSize; } diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 64f046ee0b..0ac21bb813 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2331,6 +2331,34 @@ select tt1.*, tt2.* from tt2 right join tt1 on tt1.joincol = tt2.joincol; reset enable_hashjoin; reset enable_nestloop; -- +-- regression test for bug #13908 (hash join with skew tuples & nbatch increase) +-- +set work_mem to '64kB'; +set enable_mergejoin to off; +explain (costs off) +select count(*) from tenk1 a, tenk1 b + where a.hundred = b.thousand and (b.fivethous % 10) < 10; + QUERY PLAN +------------------------------------------------------------ + Aggregate + -> Hash Join + Hash Cond: (a.hundred = b.thousand) + -> Index Only Scan using tenk1_hundred on tenk1 a + -> Hash + -> Seq Scan on tenk1 b + Filter: ((fivethous % 10) < 10) +(7 rows) + +select count(*) from tenk1 a, tenk1 b + where a.hundred = b.thousand and (b.fivethous % 10) < 10; + count +-------- + 100000 +(1 row) + +reset work_mem; +reset enable_mergejoin; +-- -- regression test for 8.2 bug with improper re-ordering of left joins -- create temp table tt3(f1 int, f2 text); diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 0358d00e40..fafbb3fb00 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -463,6 +463,22 @@ select tt1.*, tt2.* from tt2 right join tt1 on tt1.joincol = tt2.joincol; reset enable_hashjoin; reset enable_nestloop; +-- +-- regression test for bug #13908 (hash join with skew tuples & nbatch increase) +-- + +set work_mem to '64kB'; +set enable_mergejoin to off; + +explain (costs off) +select count(*) from tenk1 a, tenk1 b + where a.hundred = b.thousand and (b.fivethous % 10) < 10; +select count(*) from tenk1 a, tenk1 b + where a.hundred = b.thousand and (b.fivethous % 10) < 10; + +reset work_mem; +reset enable_mergejoin; + -- -- regression test for 8.2 bug with improper re-ordering of left joins -- -- 2.40.0