Re: weird hash plan cost, starting with pg10

On Sat, Apr 11, 2020 at 4:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
> > On 25.03.2020 13:36, Richard Guo wrote:
> >> 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.
>
> > I can propose the following patch for the problem.
>
> I looked at this patch a bit, and I don't think it goes far enough.
> What this issue is really pointing out is that EXPLAIN is not considering
> the possibility of a Hash node having had several hashtable instantiations
> over its lifespan.  I propose what we do about that is generalize the
> policy that show_hash_info() is already implementing (in a rather half
> baked way) for multiple workers, and report the maximum field values
> across all instantiations.  We can combine the code needed to do so
> with the code for the parallelism case, as shown in the 0001 patch
> below.
>

I looked through 0001 patch and it looks good to me.

At first I was wondering if we need to check whether HashState.hashtable
is not NULL in ExecShutdownHash() before we decide to allocate save
space for HashState.hinstrument. And then I convinced myself that that's
not necessary since HashState.hinstrument and HashState.hashtable cannot
be both NULL there.


>
> In principle we could probably get away with back-patching 0001,
> at least into branches that already have the HashState.hinstrument
> pointer.  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.
>

Yes it's an uncommon corner case. But I think it may still surprise
people that most of the time the hash stat shows well but sometimes it
does not.

Thanks
Richard