On Thu, Jan 13, 2022 at 11:54 AM Fujii Masao <masao.fu...@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 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
v3-0001-postgres-fdw-Add-support-for-parallel-commit.patch
Description: Binary data
v3-0002-postgres_fdw-Minor-cleanup-for-pgfdw_abort_cleanup.patch
Description: Binary data
v3-0003-postgres-fdw-Add-support-for-parallel-abort.patch
Description: Binary data
