Re: Server crashed with TRAP: FailedAssertion("!(parallel_workers > 0)" when partitionwise_aggregate true.

On Fri, Jun 22, 2018 at 6:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Fri, Jun 22, 2018 at 2:26 AM, Rajkumar Raghuwanshi
> <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:
> > I have applied patch and checked reported issue. Patch applied cleanly
> and
> > issues not reproducible any more.
>
> Committed, with a few changes:
>

Thanks Robert.

>
> - Renamed found_partially_grouped_child to partial_grouping_valid.
> The old name seemed to me to be a poor choice, because it sounds from
> the name like it gets set to true whenever we've found at least one
> partially grouped child, whereas really it gets set to false whenever
> we've failed to find at least one partially grouped child. The new
> name is also more like the names in add_paths_to_append_rel.
>
> - Modified the wording of the comment.
>
> - Omitted the new assertion in add_paths_to_append_rel. I doubt
> whether that's correct. I don't see any obvious reason why
> live_childrels can't be NIL there, and further down I see this:
>
> /*
> * If we found unparameterized paths for all children, build
> an unordered,
> * unparameterized Append path for the rel. (Note: this is
> correct even
> * if we have zero or one live subpath due to constraint
> exclusion.)
> */
>
> If it's not possible to have no live_childrels, then that comment is
> highly suspect.
>
>
OK, do these comments also holds true for partial_subpaths?

If we have NIL live_childrels, then the Assert() present inside the "if
(partial_subpaths_valid)" block will hit. Which I think is wrong.

We either need to remove these Asserts altogether or we should enter inside
this block only when we have non-NIL partial_subpaths. Also, if we don't
have any partial_subpaths, then variable partial_subpaths_valid should be
false. Currently, by default, it is set to true due to which we are ending
up inside that if block and then hitting an Assert.

> Also, even if this assertion *is* correct, I think it needs a better
> comment explaining why it's correct, because there isn't anything
> obvious in set_append_rel_pathlist that keeps IS_DUMMY_REL() from
> being true for every child.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company