Re: optimize hashjoin

Hi,

It seems you responded by writing a new message and just copying the
subject, which unfortunately doesn't set the headers used for threading
(e.g. in archives). Please just respond to the message.

Or maybe your client does not set the References/In-Reply-To headers
correctly. Not sure which mail client you're using.

On 8/22/24 14:08, bucoo wrote:
>
>> 0) The patch does not apply anymore, thanks to David committing a patch
>
>> yesterday. Attached is a patch rebased on top of current master.
>
> That patch is based on PG17. I have now rewritten it based on the master
> branch and added some comments.
>

Thanks. Yeah, patches should be based on "master".

>
>> 1) Wouldn't it be easier (and just as efficient) to use slots with
>
>> TTSOpsMinimalTuple, i.e. backed by a minimal tuple?
>
> Use diffent kind of slot, the ExecEvalExpr function will report an error.
>

Hmm, I haven't tried so it's possible it wouldn't work.

>
>> 2) I find the naming of the new functions a bit confusing. We now have
>> the "original" functions working with slots, and then also functions
>> with "Tuple" working with tuples. Seems asymmetric.
>
> In net patch function name renamed to ExecHashTableInsertSlot and
> ExecHashTableInsertTuple,
>
> also ExecParallelHashTableInsertSlotCurrentBatch and
> ExecParallelHashTableInsertTupleCurrentBatch.
>

OK

>
>> 3) The code in ExecHashJoinOuterGetTuple is hard to understand, it'd
>> very much benefit from some comments. I'm a bit unsure if the likely()
>> and unlikely() hints really help.
>
> In new patch added some comments.
>
> "Likely" and "unlikely" might offer only marginal help on some CPUs and
> might not be beneficial at all on other platforms (I think).
>

Having such hints is an implicit suggestion it's beneficial. Otherwise
we'd just use them everywhere, but we don't - only a tiny fraction of
condition has those.

>
>> 4) Is the hj_outerTupleBuffer buffer really needed / effective? I'd bet
>> just using palloc() will work just as well, thanks to the AllocSet
>> caching etc.
>
> The hj_outerTupleBuffer avoid reform(materialize) tuple in
> non-TTSOpsMinimalTuple scenarios,
>
> see ExecForceStoreMinimalTuple. I added some comments in new patch.
>

AFAIK you mean this comment:

* mtup is hold in hjstate->hj_outerTupleBuffer, so we can using
* shouldFree as false to call ExecForceStoreMinimalTuple().
*
* When slot is TTSOpsMinimalTuple we can avoid realloc memory for
* new MinimalTuple(reuse StringInfo to call ExecHashJoinGetSavedTuple).

But my point was that I don't think the palloc/repalloc should be very
expensive, once the AllocSet warms up a bit.

* More importantly, in non-TTSOpsMinimalTuple scenarios, it can avoid
* reform(materialize) tuple(see ExecForceStoreMinimalTuple).

Yeah, but doesn't that conflate two things - materialization and freeing
the memory? Only because materialization is expensive, is that a good
reason to abandon the memory management too?

>
>> Can you provide more information about the benchmark you did? What
>> hardware, what scale, PostgreSQL configuration, which of the 22 queries
>> are improved, etc.
>>
>> I ran TPC-H with 1GB and 10GB scales on two machines, and I see pretty
>> much no difference compared to master. However, it occurred to me the
>> patch only ever helps if we increase the number of batches during
>> execution, in which case we need to move tuples to the right batch.
>
> Only parallel HashJoin speed up to ~2x(all data cached in memory),
>
> not full query, include non-parallel HashJoin.
>
> non-parallel HashJoin only when batchs large then one will speed up,
>
> because this patch only optimize for read batchs tuples to memory.
>

I'm sorry, but this does not answer *any* of the questions I asked.

Please provide enough info to reproduce the benefit - benchmark scale,
which query, which parameters, etc. Show explain / explain analyze of
the query without / with the patch, stuff like that.

I ran a number of TPC-H benchmarks with the patch and I never a benefit
of this scale.

regards

--
Tomas Vondra