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

On 2021/11/07 18:06, Etsuro Fujita wrote:
> On Mon, Nov 1, 2021 at 3:22 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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