Обсуждение: Assumptions about the number of parallel workers

I can't figure out why ExecGather/ExecGatherMerge do check whether num_workers
is non-zero. I think the code would be a bit clearer if these tests were
replaced with Assert() statements, as the attached patch does.

In addition, if my assumptions are correct, I think that only Gather node
needs the single_copy field, but GatherPath does not.

In the patch I also added Assert() to add_partial_path so that I'm more likely
to catch special cases. Regression tests passed though.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Hi,

On 2020-02-05 10:50:05 +0100, Antonin Houska wrote:
> I can't figure out why ExecGather/ExecGatherMerge do check whether num_workers
> is non-zero. I think the code would be a bit clearer if these tests were
> replaced with Assert() statements, as the attached patch does.

It's probably related to force_parallel_mode. With that we'll IIRC
generate gather nodes even if num_workers == 0.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> On 2020-02-05 10:50:05 +0100, Antonin Houska wrote:
> > I can't figure out why ExecGather/ExecGatherMerge do check whether num_workers
> > is non-zero. I think the code would be a bit clearer if these tests were
> > replaced with Assert() statements, as the attached patch does.
>
> It's probably related to force_parallel_mode. With that we'll IIRC
> generate gather nodes even if num_workers == 0.

Those Gather nodes still have non-zero num_workers, see this part of
standard_planner:

    if (force_parallel_mode != FORCE_PARALLEL_OFF && top_plan->parallel_safe)
    {
    ...
        gather->num_workers = 1;
    gather->single_copy = true;

Also, if it num_workers was zero for any reason, my patch would probably make
regression tests fail. However I haven't noticed any assertion failure.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

On Wed, Feb 5, 2020 at 4:49 AM Antonin Houska <ah@cybertec.at> wrote:
> I can't figure out why ExecGather/ExecGatherMerge do check whether num_workers
> is non-zero. I think the code would be a bit clearer if these tests were
> replaced with Assert() statements, as the attached patch does.

Hmm. There are some cases where we plan on using a Gather node but
then can't actually fire up parallelism because we run out of DSM
segments or we run out of background workers. But the Gather is just
part of the plan, so it would still have num_workers > 0 in those
cases. This might just have been a thinko on my part, but I'm not
totally sure.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2020-02-07 09:44:34 +0100, Antonin Houska wrote:
> Those Gather nodes still have non-zero num_workers, see this part of
> standard_planner:
> 
>     if (force_parallel_mode != FORCE_PARALLEL_OFF && top_plan->parallel_safe)
>     {
>     ...
>         gather->num_workers = 1;
>     gather->single_copy = true;

Ick. Looks like you might be right...


> Also, if it num_workers was zero for any reason, my patch would probably make
> regression tests fail. However I haven't noticed any assertion failure.

That however, is not at all guaranteed. The regression tests don't run
(or at least not much) with force_parallel_mode set. You can try
yourself though, with something like

PGOPTIONS='-c force_parallel_mode=regress' make check

Greetings,

Andres Freund