Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.92) (envelope-from ) id 1jH72q-0006XP-1x for pgsql-hackers@arkaria.postgresql.org; Wed, 25 Mar 2020 14:30:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1jH72o-0007tw-G8 for pgsql-hackers@arkaria.postgresql.org; Wed, 25 Mar 2020 14:30:10 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1jH72n-0007tp-Vj for pgsql-hackers@lists.postgresql.org; Wed, 25 Mar 2020 14:30:10 +0000 Received: from cyclops.postgrespro.ru ([93.174.131.138] helo=mail.postgrespro.ru) by makus.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jH72g-00082C-6M for pgsql-hackers@lists.postgresql.org; Wed, 25 Mar 2020 14:30:08 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id 8293A21C5664; Wed, 25 Mar 2020 17:29:59 +0300 (MSK) X-Virus-Scanned: Debian amavisd-new at postgrespro.ru X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=x tagged_above=-99 required=4 WHITELISTED tests=[] autolearn=unavailable Received: from [192.168.27.200] (gw.postgrespro.ru [93.174.131.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mail.postgrespro.ru (Postfix) with ESMTPSA id 4273321C4D95; Wed, 25 Mar 2020 17:29:59 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mail; t=1585146599; bh=ziX6NOphFPu+KRWSn85I9sLznmoqJdie1KjsBhAQFWg=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=qJpTtyhyX4/HpHKkdCp9d0hKeJYj6LQEh4+XjkyadO0/O7l51Hsg1evqvHzBueMuz SGLpVKbdgUwyKHPte6MxZWLMTK2biSWQJi9AA4JEA6jrL2L42Zl/pysoDeHEy435NW 28eC+9wP8lsDd7iyq3NUrJqnVbw9UlUlxg2DSlZY= Subject: Re: weird hash plan cost, starting with pg10 To: Richard Guo , Thomas Munro Cc: Tom Lane , Alvaro Herrera , Pg Hackers References: <20200323165059.GA24950@alvherre.pgsql> <18131.1584982859@sss.pgh.pa.us> From: Konstantin Knizhnik Message-ID: <4751e4df-5f98-657e-fb88-c039dd1e7a31@postgrespro.ru> Date: Wed, 25 Mar 2020 17:29:59 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------7D790F62271882338D8B485D" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --------------7D790F62271882338D8B485D Content-Type: multipart/alternative; boundary="------------94865DFC16D9FF4E59F2E191" --------------94865DFC16D9FF4E59F2E191 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 25.03.2020 13:36, Richard Guo wrote: > > On Tue, Mar 24, 2020 at 3:36 PM Richard Guo > wrote: > > On Tue, Mar 24, 2020 at 11:05 AM Thomas Munro > > wrote: > > > I think there might be a case like this: > > * ExecRescanHashJoin() decides it can't reuse the hash table for a > rescan, so it calls ExecHashTableDestroy(), clears HashJoinState's > hj_HashTable and sets hj_JoinState to HJ_BUILD_HASHTABLE > * the HashState node still has a reference to the pfree'd > HashJoinTable! > * HJ_BUILD_HASHTABLE case reaches the empty-outer optimisation > case so > it doesn't bother to build a new hash table > * EXPLAIN examines the HashState's pointer to a freed > HashJoinTable struct > > > Yes, debugging with gdb shows this is exactly what happens. > > > According to the scenario above, here is a recipe that reproduces this > issue. > > -- recipe start > create table a(i int, j int); > create table b(i int, j int); > create table c(i int, j int); > > insert into a select 3,3; > insert into a select 2,2; > insert into a select 1,1; > > insert into b select 3,3; > > insert into c select 0,0; > > analyze a; > analyze b; > analyze c; > > set enable_nestloop to off; > set enable_mergejoin to off; > > explain analyze > select exists(select * from b join c on a.i > c.i and a.i = b.i and > b.j = c.j) from a; > -- recipe end > > I tried this recipe on different PostgreSQL versions, starting from > current master and going backwards. I was able to reproduce this issue > on all versions above 8.4. In 8.4 version, we do not output information > on hash buckets/batches. But manual inspection with gdb shows in 8.4 we > also have the dangling pointer for HashState->hashtable. I didn't check > versions below 8.4 though. > > Thanks > Richard I can propose the following patch for the problem. -- Konstantin Knizhnik Postgres Professional: http://www.postgrespro.com The Russian Postgres Company --------------94865DFC16D9FF4E59F2E191 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit

On 25.03.2020 13:36, Richard Guo wrote:

On Tue, Mar 24, 2020 at 3:36 PM Richard Guo <guofenglinux@gmail.com> wrote:
On Tue, Mar 24, 2020 at 11:05 AM Thomas Munro <thomas.munro@gmail.com> wrote:

I think there might be a case like this:

* ExecRescanHashJoin() decides it can't reuse the hash table for a
rescan, so it calls ExecHashTableDestroy(), clears HashJoinState's
hj_HashTable and sets hj_JoinState to HJ_BUILD_HASHTABLE
* the HashState node still has a reference to the pfree'd HashJoinTable!
* HJ_BUILD_HASHTABLE case reaches the empty-outer optimisation case so
it doesn't bother to build a new hash table
* EXPLAIN examines the HashState's pointer to a freed HashJoinTable struct

Yes, debugging with gdb shows this is exactly what happens.

According to the scenario above, here is a recipe that reproduces this
issue.

-- recipe start
create table a(i int, j int);
create table b(i int, j int);
create table c(i int, j int);

insert into a select 3,3;
insert into a select 2,2;
insert into a select 1,1;

insert into b select 3,3;

insert into c select 0,0;

analyze a;
analyze b;
analyze c;

set enable_nestloop to off;
set enable_mergejoin to off;

explain analyze
select exists(select * from b join c on a.i > c.i and a.i = b.i and b.j = c.j) from a;
-- recipe end

I tried this recipe on different PostgreSQL versions, starting from
current master and going backwards. I was able to reproduce this issue
on all versions above 8.4. In 8.4 version, we do not output information
on hash buckets/batches. But manual inspection with gdb shows in 8.4 we
also have the dangling pointer for HashState->hashtable. I didn't check
versions below 8.4 though.

Thanks
Richard

I can propose the following patch for the problem.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--------------94865DFC16D9FF4E59F2E191-- --------------7D790F62271882338D8B485D Content-Type: text/x-patch; name="hash_join_instrumentation.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="hash_join_instrumentation.patch" diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index ff2f45c..470073b 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -2685,7 +2685,8 @@ show_hash_info(HashState *hashstate, ExplainState *es) */ if (hashstate->hashtable) ExecHashGetInstrumentation(&hinstrument, hashstate->hashtable); - + else + hinstrument = hashstate->saved_instrument; /* * Merge results from workers. In the parallel-oblivious case, the * results from all participants should be identical, except where diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c index c901a80..db082be 100644 --- a/src/backend/executor/nodeHashjoin.c +++ b/src/backend/executor/nodeHashjoin.c @@ -1336,6 +1336,13 @@ ExecReScanHashJoin(HashJoinState *node) else { /* must destroy and rebuild hash table */ + HashState* state = (HashState*)innerPlanState(node); + Assert(state->hashtable == node->hj_HashTable); + if (state->ps.instrument) + { + ExecHashGetInstrumentation(&state->saved_instrument, state->hashtable); + } + state->hashtable = NULL; ExecHashTableDestroy(node->hj_HashTable); node->hj_HashTable = NULL; node->hj_JoinState = HJ_BUILD_HASHTABLE; diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 3d27d50..94afc18 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -2302,6 +2302,8 @@ typedef struct HashState SharedHashInfo *shared_info; /* one entry per worker */ HashInstrumentation *hinstrument; /* this worker's entry */ + HashInstrumentation saved_instrument; /* save here instrumentation for dropped hash */ + /* Parallel hash state. */ struct ParallelHashJoinState *parallel_state; } HashState; --------------7D790F62271882338D8B485D--