Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit

On 2022/01/06 17:29, Etsuro Fujita wrote:
> On Fri, Dec 3, 2021 at 6:07 PM Fujii Masao <masao.fujii@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
weneed this change?
 

The source comment explains that we intentionally ignore errors in the DEALLOCATE. But the patch changes DEALLOCATE ALL
sothat 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?
 

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

+   <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'dlike 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