Message-ID: <7e6f4400-ad1c-c87d-42b8-ad906e0ed39e@highgo.ca>
Date: Wed, 4 May 2022 14:38:54 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.8.1
Subject: Re: postgres_fdw: commit remote (sub)transactions in parallel during
 pre-commit
Content-Language: en-US
To: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: 
 <CAPmGK17dAZCXvwnfpr1eTfknTGdt=hYTV9405Gt5SqPOX8K84w@mail.gmail.com>
 <CAPmGK158hrd=ZfXmgkmNFHivgh18e4oE2Gz151C2Q4OBDjZ08A@mail.gmail.com>
 <3e57ebd7-b536-333e-c877-49a3ebb57079@oss.nttdata.com>
 <CAPmGK17nfEZkKhqoP8fAPKbnfSpf2JVgV5+_Xjv2mn8P-4X0hQ@mail.gmail.com>
 <2a2519b5-0c1f-87c8-5461-a4d8be0728e0@oss.nttdata.com>
 <CAPmGK152dsQHAzZC-g55M08Z-ur-h8m4J7FD0L3eh5uazCmvAQ@mail.gmail.com>
 <883589bb-077f-7d66-2406-5f67be0372a6@oss.nttdata.com>
 <CAPmGK16sRsm30a_1dpWPkVoTANg=F3H1vLRLyek-Xcw_r7JPkQ@mail.gmail.com>
 <CAPmGK15GPmCo7xBFKsi9zZnsRV-xbi8axFSRdFUzwpGBfu9NDg@mail.gmail.com>
 <CAPmGK15mbbsEEEULK9XTAy4c78JnCfhtZ_AnPUyjjGXSe-sbGA@mail.gmail.com>
 <CAPmGK15SkX=1NrY+GVyZan4Za+f17FFnM9qtQvjCEMaE3ftPcg@mail.gmail.com>
 <CAPmGK16ABpu+ff1Wiwj3MztbDi2qFNmFEa_andc3LmVbCvADCw@mail.gmail.com>
 <CAPmGK16RWyc1nJsFnsn7-=UFzQiq4OGm70PK4-=5Y2fH_Gr4iA@mail.gmail.com>
 <a4c7fd7a-4bcd-a783-9055-2d85d8299217@highgo.ca>
 <CAPmGK15DH+oqo=Lgn4S3ACnU3C07fSDYW_fKt09hVHskopQzxQ@mail.gmail.com>
From: David Zhang <david.zhang@highgo.ca>
In-Reply-To: 
 <CAPmGK15DH+oqo=Lgn4S3ACnU3C07fSDYW_fKt09hVHskopQzxQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/7e6f4400-ad1c-c87d-42b8-ad906e0ed39e%40highgo.ca>
Precedence: bulk

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.zhang@highgo.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