On Fri, Feb 18, 2022 at 1:46 AM Fujii Masao <masao.fujii@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
the001 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
tocommit"?
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
"RELEASESAVEPOINT" query string again? pgfdw_subxact_callback() already does them and probably we can make it pass
eitherof 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