At Tue, 26 Jul 2022 18:33:04 +0900, Fujii Masao <masao.fu...@oss.nttdata.com> wrote in > > 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.
I didn't see it from that viewpoint but I don't think that unconditionally justifies other refactoring. If we merge pgfdw_finish_pre_(sub)?commit_cleanup()s this way, in turn pgfdw_subxact_callback() and pgfdw_xact_callback() are going to be almost identical except event IDs to handle. But I don't think we would want to merge them. A concern on 0002 is that it is hiding the subxact-specific steps from the subxact callback. It would look reasonable if it were called from two or more places for each topleve and !toplevel, but actually it has only one caller for each. So I think that pgfdw_exec_pre_commit should not do that and should be renamed to pgfdw_commit_remote() or something. On the other hand pgfdw_finish_pre_commit() hides toplevel-specific steps from the caller so the same argument holds. Another point that makes me concern about the patch is the new function takes an SQL statement, along with the toplevel flag. I guess the reason is that the command for subxact (RELEASE SAVEPOINT %d) requires the current transaction level. However, the values isobtainable very cheap within the cleanup functions. So I propose to get rid of the parameter "sql" from the two functions. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
