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

Thanks a lot for the patch update.

On 2022-05-02 1:25 a.m., Etsuro Fujita wrote:
> Hi,
>
> On Wed, Apr 20, 2022 at 4:55 AM David Zhang <david(dot)zhang(at)highgo(dot)ca> wrote:
>> I tried to apply the patch to master and plan to run some tests, but got
>> below errors due to other commits.
> I rebased the patch against HEAD. Attached is an updated patch.

Applied the patch v8 to master branch today, and the `make check` is OK.
I also repeated previous performance tests on three virtual Ubuntu
18.04, and the performance improvement of parallel abort in 10 times
average is more consistent.

before:

    abort.1 = 2.6344 ms
    abort.2 = 4.2799 ms

after:

    abort.1 = 1.4105 ms
    abort.2 = 2.2075 ms

>> + * remote server in parallel at (sub)transaction end.
>>
>> Here, I think the comment above could potentially apply to multiple
>> remote server(s).
> I agree on that point, but I think it's correct to say "the remote
> server" here, because we determine this for the given remote server.
> Maybe I'm missing something, so could you elaborate on it?
Your understanding is correct. I was thinking `remote server(s)` would
be easy for end user to understand but this is a comment in source code,
so either way is fine for me.
>> Not sure if there is a way to avoid repeated comments? For example, the
>> same comment below appears in two places (line 231 and line 296).
>>
>> + /*
>> + * If requested, consume whatever data is available from the socket.
>> + * (Note that if all data is available, this allows
>> + * pgfdw_get_cleanup_result to call PQgetResult without forcing the
>> + * overhead of WaitLatchOrSocket, which would be large compared to the
>> + * overhead of PQconsumeInput.)
>> + */
> IMO I think it's OK to have this in multiple places, because 1) IMO it
> wouldn't be that long, and 2) we already duplicated comments like this
> in the same file in v14 and earlier. Here is such an example in
> pgfdw_xact_callback() and pgfdw_subxact_callback() in that file in
> those versions:
>
> /*
> * If a command has been submitted to the remote server by
> * using an asynchronous execution function, the command
> * might not have yet completed. Check to see if a
> * command is still being processed by the remote server,
> * and if so, request cancellation of the command.
> */
>
> Thanks for reviewing! Sorry for the delay.
>
> Best regards,
> Etsuro Fujita
--
Best regards,
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca