Re: Hash Semi Join 5,000-50,000x slower on PG18 vs PG17 with 10+ equality columns and NULL values (identical plan, no spill)

On Fri, 31 Jul 2026 at 13:19, Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> commit 9ca67658d19e6c258eb4021a326ed7d38b3ab75f
> Author: David Rowley <drowley(at)postgresql(dot)org>
> Date: Thu Oct 17 14:25:08 2024 +1300
>
> Don't store intermediate hash values in ExprState->resvalue
>
> adf97c156 made it so ExprStates could support hashing and changed
> Hash Join to use that instead of manually extracting Datums from
> tuples and hashing them one column at a time.
>
> ...
>
> I don't quite see why, exactly. David, any ideas?

Looks like I used the wrong place to store the hash result when
aborting due to NULLs for EEOP_HASHDATUM_NEXT32_STRICT and
EEOP_HASHDATUM_FIRST_STRICT. Because this isn't an outer join, we
normally don't want to insert records with NULL hash keys into the
hash table. The *_STRICT operators are meant to handle this by
aborting early when we encounter a NULL, then MultiExecPrivateHash()
sees the isnull and heads to the "else if (node->keep_null_tuples)"
path instead.

You can see when assembling this expression in ExecBuildHash32Expr()
that I adjust the place where to store the result of the hash
depending on whether it's the final step or not. This code:

if (i == num_exprs - 1)
{
/* the result for hashing the final expr is stored in the state */
scratch.resvalue = &state->resvalue;
scratch.resnull = &state->resnull;
}
else
{
Assert(iresult != NULL);

/* intermediate values are stored in an intermediate result */
scratch.resvalue = &iresult->value;
scratch.resnull = &iresult->isnull;
}

So, if it's the last hash key we're hashing, store the result in the
ExprState's resvalue/resnull. Or if we're doing an intermediate hash
key, store it in the place for the intermediate result. The problem is
that if we abort before the final hash key due to finding a NULL, then
we only store the hashed value in the intermediate value location. We
then immediately do EEO_JUMP(op->d.hashdatum.jumpdone); which does not
pick up the intermediate value we just stored.

What should be happening is that when we abort early due to a NULL, we
should store that directly to state->resnull and state->resvalue.

I'm quite surprised this bug has lasted so long. It does seem only to
be a performance problem of hashing things we don't need and don't
ever look up again. I suspect most hash joins have a single hash key,
which are not affected by this, but still surprising since multi-key
hash joins are still very common.

Here's a WIP patch. I'm still looking at the JIT version to see if
that needs to be adjusted.

David