At Tue, 26 Jul 2022 00:54:47 +0900, Fujii Masao <masao.fu...@oss.nttdata.com> wrote in > Hi, > > When reviewing the postgres_fdw parallel-abort patch [1], I found that > there are several duplicate codes in postgres_fdw/connection.c. > Which seems to make it harder to review the patch changing > connection.c. > So I'd like to remove such duplicate codes and refactor the functions > in connection.c. I attached the following three patches. > > There are two functions, pgfdw_get_result() and > pgfdw_get_cleanup_result(), > to get a query result. They have almost the same code, call > PQisBusy(), > WaitLatchOrSocket(), PQconsumeInput() and PQgetResult() in the loop, > but only pgfdw_get_cleanup_result() allows its callers to specify the > timeout. > 0001 patch transforms pgfdw_get_cleanup_result() to the common > function > to get a query result and makes pgfdw_get_result() use it instead of > its own (duplicate) code. The patch also renames > pgfdw_get_cleanup_result() > to pgfdw_get_result_timed().
Agree to that refactoring. And it looks fine to me. > pgfdw_xact_callback() and pgfdw_subxact_callback() have similar codes > to > issue COMMIT or RELEASE SAVEPOINT commands. 0002 patch adds the common > function, > pgfdw_exec_pre_commit(), for that purpose, and changes those functions > so that they use the common one. 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. > pgfdw_finish_pre_commit_cleanup() and > pgfdw_finish_pre_subcommit_cleanup() > have similar codes to wait for the results of COMMIT or RELEASE > SAVEPOINT commands. > 0003 patch adds the common function, pgfdw_finish_pre_commit(), for > that purpose, > and replaces those functions with the common one. > That is, pgfdw_finish_pre_commit_cleanup() and > pgfdw_finish_pre_subcommit_cleanup() > are no longer necessary and 0003 patch removes them. 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. > [1] https://commitfest.postgresql.org/38/3392/ regards. -- Kyotaro Horiguchi NTT Open Source Software Center
