Message-ID: <66d99132-5946-ba74-ffa6-620eb50b7a8f@highgo.ca>
Date: Fri, 18 Feb 2022 13:55:19 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.6.0
Subject: Re: postgres_fdw: commit remote (sub)transactions in parallel during
 pre-commit
Content-Language: en-US
To: Fujii Masao <masao.fujii@oss.nttdata.com>,
 Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <CAPmGK17dAZCXvwnfpr1eTfknTGdt=hYTV9405Gt5SqPOX8K84w@mail.gmail.com>
 <65af8fb4-4c45-9cc3-c6ec-80f3ac488e07@oss.nttdata.com>
 <CAPmGK17XczZmzU=0mhk2_oG9Omo1p-uZCNdZAh_3vuC6+YboiA@mail.gmail.com>
 <95066202-c7e6-e4be-05cc-bdceb45b68c7@oss.nttdata.com>
 <CAPmGK14rr1R_t4xxWyJVng04nDjGnxi=J5fz-TBNBH9Xn0OFYQ@mail.gmail.com>
 <2d7c480a-63c8-a244-0f2a-361e5097764f@oss.nttdata.com>
 <CAPmGK15FuPVGx3TGHKShsbPKKtF1y58-ZLcKoxfN-nqLj1dZ=g@mail.gmail.com>
 <d888307d-c5b2-88e7-e2c0-f2228ad0756b@oss.nttdata.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>
From: David Zhang <david.zhang@highgo.ca>
In-Reply-To: <2a2519b5-0c1f-87c8-5461-a4d8be0728e0@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/66d99132-5946-ba74-ffa6-620eb50b7a8f%40highgo.ca>
Precedence: bulk

Thanks a lot for updating the patch.

Tried to apply the patches to master branch, no warning found and 
regression test passed.

Now, we have many places (5) calling the same function with a constant 
number 30000. Is this a good time to consider redefine this number a 
macro somewhere?

Thank you,

On 2022-02-17 8:46 a.m., Fujii Masao wrote:
>
>
> On 2022/02/11 21:59, Etsuro Fujita wrote:
>> I tweaked comments/docs a little bit as well.
>
> Thanks for updating the patches!
>
> I reviewed 0001 patch. It looks good to me except the following minor 
> things. If these are addressed, I think that the 001 patch can be 
> marked as ready for committer.
>
> +     * Also determine to commit (sub)transactions opened on the 
> remote server
> +     * in parallel at (sub)transaction end.
>
> Like the comment "Determine whether to keep the connection ...", 
> "determine to commit" should be "determine whether to commit"?
>
> "remote server" should be "remote servers"?
>
>
> +    curlevel = GetCurrentTransactionNestLevel();
> +    snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel);
>
> Why does pgfdw_finish_pre_subcommit_cleanup() need to call 
> GetCurrentTransactionNestLevel() and construct the "RELEASE SAVEPOINT" 
> query string again? pgfdw_subxact_callback() already does them and 
> probably we can make it pass either of them to 
> pgfdw_finish_pre_subcommit_cleanup() as its argument.
>
>
> +       This option controls whether <filename>postgres_fdw</filename> 
> commits
> +       remote (sub)transactions opened on a foreign server in a local
> +       (sub)transaction in parallel when the local (sub)transaction 
> commits.
>
> "a foreign server" should be "foreign servers"?
>
> "in a local (sub)transaction" part seems not to be necessary.
>
> Regards,
>
-- 
David

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