Re: Assertion failure with barriers in parallel hash join

On Wed, Mar 31, 2021 at 6:25 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Wed, Mar 17, 2021 at 8:18 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> >
> > On Wed, Mar 17, 2021 at 6:58 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> > > According to BF animal elver there is something wrong with this
> > > commit. Looking into it.
> >
> > Assertion failure reproduced here and understood, but unfortunately
> > it'll take some more time to fix this. I've reverted the commit for
> > now to unbreak the ~5 machines that are currently showing red in the
> > build farm.
>
> So, I think the premise of the patch
> v6-0001-Fix-race-condition-in-parallel-hash-join-batch-cl.patch makes
> sense: freeing the hashtable memory should happen atomically with
> updating the flag that indicates to all others that the memory is not to
> be accessed any longer.
>
> As was likely reported by the buildfarm leading to you reverting the
> patch, when the inner side is empty and we dump out before advancing the
> build barrier past PHJ_BUILD_HASHING_OUTER, we trip the new Asserts
> you've added in ExecHashTableDetach().
>
> Assert(!pstate ||
> BarrierPhase(&pstate->build_barrier) >= PHJ_BUILD_RUNNING);
>
> Hmm.
>
> Maybe if the inner side is empty, we can advance the build barrier to
> the end?
> We help batch 0 along like this in ExecParallelHashJoinSetUpBatches().
>
> But, I'm not sure we can expect the process executing this code to be
> attached to the build barrier, can we?
>
> @@ -296,7 +304,19 @@ ExecHashJoinImpl(PlanState *pstate, bool parallel)
> * outer relation.
> */
> if (hashtable->totalTuples == 0 &&
> !HJ_FILL_OUTER(node))
> + {
> + if (parallel)
> + {
> + Barrier *build_barrier;
> +
> + build_barrier =
> &parallel_state->build_barrier;
> + while
> (BarrierPhase(build_barrier) < PHJ_BUILD_DONE)
> +
> BarrierArriveAndWait(build_barrier, 0);
> + BarrierDetach(build_barrier);
> + }
> +
> return NULL;
> + }

I've attached a new version of the patch which contains my suggested fix
for the problem with the empty inner optimization.

If you apply Thomas' injected sleeps original bug repro patch and use
the following DDL and query, you can reproduce the issue with the empty
inner optimization present in his v2 patch. Then, if you apply my
attached v3 patch, you can see that we no longer trip the assert.

drop table if exists empty_simple;
create table empty_simple(id int, col2 text);
update pg_class set reltuples = 10000 where relname = 'empty_simple';

drop table if exists simple;
create table simple as
select generate_series(1, 20000) AS id, 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
analyze simple;

set min_parallel_table_scan_size = 0;
set parallel_setup_cost = 0;
set enable_hashjoin = on;
set parallel_leader_participation to off;
set work_mem = '4MB';
set enable_parallel_hash = on;
select count(*) from empty_simple join simple using (id);

- Melanie