On 2022/07/26 16:25, Kyotaro Horiguchi wrote:
> Agree to that refactoring. And it looks fine to me.
Thanks for reviewing the patches!
> I'm not sure the two are similar with each other. The new function
> pgfdw_exec_pre_commit() looks like a merger of two isolated code paths
> intended to share a seven-line codelet. I feel the code gets a bit
> harder to understand after the change. I mildly oppose to this part.
If so, we can pgfdw_exec_pre_commit() into two, one is the common
function that sends or executes the command (i.e., calls
do_sql_command_begin() or do_sql_command()), and another is
the function only for toplevel. The latter function calls
the common function and then executes DEALLOCATE ALL things.
But this is not the way that other functions like pgfdw_abort_cleanup()
is implemented. Those functions have both codes for toplevel and
!toplevel (i.e., subxact), and run the processings depending
on the argument "toplevel". So I'm thinking that
pgfdw_exec_pre_commit() implemented in the same way is better.
> It gives the same feeling with 0002. Considering that
> pending_deallocate becomes non-NIL only when toplevel, 38 lines out of
> 66 lines of the function are the toplevel-dedicated stuff.
Same as above.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
