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'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.
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.
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?
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);
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
