RE: Parallel INSERT SELECT take 2

On Thursday, June 10, 2021 1:39 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> On Thu, Jun 10, 2021 at 11:26 AM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > Through further review and thanks for greg-san's suggestions, I
> > attached a new version patchset with some minor change in 0001,0003 and
> 0004.
> >
> > 0001.
> > * fix a typo in variable name.
> > * add a TODO in patch comment about updating the version number when
> branch PG15.
> >
> > 0003
> > * fix a 'git apply white space' warning.
> > * Remove some unnecessary if condition.
> > * add some code comments above the safety check function.
> > * Fix some typo.
> >
> > 0004
> > * add a testcase to test ALTER PARALLEL DML UNSAFE/RESTRICTED.
> >
>
> Thanks, those updates addressed most of what I was going to comment on
> for the v9 patches.
>
> Some additional comments on the v10 patches:
>
> (1) I noticed some functions in the 0003 patch have no function header:
>
> make_safety_object
> parallel_hazard_walker
> target_rel_all_parallel_hazard_recurse

Thanks, added.

> (2) I found the "recurse_partition" parameter of the
> target_rel_all_parallel_hazard_recurse() function a bit confusing, because the
> function recursively checks partitions without looking at that flag. How about
> naming it "is_partition"?

Yeah, it looks better. Changed.

> (3) The names of the utility functions don't convey that they operate on tables.
>
> How about:
>
> pg_get_parallel_safety() -> pg_get_table_parallel_safety()
> pg_get_max_parallel_hazard() -> pg_get_table_max_parallel_hazard()
>
> or pg_get_rel_xxxxx()?
>
> What do you think?

I changed it like the following:
pg_get_parallel_safety -> pg_get_table_parallel_dml_safety
pg_get_max_parallel_hazard -> pg_get_table_max_parallel_dml_hazard

> (4) I think that some of the tests need parallel dml settings to match their
> expected output:
>
> (i)
> +-- Test INSERT with underlying query - and RETURNING (no projection)
> +-- (should create a parallel plan; parallel SELECT)
>
> -> but creates a serial plan (so needs to set parallel dml safe, so a
> parallel plan is created)

Changed.

> (ii)
> +-- Parallel INSERT with unsafe column default, should not use a
> +parallel plan
> +--
> +alter table testdef parallel dml safe;
>
> -> should set "unsafe" not "safe"

I thought the testcase about table 'testdef' is to test if the planner is able to
check whether it has parallel unsafe or restricted column default expression,
because column default expression will be merged into select part in planner.
So, It seems we don't need to change the table's parallel safety for these cases ?

> (iii)
> +-- Parallel INSERT with restricted column default, should use parallel
> +SELECT
> +--
> +explain (costs off) insert into testdef(a,b,d) select a,a*2,a*8 from
> +test_data;
>
> -> should use "alter table testdef parallel dml restricted;" before the
> -> explain
>
> (iv)
> +--
> +-- Parallel INSERT with restricted and unsafe column defaults, should
> not use a parallel plan
> +--
> +explain (costs off) insert into testdef(a,d) select a,a*8 from
> +test_data;
>
> -> should use "alter table testdef parallel dml unsafe;" before the
> -> explain

I addressed most of the comments and rebased the patch.
Besides, I changed the following things:
* I removed the safety check for index-am function as discussed[1].
* change version 140000 to 150000

Attach new version patchset for further review.

[1] https://www.postgresql.org/message-id/164474.1623652853%40sss.pgh.pa.us

Best regards,
houzj