At Thu, 28 Jul 2022 15:26:42 +0900, Fujii Masao <masao.fu...@oss.nttdata.com> wrote in > > > On 2022/07/27 10:36, Kyotaro Horiguchi wrote: > > At Tue, 26 Jul 2022 18:33:04 +0900, Fujii Masao > > <masao.fu...@oss.nttdata.com> wrote in > > 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. > > I don't think they are so identical because (as you say) they have to > handle different event IDs. So I agree we don't want to merge them.
The xact_callback and subxact_callback handles different sets of event IDs so they can be merged into one switch(). I don't think there's much difference from merging the functions for xact and subxact into one rountine then calling it with a flag to chose one of the two paths. (Even though less-than-half lines of the fuction are shared..) However, still I don't think they ought to be merged. > > 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. > > So you conclusion is to rename pgfdw_exec_pre_commit() to > pgfdw_commit_remote() or something? And the remote stuff is removed from the function. That being said, I don't mean to fight this no longer since that is rather a matter of taste. > > 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. > > Yes, that's possible. That is, pgfdw_exec_pre_commit() can construct > the query string by executing the following codes, instead of > accepting the query as an argument. But one downside of this approach > is that the following codes are executed for every remote > subtransaction entries. Maybe it's cheap to construct the query string > as follows, but I'd like to avoid any unnecessray overhead if > possible. So the patch makes the caller, pgfdw_subxact_callback(), > construct the query string only once and give it to > pgfdw_exec_pre_commit(). > > curlevel = GetCurrentTransactionNestLevel(); > snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel); That *overhead* has been there and I'm not sure how much actual impact it gives on performance (comparing to the surrounding code). But I would choose leaving it open-coded as-is than turning it into a function that need two tightly-bonded parameters passed and that also tightly bonded to the caller via the parameters. ...In other words, the original code doesn't seem to meet the requirement for a function. However, it's okay if you prefer the functions than the open-coded lines based on the above discussion, I'd stop objecting. regards. -- Kyotaro Horiguchi NTT Open Source Software Center