On 2021/11/07 18:06, Etsuro Fujita wrote:
On Mon, Nov 1, 2021 at 3:22 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
On 2021/10/31 18:05, Etsuro Fujita wrote:
The patch is pretty simple: if a server option added
by the patch “parallel_commit” is enabled,
Could you tell me why the parameter is necessary?
Can't we always enable the feature?
I don’t think parallel commit would cause performance degradation,
even in the case when there is just a single remote (sub)transaction
to commit when called from pgfdw_xact_callback
(pgfdw_subxact_callback), so I think it might be OK to enable it by
default. But my concern about doing so is the remote side: during
those functions, if there are a lot of (sub)transactions on a single
remote server that need to be committed, parallel commit would
increase the remote server’s load at (sub)transaction end than serial
commit, which is the existing implementation, as the requests to
commit those (sub)transactions are sent to the remote server at the
same time; which some users might want to avoid.
Thanks for explaining this! But probably I failed to get your point.
Sorry... Whichever parallel or serial commit approach, the number of
transactions to commit on the remote server is the same, isn't it?
For example, please imagine the case where a client requests
ten transactions per second to the local server. Each transaction
accesses to the foreign table, so which means that ten transaction
commit operations per second are requested to the remote server.
Unless I'm missing something, this number doesn't change whether
the foreign transaction is committed in parallel way or not.
Thought?
I think we could extend this to abort cleanup of remote
(sub)transactions during post-abort. Anyway, I think this is useful,
so I’ll add this to the upcoming commitfest.
Thanks!
I'll update the patch as such in the next version.
IMO it's better to implement and commit these features gradually
if possible. Which would simplify the patch and make it
easier-to-review. So I think that it's better to implement
this feature as a separate patch.
+ /* Consume whatever data is available from the socket */
+ if (!PQconsumeInput(conn))
+ pgfdw_report_error(ERROR, NULL, conn, false, sql);
Without the patch, PQconsumeInput() is not called before pgfdw_get_result()
But could you tell me why you added PQconsumeInput() there?
The reason is that there might be the result already before calling
pgfdw_get_result(), in which case PQconsumeInput() followed by
PQisBusy() would allow us to call PQgetResult() without doing
WaitLatchOrSocket(), which I think is rather expensive.
Understood. It's helpful to add the comment about why PQconsumeInput()
is called there.
Also could you tell me how much expensive it is?
When ignore_errors argument is true, the error reported by
PQconsumeInput() should be ignored?
I’m not sure about that, because the error might be caused by e.g.,
OOM in the local server, in which case I don’t think it is safe to
ignore it and continue the (sub)transaction-end processing.
But the existing code ignores the error at all, doesn't it?
If it's unsafe to do that, probably we should fix that at first?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
