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
