Minor: Remove memory reservation in JoinLeftData used in HashJoin

[jayzhan-synnada]

Which issue does this PR close?

Closes #.

Rationale for this change

memory reservation is not used outside of collect_left_input, I don't think there is any good reason to add it in JoinLeftData. Remove this so we don't need to create dummy value for reservation when we create JoinLeftData

What changes are included in this PR?

Are these changes tested?

Also, Debug and Clone for HashJoinStreamState

Are there any user-facing changes?

[comphead]

Good catch. Some day ago it was intention to add spilling to HJ #12952
but even for that case the mem reservation on left side is not needed

[ozankabak]

LGTM

[korowa]

In case reservation is not stored in JoinLeftData, it'll be dropped right after collect_left_input due to being unneeded anymore. And due to MemoryReservation calls free on its drop, it looks like that with this patch, during the whole join execution build side data will be untracked by memory pool.

So the initial idea of this attribute was to make reservation to live as long as JoinLeftData exists.

[korowa]

UPD: some "logs" (additional prints 😞 ) just to check join execution before/after this patch:

// Before
---- joins::hash_join::tests::join_inner_one_no_shared_column_names stdout ----
Waiting build side
Reserving 324 bytes
Reserving 124 bytes
Fetching probe batch
Processing probe batch
Fetching probe batch
Processing unmatched build side
Freeing 448 bytes

// After
---- joins::hash_join::tests::join_inner_one_no_shared_column_names stdout ----
Waiting build side
Reserving 324 bytes
Reserving 124 bytes
Freeing 448 bytes
Fetching probe batch
Processing probe batch
Fetching probe batch
Processing unmatched build side

so yes, the memory is "freed" before join completes its execution, which doesn't seem to be an expected behavior.

[jayzhan211]

The reason I think this is not an issue is because it only matters when we run 'collect_left_join', we track the memory and free after the build side is done. Although, the memory is clean while the hash map is not freed. BUT, since we don't have expected memory change in the probe stage, the memory is not helpful for the probe stage. That's why I think this will not cause any issues if we drop the reservation early.

[korowa]

Although, the memory is clean while the hash map is not freed

That actually is the issue, because there is single memory pool for a RuntimeEnv, and it's intended to be used by multiple sessions or at least by the multiple operators within the same query. Even in case of single query like select * from a join b order by a.field untracked build side may cause OOM due to SortExec will consider MemoryPool as being empty, while there is a build side data in memory.

[jayzhan211]

After join, we can release hash table memory on the build side, why will we keep it when we are running Sort or other upstream operator?

[jayzhan211]

Waiting build side
Reserving 324 bytes
Reserving 124 bytes
Fetching probe batch
Processing probe batch
Fetching probe batch
Processing unmatched build side
Freeing 448 bytes

-> SortExec, no memory used for join

[korowa]

Because all operators streams are running simultaneously, consuming each other and requesting memory from the pool. For the query above (select * from a join b order by a.field) the timeline will be roughly like following:

1. Sort stream is started and waiting for the Join results
2. Join stream is started, and in order to produce some output it collects build side data reserving memory in the pool
3. Join stream completed with build side and produces the output batch (build side is still alive since it's required for producing join output)
4. Sort stream receives input batch from the join, and starts accumulating + sorting the data, making memory reservations in memory pool
5. steps 3-4 are repeated multiple times, while more and more memory being reserved by Sort (since it needs to read and sort all the input before producing its output)
6. Join completes processing of probe side, returns None
7. Sort completes processing of join output, finishes processing of internally accumulated data, and gets destroyed freeing OS memory
8. Join is not required anymore, and it also dropped freeing OS memory

Previously, the memory in the pool for join build side was "freed" on step 8, so while sort operator requested for additional memory, there was information in the pool that build side data still exists and takes N bytes, and now, after this patch, the reservation is deleted in the end of step 2.

So, now when Sort will start requesting for the memory, DF will report that memory pool is empty, while build side still exists in OS memory, which is a potential source of OOM -- e.g:

• running on 5G memory machine
• having memory pool set to 4G
• having build side with the size of 3G
• having probe side with the size of 10G

At the start of Sort input processing (step 4 from above) DF will think that there are 4G available in the pool (while actually OS will have 2G available, due to in-memory join build side), so the sort will try to accumulate up to 4G before spilling, and likely will be OOM killed after reading ~2G of data (when OS memory will be exhausted).

[korowa]

And anyway -- the general idea of MemoryPool is to "mirror" OS memory usage for potentially unbounded memory consumers (such as join build side / aggregation accumulator / etc), so I don't think that removal of memory tracking from one of these consumers is a proper direction to go.

[jayzhan211]

I see. This makes sense.