On Tue, Dec 29, 2020 at 03:28:12PM +1300, Thomas Munro wrote:
> I had some feedback I meant to
> post in November but didn't get around to:
>
> * PHJ_BATCH_PROBING -- all probe
> - * PHJ_BATCH_DONE -- end
> +
> + * PHJ_BATCH_DONE -- queries not requiring inner fill done
> + * PHJ_BATCH_FILL_INNER_DONE -- inner fill completed, all queries done
>
> Would it be better/tidier to keep _DONE as the final phase? That is,
> to switch around these two final phases. Or does that make it too
> hard to coordinate the detach-and-cleanup logic?
I updated this to use your suggestion. My rationale for having
PHJ_BATCH_DONE and then PHJ_BATCH_FILL_INNER_DONE was that, for a worker
attaching to the batch for the first time, it might be confusing that it
is in the PHJ_BATCH_FILL_INNER state (not the DONE state) and yet that
worker still just detaches and moves on. It didn't seem intuitive.
Anyway, I think that is all sort of confusing and unnecessary. I changed
it to PHJ_BATCH_FILLING_INNER -- then when a worker who hasn't ever been
attached to this batch before attaches, it will be in the
PHJ_BATCH_FILLING_INNER phase, which it cannot help with and it will
detach and move on.
>
> +/*
> + * ExecPrepHashTableForUnmatched
> + * set up for a series of ExecScanHashTableForUnmatched calls
> + * return true if this worker is elected to do the
> unmatched inner scan
> + */
> +bool
> +ExecParallelPrepHashTableForUnmatched(HashJoinState *hjstate)
>
> Comment name doesn't match function name.
Updated -- and a few other comment updates too.
I just attached the diff.