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

On Tue, Jun 19, 2018 at 2:13 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
>
> In the reported testcase, parallel_workers is set to 0 for all partition
> (child) relations. Which means partial parallel paths are not possible for
> child rels. However, the parent can have partial parallel paths. Thus, when
> we have a full partitionwise aggregate possible (as the group by clause
> matches with the partition key), we end-up in a situation where we do create
> a partially_grouped_rel for the parent but there won't be any
> partially_grouped_live_children.
>
> The current code was calling add_paths_to_append_rel() without making sure
> of any live children present or not (sorry, it was my fault). This causes an
> Assertion failure in add_paths_to_append_rel() as this function assumes that
> it will have non-NIL live_childrels list.
>

Thanks for the detailed explanation.

> I have fixed partitionwise aggregate code which is calling
> add_paths_to_append_rel() by checking the live children list correctly. And
> for robustness, I have also added an Assert() in add_paths_to_append_rel().
>
> I have verified the callers of add_paths_to_append_rel() and except one, all
> are calling it by making sure that they have a non-NIL live children list.

I actually thought about moving if(live_childrel != NIL) test inside
this function, but then every caller is doing something different when
that condition occurs. So doesn't help much.

> The one which is calling add_paths_to_append_rel() directly is
> set_append_rel_pathlist(). And I think, at that place, we will never have an
> empty live children list,

Yes, I think so too. That's because set_append_rel_size() should have
marked such a parent as dummy and subsequent set_rel_pathlist() would
not create any paths for it.

Here are some review comments.

+    /* We should end-up here only when we have few live child rels. */

I think the right wording is " ... we have at least one child." I was actually
thinking if we should just return from here when live_children == NIL. But then
we will loose an opportunity to catch a bug earlier than set_cheapest().

+     * However, if there are no live children, then we cannot create any append
+     * path.

I think "no live children" is kind of misleading here. We usually use that term
to mean at least one non-dummy child. But in this case there is no child at
all, so we might want to better describe this situation. May be also explain
when this condition can happen.

+    if (patype == PARTITIONWISE_AGGREGATE_FULL && grouped_live_children != NIL)

I think for this to happen, the parent grouped relation and the underlying scan
itself should be dummy. So, would an Assert be better? I think we have
discussed this earlier, but I can not spot the mail.

+-- Test when partition tables has parallel_workers = 0 but not the parent

Better be worded as "Test when parent can produce parallel paths but not any of
its children". parallel_worker = 0 is just a means to test that. Although the
EXPLAIN output below doesn't really reflect that parent can have parallel
plans. Is it possible to create a scenario to show that.

I see that you have posted some newer versions of this patch, but I
think those still need to address these comments.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company