Message-ID: <2a2519b5-0c1f-87c8-5461-a4d8be0728e0@oss.nttdata.com>
Date: Fri, 18 Feb 2022 01:46:38 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.5.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: 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>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <CAPmGK17nfEZkKhqoP8fAPKbnfSpf2JVgV5+_Xjv2mn8P-4X0hQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/2a2519b5-0c1f-87c8-5461-a4d8be0728e0%40oss.nttdata.com>
Precedence: bulk


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,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION