Re: [HACKERS] why not parallel seq scan for slow functions

On Thu, Feb 15, 2018 at 4:18 PM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>
>

After recent commits, the patch doesn't get applied cleanly, so
rebased it and along the way addressed the comments raised by you.

> Here are some comments on the patch.
>
> + /*
> + * Except for the topmost scan/join rel, consider gathering
> + * partial paths. We'll do the same for the topmost scan/join
> This function only works on join relations. Mentioning scan rel is confusing.
>

Okay, removed the 'scan' word from the comment.

> index 6e842f9..5206da7 100644
> --- a/src/backend/optimizer/path/allpaths.c
> +++ b/src/backend/optimizer/path/allpaths.c
> @@ -481,14 +481,21 @@ set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
> }
>
> + *
> + * Also, if this is the topmost scan/join rel (that is, the only baserel),
> + * we postpone this until the final scan/join targelist is available (see
>
> Mentioning join rel here is confusing since we deal with base relations here.
>

Okay, removed the 'join' word from the comment.

> + bms_membership(root->all_baserels) != BMS_SINGLETON)
>
> set_tablesample_rel_pathlist() is also using this method to decide whether
> there are any joins in the query. May be macro-ize this and use that macro at
> these two places?
>

maybe, but I am not sure if it improves the readability. I am open to
changing it if somebody else also feels it is better to macro-ize this
usage.

> - * for the specified relation. (Otherwise, add_partial_path might delete a
> + * for the specified relation. (Otherwise, add_partial_path might delete a
>
> Unrelated change?
>

oops, removed.

> + /* Add projection step if needed */
> + if (target && simple_gather_path->pathtarget != target)
>
> If the target was copied someplace, this test will fail. Probably we want to
> check containts of the PathTarget structure? Right now copy_pathtarget() is not
> called from many places and all those places modify the copied target. So this
> isn't a problem. But we can't guarantee that in future. Similar comment for
> gather_merge path creation.
>

I am not sure if there is any use of copying the path target unless
you want to modify it , but anyway we use the check similar to what is
used in patch in the multiple places in code. See
create_ordered_paths. So, we need to change all those places first if
we sense any such danger.

> + simple_gather_path = apply_projection_to_path(root,
> + rel,
> + simple_gather_path,
> + target);
> +
>
> Why don't we incorporate those changes in create_gather_path() by passing it
> the projection target instead of rel->reltarget? Similar comment for
> gather_merge path creation.
>

Nothing important, just for the sake of code consistency, some other
places in code uses it this way. See create_ordered_paths. Also, I am
not sure if there is any advantage of doing it inside
create_gather_path.

> + /*
> + * Except for the topmost scan/join rel, consider gathering
> + * partial paths. We'll do the same for the topmost scan/join rel
>
> Mentioning scan rel is confusing here.
>

Okay, changed.

>
> deallocate tenk1_count;
> +explain (costs off) select ten, costly_func(ten) from tenk1;
>
> verbose output will show that the parallel seq scan's targetlist has
> costly_func() in it. Isn't that what we want to test here?
>

Not exactly, we want to just test whether the parallel plan is
selected when the costly function is used in the target list.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com