On 2022/02/11 21:59, Etsuro Fujita wrote:
I tweaked comments/docs a little bit as well.
Thanks for updating the patches! I reviewed 0001 patch. It looks good to me except the following minor things. If these are addressed, I think that the 001 patch can be marked as ready for committer. + * Also determine to commit (sub)transactions opened on the remote server + * in parallel at (sub)transaction end. Like the comment "Determine whether to keep the connection ...", "determine to commit" should be "determine whether to commit"? "remote server" should be "remote servers"? + curlevel = GetCurrentTransactionNestLevel(); + snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel); Why does pgfdw_finish_pre_subcommit_cleanup() need to call GetCurrentTransactionNestLevel() and construct the "RELEASE SAVEPOINT" query string again? pgfdw_subxact_callback() already does them and probably we can make it pass either of them to pgfdw_finish_pre_subcommit_cleanup() as its argument. + This option controls whether <filename>postgres_fdw</filename> commits + remote (sub)transactions opened on a foreign server in a local + (sub)transaction in parallel when the local (sub)transaction commits. "a foreign server" should be "foreign servers"? "in a local (sub)transaction" part seems not to be necessary. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
