On 2022/01/06 17:29, Etsuro Fujita wrote:
On Fri, Dec 3, 2021 at 6:07 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
On 2021/11/16 18:55, Etsuro Fujita wrote:
I changed my mind; I’ll update the patch to ignore the error as
before, because 1) as far as I know, there are no reports from the
field concerning that we ignore all kinds of errors in cleaning up the
prepared statements, so maybe we don’t need to change that, and 2) we
already committed at least one of the remote transactions, so it’s not
good to abort the local transaction unless we really have to.

Done.

Are you planning to update the patch? In addition to this change,
at least documentation about new parallel_commit parameter needs
to be included in the patch.

Done.  Attached is a new version.

* 0001
This is an updated version of the previous patch.  In addition to the
above, I expanded a comment in do_sql_command_end() a bit to explain
why we do PQconsumeInput() before doing pgfdw_get_result(), to address
your comment.  Also, I moved the code to finish closing pending
(sub)transactions in pgfdw_xact_callback()(pgfdw_subxact_callback())
into separate functions.  Also, I modified regression test cases a bit
to access multiple foreign servers.

Thanks for updating the patch!

At first I'm reading the 0001 patch. Here are the comments for the patch.

0001 patch failed to be applied. Could you rebase the patch?

+                       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 we need this change?

The source comment explains that we intentionally ignore errors in the 
DEALLOCATE. But the patch changes DEALLOCATE ALL so 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 result is received via 
do_sql_command_end() that can log warning message even when ignore_errors 
argument is enabled. Why do we need to change the behavior?

+      <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_fdw should be documented, instead?

+   <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?

async_capable=true also may cause the similar issue? If so, this kind of note 
should be documented also in async_capable?

This explains that the remote server's load will be increased *significantly*. But 
"significantly" part is really true? I'd like to know how much 
parallel_commit=true actually can increase the load in a remote server.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply via email to