From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
To: Richard Guo <guofenglinux@gmail.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: weird hash plan cost, starting with pg10
Date: Wed, 25 Mar 2020 17:29:59 +0300
Message-ID: <4751e4df-5f98-657e-fb88-c039dd1e7a31@postgrespro.ru> (raw)
In-Reply-To: <CAMbWs4-oR+V1AtR7j8Nv85-n1eEng6Z4NmHvcJLEpsb69x5JMw@mail.gmail.com>
References: <20200323165059.GA24950@alvherre.pgsql>
<18131.1584982859@sss.pgh.pa.us>
<CA+hUKGKC3Wzkq3bB0fpYx3H=AZWG6GnU5BcsuEaCYdwQbEY3vA@mail.gmail.com>
<CA+hUKGKoxhHo07+CoQs30dyM1rtCaRxndhDK1RfBvLvNFcwjoQ@mail.gmail.com>
<CAMbWs4_+CLFi+mgEkVk7u0YW0DK6y4TWLe6Hrt5pTfYh2jipRg@mail.gmail.com>
<CAMbWs4-oR+V1AtR7j8Nv85-n1eEng6Z4NmHvcJLEpsb69x5JMw@mail.gmail.com>
On 25.03.2020 13:36, Richard Guo wrote:
>
> On Tue, Mar 24, 2020 at 3:36 PM Richard Guo <guofenglinux@gmail.com
> <mailto:guofenglinux@gmail.com>> wrote:
>
> On Tue, Mar 24, 2020 at 11:05 AM Thomas Munro
> <thomas.munro@gmail.com <mailto: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
Attachments:
[text/x-patch] hash_join_instrumentation.patch (1.7K, ../4751e4df-5f98-657e-fb88-c039dd1e7a31@postgrespro.ru/3-hash_join_instrumentation.patch)
download | inline diff:diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.cindex 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.cindex 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.hindex 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;
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: k.knizhnik@postgrespro.ru, guofenglinux@gmail.com, thomas.munro@gmail.com, tgl@sss.pgh.pa.us, alvherre@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: weird hash plan cost, starting with pg10
In-Reply-To: <4751e4df-5f98-657e-fb88-c039dd1e7a31@postgrespro.ru>
* 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