Re: Parallel INSERT SELECT take 2

On Thu, Jun 10, 2021 at 11:26 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.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

(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"?

(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?

(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)

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

-> should set "unsafe" not "safe"

(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


Regards,
Greg Nancarrow
Fujitsu Australia