Hi,
For v6-0003-Parallel-Hash-Full-Right-Outer-Join.patch
+ * current_chunk_idx: index in current HashMemoryChunk
The above comment seems to be better fit for ExecScanHashTableForUnmatched(), instead of ExecParallelPrepHashTableForUnmatched.
I wonder where current_chunk_idx should belong (considering the above comment and what the code does).
+ while (hashtable->current_chunk_idx < hashtable->current_chunk->used)
...
+ next = hashtable->current_chunk->next.unshared;
+ hashtable->current_chunk = next;
+ hashtable->current_chunk_idx = 0;
Each time we advance to the next chunk, current_chunk_idx is reset. It seems current_chunk_idx can be placed inside chunk.
Maybe the consideration is that, with the current formation we save space by putting current_chunk_idx field at a higher level.
If that is the case, a comment should be added.
Cheers
On Tue, Mar 2, 2021 at 11:27 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Fri, Feb 12, 2021 at 11:02 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > I just attached the diff.
>
> Squashed into one patch for the cfbot to chew on, with a few minor
> adjustments to a few comments.
I did some more minor tidying of comments and naming. It's been on my
to-do-list to update some phase names after commit 3048898e, and while
doing that I couldn't resist the opportunity to change DONE to FREE,
which somehow hurts my brain less, and makes much more obvious sense
after the bugfix in CF #3031 that splits DONE into two separate
phases. It also pairs obviously with ALLOCATE. I include a copy of
that bugix here too as 0001, because I'll likely commit that first, so
I rebased the stack of patches that way. 0002 includes the renaming I
propose (master only). Then 0003 is Melanie's patch, using the name
SCAN for the new match bit scan phase. I've attached an updated
version of my "phase diagram" finger painting, to show how it looks
with these three patches. "scan*" is new.
