On Thu, Jan 13, 2022 at 11:54 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> At first I'm reading the 0001 patch. Here are the comments for the patch.
Thanks for reviewing!
> 0001 patch failed to be applied. Could you rebase the patch?
Done. Attached is an updated version of the patch set.
> + entry->changing_xact_state = true;
> + do_sql_command_begin(entry->conn, "DEALLOCATE ALL");
> + pending_deallocs = lappend(pending_deallocs, entry);
>
> Originally entry->changing_xact_state is not set to true when executing DEALLOCATE ALL. But the patch do that. Why do
weneed this change?
>
> The source comment explains that we intentionally ignore errors in the DEALLOCATE. But the patch changes DEALLOCATE
ALLso that it's executed via do_sql_command_begin() that can cause an error. Is this OK?
>
> + if (ignore_errors)
> + pgfdw_report_error(WARNING, res, conn, true, sql);
>
> When DEALLOCATE fails, originally even warning message is not logged. But the patch changes DEALLOCATE so that its
resultis received via do_sql_command_end() that can log warning message even when ignore_errors argument is enabled.
Whydo we need to change the behavior?
Yeah, we don’t need to change the behavior as discussed before, so I
fixed these. I worked on the patch after a while, so I forgot about
that. :-(
> + <para>
> + This option controls whether <filename>postgres_fdw</filename> commits
> + multiple remote (sub)transactions opened in a local (sub)transaction
> + in parallel when the local (sub)transaction commits.
>
> Since parallel_commit is an option for foreign server, how the server with this option enabled is handled by
postgres_fdwshould be documented, instead?
Agreed. I rewrote this slightly like the attached. Does that make sense?
> + <para>
> + Note that if many remote (sub)transactions are opened on a remote server
> + in a local (sub)transaction, this option might increase the remote
> + server’s load significantly when those remote (sub)transactions are
> + committed. So be careful when using this option.
> + </para>
>
> This paragraph should be inside the listitem for parallel_commit, shouldn't it?
I put this note outside, because it’s rewritten to a note about both
the parallel_commit and parallel_abort options in the following patch.
But it would be good to make the parallel-commit patch independent, so
I moved it into the list.
> async_capable=true also may cause the similar issue? If so, this kind of note should be documented also in
async_capable?
That’s right. I think it would be good to add a similar note about
that, but I’d like to leave that for another patch.
> This explains that the remote server's load will be increased *significantly*. But "significantly" part is really
true?
I think that that would depend on how many transactions are committed
on the remote side at the same time. But the word “significantly”
might be too strong, so I dropped the word.
> I'd like to know how much parallel_commit=true actually can increase the load in a remote server.
Ok, I’ll do a load test.
About the #0002 patch:
This is in preparation for the parallel-abort patch (#0003), but I’d
like to propose a minor cleanup for commit 85c696112: 1) build an
abort command to be sent to the remote in pgfdw_abort_cleanup(), using
a macro, only when/if necessary, as before, and 2) add/modify comments
a little bit.
Sorry for the delay again.
Best regards,
Etsuro Fujita
