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

On Thu, Jan 13, 2022 at 11:54 AM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)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 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?

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_fdw should 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