On Fri, Feb 18, 2022 at 1:46 AM Fujii Masao <masao.fu...@oss.nttdata.com> wrote: > 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.
OK > + * 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"? Agreed. I’ll change it as such. > "remote server" should be "remote servers"? Maybe I’m missing something, but we determine this for the given remote server, so it seems to me correct to say “the remote server”, not “the 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. Yeah, that would save cycles, but I think that that makes code a bit unclean IMO. (To save cycles, I think we could also modify pgfdw_subxact_callback() to reuse the query in the while loop in that function when processing multiple open remote subtransactions there, but that would make code a bit complicated, so I don’t think it’s a good idea to do so, either.) So I’d vote for reconstructing the query in pgfdw_finish_pre_subcommit_cleanup() as we do in pgfdw_subxact_callback(). To avoid calling GetCurrentTransactionNestLevel() again, I think we could pass the curlevel variable to that function. > + 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"? I thought it would be good to say “a foreign server”, not “foreign servers”, because it makes clear that even remote transactions opened on a single foreign server are committed in parallel. (To say that this option is not for a specific foreign server, I added to the documentation “This option can only be specified for foreign servers”.) > "in a local (sub)transaction" part seems not to be necessary. And I thought adding it would make clear which remote transactions are committed in parallel. But maybe I’m missing something, so could you elaborate a bit more on these? Thanks for reviewing! Best regards, Etsuro Fujita
