Re: Refactoring postgres_fdw/connection.c

At Thu, 28 Jul 2022 15:26:42 +0900, Fujii Masao <masao.fujii@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.fujii@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