Hi,

On 2017-12-21 10:35:06 -0500, Robert Haas wrote:
> Great subject line!

Had started written the email before finishing testing, forgot to fill
in the blank. mutt annoyingly aborts with an empty subject, hence
smashing one key a couple times.


> On Thu, Dec 21, 2017 at 10:18 AM, Robert Haas <robertmh...@gmail.com> wrote:
> > If I run the regression tests with force_parallel_mode=on prior to the
> > parallel hash join patch, they pass.  If I run them now, they fail
> > inside the parallel hash join tests here:
> >
> > create table wide as select generate_series(1, 2) as id, rpad('',
> > 320000, 'x') as t;
> >
> > I'm guessing that test case would have failed before, too, but we
> > didn't have it.  I'll analyze this further in a bit.
> 
> I think this is just a poorly-written assertion.  currentCommandIdUsed
> is only used to skip redundant increments of the command counter, and
> CommandCounterIncrement() is categorically denied under parallelism
> anyway.  Therefore, it's OK for this to happen in parallel mode; we
> just need to be in the leader, not the worker.
> 
> Therefore, I proposed the attached patch, which fixes the regression
> test crash for me.


> diff --git a/src/backend/access/transam/xact.c 
> b/src/backend/access/transam/xact.c
> index d430e662e6..b37510c24f 100644
> --- a/src/backend/access/transam/xact.c
> +++ b/src/backend/access/transam/xact.c
> @@ -683,12 +683,12 @@ GetCurrentCommandId(bool used)
>       if (used)
>       {
>               /*
> -              * Forbid setting currentCommandIdUsed in parallel mode, 
> because we
> -              * have no provision for communicating this back to the master. 
>  We
> +              * Forbid setting currentCommandIdUsed in a parallel worker, 
> because
> +              * we have no provision for communicating this back to the 
> master.  We
>                * could relax this restriction when currentCommandIdUsed was 
> already
>                * true at the start of the parallel operation.
>                */
> -             Assert(CurrentTransactionState->parallelModeLevel == 0);
> +             Assert(!IsParallelWorker());
>               currentCommandIdUsed = true;
>       }
>       return currentCommandId;

Yea, that looks reasonable to me.

Greetings,

Andres Freund

Reply via email to