Horton

Thread

Commits

  1. Don't store intermediate hash values in ExprState->resvalue

  1. Fixing Hash Join bug I caused with adf97c156

    David Rowley <dgrowleyml@gmail.com> — 2024-10-16T01:10:06Z

    Yesterday Andres mentioned to me that he's getting wrong results with
TPCH-Q02. Andres did the analysis to figure out that this was caused
by adf97c156 due to how I chose to store intermediate hash values when
the Hash Join has multiple join keys.

Per Andres' investigation, it seems what's going on is that
EEOP_PARAM_SET calls ExecEvalParamSet() and that sets the param value
from the ExprState->resvalue, the same location as the previous
hashing step stored its intermediate result.

It's probably not very good practice to store intermediate things in
ExprState->resvalue and expect them to still be there after evaluation
of steps that you might not have any control over, so the attached
adjust things to add a dedicated location for the intermediate hash
value and adjusts the step generation code to store all apart from the
final hashing step into that location.

I tested this with TPCH-Q02 and the results are correct again (when
compared with SET enable_hashjoin=0). I tried both with JIT on and
off.

The Hash Join in question looks like:

 ->  Hash Join  (cost=137144.04..630694.61 rows=1 width=197)
               Hash Cond: ((part.p_partkey = partsupp.ps_partkey) AND
((SubPlan 1) = partsupp.ps_supplycost))

I plan to push this soon, but if anyone wants to look over it, I'll
leave it here for a while before doing so.

David

  2. Re: Fixing Hash Join bug I caused with adf97c156

    Alena Rybakina <a.rybakina@postgrespro.ru> — 2024-10-16T01:22:29Z

    Hi!

On 16.10.2024 04:10, David Rowley wrote:
> Yesterday Andres mentioned to me that he's getting wrong results with
> TPCH-Q02. Andres did the analysis to figure out that this was caused
> by adf97c156 due to how I chose to store intermediate hash values when
> the Hash Join has multiple join keys.
>
> Per Andres' investigation, it seems what's going on is that
> EEOP_PARAM_SET calls ExecEvalParamSet() and that sets the param value
> from the ExprState->resvalue, the same location as the previous
> hashing step stored its intermediate result.
>
> It's probably not very good practice to store intermediate things in
> ExprState->resvalue and expect them to still be there after evaluation
> of steps that you might not have any control over, so the attached
> adjust things to add a dedicated location for the intermediate hash
> value and adjusts the step generation code to store all apart from the
> final hashing step into that location.
>
> I tested this with TPCH-Q02 and the results are correct again (when
> compared with SET enable_hashjoin=0). I tried both with JIT on and
> off.
>
> The Hash Join in question looks like:
>
>   ->  Hash Join  (cost=137144.04..630694.61 rows=1 width=197)
>                 Hash Cond: ((part.p_partkey = partsupp.ps_partkey) AND
> ((SubPlan 1) = partsupp.ps_supplycost))
>
> I plan to push this soon, but if anyone wants to look over it, I'll
> leave it here for a while before doing so.
>

Thank you for the work on this issue!

I haven't noticed anything wrong yet. I think it's worth adding a test 
to regression tests, isn't it?

-- 
Regards,
Alena Rybakina
Postgres Professional

  3. Re: Fixing Hash Join bug I caused with adf97c156

    David Rowley <dgrowleyml@gmail.com> — 2024-10-16T02:21:44Z

    On Wed, 16 Oct 2024 at 14:22, Alena Rybakina <a.rybakina@postgrespro.ru> wrote:
> I haven't noticed anything wrong yet. I think it's worth adding a test
> to regression tests, isn't it?

Here's a patch including a test this time.

I had imagined that it would be hard to make the same mistake again...

David

  4. Re: Fixing Hash Join bug I caused with adf97c156

    David Rowley <dgrowleyml@gmail.com> — 2024-10-17T01:25:51Z

    On Wed, 16 Oct 2024 at 15:21, David Rowley <dgrowleyml@gmail.com> wrote:
> Here's a patch including a test this time.

I've pushed this patch. Thanks for looking.

David