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