Hi, For v3_0003-reloption-parallel_dml-src.patch : + * Check if parallel_dml_enabled is enabled for the target table, + * if not, skip the safety checks and return PARALLEL_UNSAFE.
Looks like the return value is true / false. So the above comment should be adjusted. + if (!RelationGetParallelDML(rel, true)) + { + table_close(rel, NoLock); + return false; + } + + table_close(rel, NoLock); Since the rel would always be closed, it seems the return value from RelationGetParallelDML() can be assigned to a variable, followed by call to table_close(), then the return statement. Cheers On Mon, Feb 1, 2021 at 3:56 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > On Mon, Feb 1, 2021 at 4:02 PM Hou, Zhijie <houzj.f...@cn.fujitsu.com> > wrote: > > > > Attatching v2 patch which addressed the comments above. > > > > Some further refactor: > > > > Introducing a new function is_parallel_possible_for_modify() which > decide whether to do safety check. > > > > IMO, It seems more readable to extract all the check that we can do > before the safety-check and put them > > in the new function. > > > > Please consider it for further review. > > > > I've updated your v2 patches and altered some comments and > documentation changes (but made no code changes) - please compare > against your v2 patches, and see whether you agree with the changes to > the wording. > In the documentation, you will also notice that in your V2 patch, it > says that the "parallel_dml_enabled" table option defaults to false. > As it actually defaults to true, I changed that in the documentation > too. > > Regards, > Greg Nancarrow > Fujitsu Australia >