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

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