Date: Mon, 27 Apr 2020 11:18:23 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>,
	Richard Guo <guofenglinux@gmail.com>,
	Thomas Munro <thomas.munro@gmail.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: weird hash plan cost, starting with pg10
Message-ID: <20200427161823.GY28974@telsasoft.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>
 <4751e4df-5f98-657e-fb88-c039dd1e7a31@postgrespro.ru>
 <31321.1586549487@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <31321.1586549487@sss.pgh.pa.us>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Fri, Apr 10, 2020 at 04:11:27PM -0400, Tom Lane wrote:
> I'm not sure it's worth any risk though.  A much simpler
> fix is to make sure we clear the dangling hashtable pointer, as in
> 0002 below (a simplified form of Konstantin's patch).  The net
> effect of that is that in the case where a hash table is destroyed
> and never rebuilt, EXPLAIN ANALYZE would report no hash stats,
> rather than possibly-garbage stats like it does today.  That's
> probably good enough, because it should be an uncommon corner case.
> 
> Thoughts?

Checking if you're planning to backpatch this ?

> diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
> index c901a80..9e28ddd 100644
> --- a/src/backend/executor/nodeHashjoin.c
> +++ b/src/backend/executor/nodeHashjoin.c
> @@ -1336,6 +1336,12 @@ ExecReScanHashJoin(HashJoinState *node)
>  		else
>  		{
>  			/* must destroy and rebuild hash table */
> +			HashState  *hashNode = castNode(HashState, innerPlanState(node));
> +
> +			/* for safety, be sure to clear child plan node's pointer too */
> +			Assert(hashNode->hashtable == node->hj_HashTable);
> +			hashNode->hashtable = NULL;
> +
>  			ExecHashTableDestroy(node->hj_HashTable);
>  			node->hj_HashTable = NULL;
>  			node->hj_JoinState = HJ_BUILD_HASHTABLE;


-- 
Justin